Re: [Xen-devel] Shared image files and block script performance

[Mike Latimer <mlatimer@xxxxxxxx>]

Hi Ian,

On Tuesday, September 29, 2015 10:25:32 AM Ian Campbell wrote:
> On Mon, 2015-09-28 at 17:14 -0600, Mike Latimer wrote:
> > Any better options or ideas?
> 
> Is part of the problem that shell is a terrible choice for this kind of
> check?

There is some truth to that...  ;)

> Would shelling out to a helper utility allow this to be written in
> something better?

A helper utility would be useful, however, I'm seeing a huge amount of gain 
with nothing more than a little code motion. Specifically, if shared_list is 
generated within the check_sharing function, the (potentially) large list of 
devices is not too painful to work with.

For example, the attached patch works well in my environment, and removes the 
exponential slowdown. The main change is that $devmm becomes a comma delimited 
list of devices (major:minor) to check against the vbd's found in xenstore. A 
few minor changes are required along the way, but nothing significant. The 
comma delimited list might become problematic at very large numbers (hundreds) 
of a single shared device, but I don't think it will be a problem in practice. 
Even if it has limitations, this approach offers significant improvements in 
performance.

I'll continue to test this patch here, but I'm interested in your opinion.

Thanks,
Mike

---

--- a/etc/xen/scripts/block     2015-09-26 16:52:19.000000000 -0600
+++ b/etc/xen/scripts/block     2015-09-29 14:48:43.000000000 -0600
@@ -66,10 +66,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' ]
@@ -85,9 +102,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
@@ -100,13 +117,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
@@ -117,7 +134,7 @@ check_sharing()
           then
             if ! same_vm $dom
             then
-              echo 'guest'
+              echo 'guest' "$d"
               return
             fi
           fi
@@ -146,11 +163,11 @@ check_device_sharing()
     return 0
   fi
 
-  result=$(check_sharing "$dev" "$mode")
+  result=$(check_sharing "device" "$dev" "$mode")
 
-  if [ "$result" != 'ok' ]
+  if [[ "$result" != "ok"* ]]
   then
-    do_ebusy "Device $dev is mounted " "$mode" "$result"
+    do_ebusy "Device $dev is mounted " "$mode" "${result%% *}"
   fi
 }
 
@@ -166,13 +183,14 @@ check_file_sharing()
   local file="$1"
   local dev="$2"
   local mode="$3"
+  local inode="$4"
 
-  result=$(check_sharing "$dev" "$mode")
+  result=$(check_sharing "file" "$dev" "$mode" "$inode")
 
-  if [ "$result" != 'ok' ]
+  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 ${dev#* },
+which is mounted " "$mode" "${result%% *}"
   fi
 }
 
@@ -289,15 +307,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)

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