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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Oct 1, 2015 at 10:51 AM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx>
>
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel