Block script performance with shared image files

Create the list of shared loopback devices from within check_sharing, rather than calling check_sharing for every loopback device using the same shared image.

This change prevents the xenstore database from being walked for every shared device, which causes an exponential decrease in performance.

Signed-off-by Mike Latimer <[email protected]>

---
 tools/hotplug/Linux/block | 67 +++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 26 deletions(-)

tools/hotplug/Linux/block +6 -3

index 8d2ee9d..aef051c 100644
@@ -38,7 +38,7 @@ find_free_loopback_dev() {
 }
 
 ##
-# check_sharing device mode
+# check_sharing devtype device mode [inode]
 #
 # Check whether the device requested is already in use.  To use the device in
 # read-only mode, it may be in use in read-only mode, but may not be in use in

@@ -56,10 +56,27 @@ find_free_loopback_dev() {
 #
 check_sharing()
 {
-  local dev="$1"
-  local mode="$2"
+  local devtype=$1
+  local dev="$2"
+  local mode="$3"
+
+  if [ "$devtype" = "file" ];

tools/hotplug/Linux/block +5 -1

+      fi
+    done
+  else
+    local devmm="$(device_major_minor "$dev"),"
+  fi
 
-  local devmm=$(device_major_minor "$dev")
   local file
 
   if [ "$mode" = 'w' ]

CC'ing relevant maintainers, and someone who's worked with block scripts before...

Quoted 8 lines · Mike Latimer

Create the list of shared loopback devices from within check_sharing, rather than calling check_sharing for every loopback device using the same shared image.

This change prevents the xenstore database from being walked for every shared device, which causes an exponential decrease in performance.

Signed-off-by Mike Latimer <[email protected]>

Thanks for trying to address this -- this has obviously been a bit of a pain for a while. A couple of comments:

Quoted 28 lines

---
 tools/hotplug/Linux/block | 67 +++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 26 deletions(-)

tools/hotplug/Linux/block +6 -3

index 8d2ee9d..aef051c 100644
@@ -38,7 +38,7 @@ find_free_loopback_dev() {
 }
 
 ##
-# check_sharing device mode
+# check_sharing devtype device mode [inode]
 #
 # Check whether the device requested is already in use.  To use the device in
 # read-only mode, it may be in use in read-only mode, but may not be in use in

@@ -56,10 +56,27 @@ find_free_loopback_dev() {
 #
 check_sharing()
 {
-  local dev="$1"
-  local mode="$2"
+  local devtype=$1
+  local dev="$2"
+  local mode="$3"
+
+  if [ "$devtype" = "file" ];

tools/hotplug/Linux/block find_free_loopback_dev() { · line 73

Quoted 10 lines

tools/hotplug/Linux/block +10 -0

+  then
+    local inode="$4"
+
+    shared_list=$(losetup -a |
+          sed -n -e "s@^\([^:]\+\)\(:[[:blank:]]\[0*${dev}\]:${inode}[[:blank:]](.*)\)@\1@p" )
+    for dev in $shared_list
+    do
+      if [ -n "$dev" ]
+      then
+        devmm="${devmm}$(device_major_minor $dev),"

You forgot to declare devmm local. Since devmm is declared and used on both sides of the 'if', it seems like you should probably declare it local up top, rather than when it's first used.

Quoted 14 lines

tools/hotplug/Linux/block +5 -1

+      fi
+    done
+  else
+    local devmm="$(device_major_minor "$dev"),"
+  fi
 
-  local devmm=$(device_major_minor "$dev")
   local file
 
   if [ "$mode" = 'w' ]

@@ -75,9 +92,9 @@ check_sharing()
     then
       local d=$(device_major_minor "$file")
 

tools/hotplug/Linux/block check_sharing() · line 95

Quoted 2 lines

tools/hotplug/Linux/block +1 -1

-      if [ "$d" = "$devmm" ]
+      if [[ "$devmm" == *"$d,"* ]]

Style nit: using [[ instead of [. TBH I prefer [[, but it's probably better to be consistent with the rest of the file.

tools/hotplug/Linux/block check_sharing() · line 108

Quoted 9 lines

then

tools/hotplug/Linux/block +1 -1

-        echo 'local'
+        echo "local $d"
         return
       fi
     fi

@@ -90,13 +107,13 @@ check_sharing()
     do
       d=$(xenstore_read_default "$base_path/$dom/$dev/physical-device" 

tools/hotplug/Linux/block check_sharing() · line 110

Quoted 3 lines

tools/hotplug/Linux/block +1 -1

-      if [ "$d" = "$devmm" ]
+      if [ -n "$d" ] && [[ "$devmm" == *"$d,"* ]]

Is there a risk here of aliasing? i.e., suppose that a device with major-minor '11:1' that's already in use by a guest, and you're checking to see if you can share device '1:1' with a different guest. Won't this match, and (falsely) reject 1:1, even though it's not being used by anyone else?

Would it make more sense maybe to initialize devmm to ",", and then search for *",$d,"*?

tools/hotplug/Linux/block check_sharing() · line 95

Quoted 3 lines

tools/hotplug/Linux/block +1 -1

-      if [ "$d" = "$devmm" ]
+      if [[ "$devmm" == *"$d,"* ]]

Style nit: using [[ instead of [. TBH I prefer [[, but it's probably better to be consistent with the rest of the file.

Hi again,

Quoted 6 lines · George Dunlap

tools/hotplug/Linux/block check_sharing() · line 95

Quoted 3 lines

tools/hotplug/Linux/block +1 -1

-      if [ "$d" = "$devmm" ]
+      if [[ "$devmm" == *"$d,"* ]]

Style nit: using [[ instead of [. TBH I prefer [[, but it's probably better to be consistent with the rest of the file.

I was about to change this to something like:

  if [ "$devmm" != ${devmm/$d/} ]

but I'm a bit concerned about the potential size of the variables involved. If there are 500 devices with 5-7 characters per device, $devmm becomes a pretty large string. I'm not sure this bash substitution method is a great option. I also don't really want the hit of passing the variable to grep.

According to one post I found benchmarking various types of tests [1], the two fastest options are:

[[ $b == *$a* ]]

case $b in *$a):;;esac

I can implement a case statement, but that seems even less clean than the simple [[ ... ]] approach (since there is only one case we care about).

As this is a #!/bin/bash script, is [[ ... ]] okay to use, or would you prefer to use the case statement? (If you have any other ideas, I'm open to that as well.)