Block script performance with shared image files
v2 cover Block script performance with shared image files
Hi,
Version 2 of this patch contains a couple of minor syntax changes, and a change to return immediately if the file being checked is not a shared image file.
This version also fixes the issue George pointed out, and the code will now properly differentiate between devices such as 1:1, 11:1 and 1:11.
Finally, I added a more complete description of the problem to the patch itself.
Thanks,
Mike
Mike Latimer (1):
tools/hotplug: Scan xenstore once when attaching shared images files
tools/hotplug/Linux/block | 76 +++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 26 deletions(-)
Signature · 1 line
1.8.4.5
PATCH v2 1/1 tools/hotplug: Scan xenstore once when attaching shared images files
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.
Prior to this patch, the entire xenstore database is walked for every instance of every loopback device pointing to the same shared image file. This process causes the block attachment process to becomes exponentially slower with every additional attachment of a shared image.
Rather than scanning all of xenstore for every instance of a shared loopback device, this patch creates a list of the major and minor numbers from all matching loopback devices. After generating this list, Xenstore is walked once, and major and minor numbers from every vbd are checked against the list. If a match is found, the mode of that vbd is checked for compatibility with the mode of the device being attached.
--- tools/hotplug/Linux/block | 76 +++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 26 deletions(-)
tools/hotplug/Linux/block +50 -26
@@ -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,34 @@ find_free_loopback_dev() { # check_sharing() { - local dev="$1" - local mode="$2" + local devtype=$1 + local dev="$2" + local mode="$3" + local devmm="," + + 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 + # if $devmm is unchanged, file being checked is not a shared loopback device + if [ "$devmm" = "," ]; + then + echo 'ok' + return + fi + else + devmm=${devmm}$(device_major_minor "$dev")"," + fi - local devmm=$(device_major_minor "$dev") local file if [ "$mode" = 'w' ]
@@ -75,9 +99,10 @@ check_sharing() then local d=$(device_major_minor "$file") - if [ "$d" = "$devmm" ] + # checking for $d in $devmm is best through the [[...]] bashism + if [[ "$devmm" == *",$d,"* ]] then - echo 'local' + echo "local $d" return fi fi
@@ -90,13 +115,14 @@ check_sharing() do d=$(xenstore_read_default "$base_path/$dom/$dev/physical-device" "") - if [ "$d" = "$devmm" ] + # checking for $d in $devmm is best through the [[...]] bashism + 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 +133,7 @@ check_sharing() then if ! same_vm $dom then - echo 'guest' + echo "guest $d" return fi fi
@@ -129,6 +155,7 @@ check_device_sharing() { local dev="$1" local mode=$(canonicalise_mode "$2") + local type="device" local result if [ "x$mode" = 'x!' ]
@@ -136,33 +163,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 +313,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)
Signature · 1 line
1.8.4.5
Re: [PATCH v2 1/1] tools/hotplug: Scan xenstore once when attaching shared images files
Quoted 12 lines · Mike Latimer
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.
Prior to this patch, the entire xenstore database is walked for every instance of every loopback device pointing to the same shared image file. This process causes the block attachment process to becomes exponentially slower with every additional attachment of a shared image.
Quoted 8 lines
Rather than scanning all of xenstore for every instance of a shared loopback device, this patch creates a list of the major and minor numbers from all matching loopback devices. After generating this list, Xenstore is walked once, and major and minor numbers from every vbd are checked against the list. If a match is found, the mode of that vbd is checked for compatibility with the mode of the device being attached.
Signed-off-by Mike Latimer <[email protected]>
This looks good to me apart from one thing which is that these:
[...]
Quoted 4 lines
then
tools/hotplug/Linux/block +1
-1- echo 'local' + echo "local $d"
[...]
Quoted 3 lines
then
(quoted diff) +1
-1- echo 'guest' + echo "guest $d"
[...]
Review
Quoted 3 lines
then
(quoted diff) +1
-1- echo 'guest' + echo "guest $d"
Are invalidating this comment on the check_sharing function regarding what it returns:
# Prints one of
#
# 'local': the device may not be used because it is mounted in the current
# (i.e. the privileged domain) in a way incompatible with the
# requested mode;
# 'guest': the device may not be used because it already mounted by a guest
# in a way incompatible with the requested mode; or
# 'ok': the device may be used.
Ian.
Re: [PATCH v2 1/1] tools/hotplug: Scan xenstore once when attaching shared images files
In reply to Mike LatimerMike Latimer writes ("[PATCH v2 1/1] tools/hotplug: Scan xenstore once when attaching shared images files"):
Quoted 6 lines
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.
Thanks for the patch. Subject to Ian's observations about the doc comment it looks good to me.
Ian.