Re: [Xen-devel] [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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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'

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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  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 

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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
 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  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 
>
> 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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [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 
---
 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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel