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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

>        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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel