[PATCH 0/1] Block script performance with shared image files

[PATCH 0/1] Block script performance with shared image files

[Mike Latimer]

As mentioned in a previous thread[1], the hotplug block script suffers
from an exponential performance degredation when attaching shared image
files due to scanning xenstore multiple times.

During the attachment of a loopback mounted image file, the mode of all
curent instances of this device already attached to other domains must be
checked. This requires finding all loopback devices pointing to the inode
of the shared image file, and then comparing the major and minor number of
these devices to the major and minor number of every vbd device found in the
xenstore database. Scanning all of xenstore is performed for every instance
of every loopback device pointing to the same shared image file, and becomes
slower and slower with every additional instance.

Rather than scanning the entire xenstore database for every instance of a
shared loopback device, the attached patch creates a list of the major and
minor numbers from all matching loopback devices. Xenstore is then walked
once, and major and minor numbers from every vbd are checked against the list.

I think this is a reasonable approach to this issue, and the benefits can be
seen with as few as 10 domains sharing the same image file.

Thanks,
Mike

[1]http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg03551.html

Mike Latimer (1):
  tools/hotplug: Scan xenstore once when attaching shared images files

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

-- 
1.8.4.5

[PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[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 <mlatimer@suse.com>
---
 tools/hotplug/Linux/block | 67 +++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
index 8d2ee9d..aef051c 100644
--- a/tools/hotplug/Linux/block
+++ b/tools/hotplug/Linux/block
@@ -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" ];
+  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),"
+      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")
 
-      if [ "$d" = "$devmm" ]
+      if [[ "$devmm" == *"$d,"* ]]
       then
-        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" "")
 
-      if [ "$d" = "$devmm" ]
+      if [ -n "$d" ] && [[ "$devmm" == *"$d,"* ]]
       then
         if [ "$mode" = 'w' ]
         then
           if ! same_vm $dom
           then
-            echo 'guest'
+            echo "guest $d"
             return
           fi
         else
@@ -107,7 +124,7 @@ check_sharing()
           then
             if ! same_vm $dom
             then
-              echo 'guest'
+              echo "guest $d"
               return
             fi
           fi
@@ -129,6 +146,7 @@ check_device_sharing()
 {
   local dev="$1"
   local mode=$(canonicalise_mode "$2")
+  local type="device"
   local result
 
   if [ "x$mode" = 'x!' ]
@@ -136,33 +154,38 @@ check_device_sharing()
     return 0
   fi
 
-  result=$(check_sharing "$dev" "$mode")
+  result=$(check_sharing "$type" "$dev" "$mode")
 
   if [ "$result" != 'ok' ]
   then
-    do_ebusy "Device $dev is mounted " "$mode" "$result"
+    do_ebusy "Device $dev is mounted " "$mode" "${result%% *}"
   fi
 }
 
 
 ##
-# check_device_sharing file dev mode
+# check_device_sharing file dev mode inode
 #
-# Perform the sharing check for the given file mounted through the given
-# loopback interface, in the given mode.
+# Perform the sharing check for the given file, with its corresponding
+# device, inode and mode. As the file can be mounted multiple times,
+# the inode is passed through to check_sharing for all instances to be
+# checked.
 #
 check_file_sharing()
 {
   local file="$1"
   local dev="$2"
   local mode="$3"
+  local inode="$4"
+  local type="file"
+  local result
 
-  result=$(check_sharing "$dev" "$mode")
+  result=$(check_sharing "$type" "$dev" "$mode" "$inode")
 
   if [ "$result" != 'ok' ]
   then
-    do_ebusy "File $file is loopback-mounted through $dev,
-which is mounted " "$mode" "$result"
+    do_ebusy "File $file is loopback-mounted through ${result#* },
+which is mounted " "$mode" "${result%% *}"
   fi
 }
 
@@ -281,15 +304,7 @@ mount it read-write in a guest domain."
             fatal "Unable to lookup $file: dev: $dev inode: $inode"
           fi
 
-          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
-              check_file_sharing "$file" "$dev" "$mode"
-            fi
-          done
+          check_file_sharing "$file" "$dev" "$mode" "$inode"
         fi
 
         loopdev=$(losetup -f 2>/dev/null || find_free_loopback_dev)
-- 
1.8.4.5

Re: [PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[George Dunlap]

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

On Wed, Sep 30, 2015 at 10:10 PM, Mike Latimer <mlatimer@suse.com> wrote:
> 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 <mlatimer@suse.com>

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

> ---
>  tools/hotplug/Linux/block | 67 +++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
> index 8d2ee9d..aef051c 100644
> --- a/tools/hotplug/Linux/block
> +++ b/tools/hotplug/Linux/block
> @@ -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" ];
> +  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.

> +      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")
>
> -      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.

>        then
> -        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" "")
>
> -      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,"*?

Also, [[ again.

Other than that,  seems good to me.

 -George

Re: [PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[George Dunlap]

On Thu, Oct 1, 2015 at 10:51 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> CC'ing relevant maintainers, and someone who's worked with block
> scripts before...
>
> On Wed, Sep 30, 2015 at 10:10 PM, Mike Latimer <mlatimer@suse.com> wrote:
>> 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 <mlatimer@suse.com>
>
> Thanks for trying to address this -- this has obviously been a bit of
> a pain for a while.  A couple of comments:
>
>> ---
>>  tools/hotplug/Linux/block | 67 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 41 insertions(+), 26 deletions(-)
>>
>> diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
>> index 8d2ee9d..aef051c 100644
>> --- a/tools/hotplug/Linux/block
>> +++ b/tools/hotplug/Linux/block
>> @@ -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" ];
>> +  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.
>
>> +      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")
>>
>> -      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.

Obviously the check here should be *",$d,"* as well.

BTW my experiments bear out the aliasing issue, and my proposed solution:

$ a="11:1,12:1,13:1"
$ if [[ "$a" == *"1:1,"* ]] ; then echo match ; fi
match
$ a=",11:1,12:1,13:1"
$ if [[ "$a" == *",1:1,"* ]] ; then echo match ; fi
$ if [[ "$a" == *",11:1,"* ]] ; then echo match ; fi
match

 -George

Re: [PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[Mike Latimer]

Hi George,

On Thursday, October 01, 2015 10:51:08 AM George Dunlap wrote:
> >        then
> > -        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" 
"")
> >
> > -      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,"*?

Ah, thanks for catching that! I caught the "1:11" case, but somehow missed the 
"11:1" side.

I'll address that, and your other comments and submit a V2 shortly.

-Mike

Re: [PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[Mike Latimer]

Hi again,

On Thursday, October 01, 2015 10:51:08 AM George Dunlap wrote:
> >
> > -      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.)

Thanks!
Mike

[1]http://stackoverflow.com/questions/229551/string-contains-in-bash  (search 
for 'fastest'

Re: [PATCH 1/1] tools/hotplug: Scan xenstore once when attaching shared images files

[George Dunlap]

On 01/10/15 16:16, Mike Latimer wrote:
> Hi again,
> 
> On Thursday, October 01, 2015 10:51:08 AM George Dunlap wrote:
>>>
>>> -      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.)

Oh, right -- sorry, I didn't realize that the test you were using was
only available with [[ ]].

The maintainers (of which I'm not one) have the final say.  I'd
personally be in favor of leaving it [[ ]] with a comment before each
one saying that the $b == *$a* format is only available in [[ ]], so
that no one comes later and "cleans it up".

I'd personally be even more happy if all the [ ] in the script were
magically changed to [[ ]]. :-)

 -George