diff mbox series

[v2,1/3] hotplug: Update block-tap

Message ID 20240407204953.60442-2-jandryuk@gmail.com (mailing list archive)
State New, archived
Headers show
Series libxl: blktap/tapback support | expand

Commit Message

Jason Andryuk April 7, 2024, 8:49 p.m. UTC
Implement a sharing check like the regular block script.

Checking tapback inside block-tap is too late since it needs to be
running to transition the backend to InitWait before block-tap is run.

tap-ctl check will be removed when the requirement for the blktap kernel
driver is removed.  Remove it now as it is of limited use.

find_device() needs to be non-fatal allow a sharing check.

Only write physical-device-path because that is all that tapback needs.
Also write_dev doesn't handled files and would incorrectly store
physical-device as 0:0 which would confuse the minor inside tapback

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
v2:

Update usage string
Use local more
Don't use local for pid/minor
Use tap-ctl destroy
Use 4 space indent in count_using and check_tap_sharing
---
 tools/hotplug/Linux/block-tap | 169 +++++++++++++++++++++++++++++++---
 1 file changed, 157 insertions(+), 12 deletions(-)

Comments

Anthony PERARD April 22, 2024, 3:11 p.m. UTC | #1
On Sun, Apr 07, 2024 at 04:49:51PM -0400, Jason Andryuk wrote:
> diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
> index 89247921b9..126e472786 100755
> --- a/tools/hotplug/Linux/block-tap
> +++ b/tools/hotplug/Linux/block-tap
> @@ -18,11 +18,11 @@
>  #
>  # Usage:
>  #
> -# Target should be specified using the following syntax:
> +# Disks should be specified using the following syntax:
>  #
> -# script=block-tap,vdev=xvda,target=<type>:<file>
> +# vdev=xvda,backendtype=tap,format=vhd,target=/srv/target.vhd

I still have unanswered question from the previous round:
    Is `block-tap` still going to work with the current example given in
    the script header? That is:
        "script=block-tap,vdev=xvda,target=<type>:<file>"
    Or maybe, that example is already broken?

If it's not broken, there could be users which rely on it. But maybe
it's not really broken, and the new syntax is better anyway.

My guess is that using "script=block-tap,..." might still work, but
we should say something in the CHANGELOG to encourage people to move to
the new syntax, with "backendtype=tap" to avoid issues.


In any case, the patch looks good:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Jason Andryuk April 23, 2024, 2:21 a.m. UTC | #2
On Mon, Apr 22, 2024 at 11:11 AM Anthony PERARD
<anthony.perard@cloud.com> wrote:
>
> On Sun, Apr 07, 2024 at 04:49:51PM -0400, Jason Andryuk wrote:
> > diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
> > index 89247921b9..126e472786 100755
> > --- a/tools/hotplug/Linux/block-tap
> > +++ b/tools/hotplug/Linux/block-tap
> > @@ -18,11 +18,11 @@
> >  #
> >  # Usage:
> >  #
> > -# Target should be specified using the following syntax:
> > +# Disks should be specified using the following syntax:
> >  #
> > -# script=block-tap,vdev=xvda,target=<type>:<file>
> > +# vdev=xvda,backendtype=tap,format=vhd,target=/srv/target.vhd
>
> I still have unanswered question from the previous round:
>     Is `block-tap` still going to work with the current example given in
>     the script header? That is:
>         "script=block-tap,vdev=xvda,target=<type>:<file>"
>     Or maybe, that example is already broken?

Oh, right.  Sorry about that.

> If it's not broken, there could be users which rely on it. But maybe
> it's not really broken, and the new syntax is better anyway.
>
> My guess is that using "script=block-tap,..." might still work, but
> we should say something in the CHANGELOG to encourage people to move to
> the new syntax, with "backendtype=tap" to avoid issues.

I think the old syntax with "backendtype=phy" would work except for this:
-    write_dev $dev
...
+    # dev, as a unix socket, would end up with major:minor 0:0 in
+    # physical-device if write_dev were used.  tapback would be thrown off by
+    # that incorrect minor, so just skip writing physical-device.
+    xenstore_write "$XENBUS_PATH/physical-device-path" "$dev"

write_dev is needed for blkback to see physical-device and set things
up properly.

I could create a second script, but that's a little silly for just the
single line.  Another approach would be to differentiate off the
device type, vbd vs. vbd3, and use write_dev or not that way.  Should
I just do that?

block-tap will only support backendtype=phy as long as blktap uses the
kernel driver.  In that case tap-ctl create is returning the kernel
module's block dev path.  Once the kernel drive support is removed,
backendtype=tap will be the only option - without it there is no block
dev for blkback.

FYI, "backendtype=tap" is more performant because it is a shorter data path:
blkfront -> tapback/tapdisk
vs
blkfront -> blkback -> /dev/xen/blktap-2/tapdevN -> tapdisk

There are extra copies between blktap and the tapdev.

> In any case, the patch looks good:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Jason
diff mbox series

Patch

diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
index 89247921b9..126e472786 100755
--- a/tools/hotplug/Linux/block-tap
+++ b/tools/hotplug/Linux/block-tap
@@ -18,11 +18,11 @@ 
 #
 # Usage:
 #
-# Target should be specified using the following syntax:
+# Disks should be specified using the following syntax:
 #
-# script=block-tap,vdev=xvda,target=<type>:<file>
+# vdev=xvda,backendtype=tap,format=vhd,target=/srv/target.vhd
 #
-# Type is either "aio" (for raw files), or "vhd"
+# format is either "aio" (for raw files), or "vhd"
 
 dir=$(dirname "$0")
 . "$dir/block-common.sh"
@@ -37,10 +37,6 @@  check_tools()
     if ! command -v tap-ctl > /dev/null 2>&1; then
         fatal "Unable to find tap-ctl tool"
     fi
-    modprobe blktap
-    if ! tap-ctl check >& /dev/null ; then
-	fatal "Blocktap kernel module not available"
-    fi
 }
 
 # Sets the following global variables based on the params field passed in as
@@ -81,7 +77,109 @@  find_device()
     done
 
     if [ -z "$pid" ] || [ -z "$minor" ]; then
-        fatal "cannot find required parameters"
+        return 1
+    fi
+
+    return 0
+}
+
+count_using()
+{
+    local file="$1"
+    local dom
+    local dev
+    local f
+
+    local i=0
+    local base_path="$XENBUS_BASE_PATH/$XENBUS_TYPE"
+    for dom in $(xenstore-list "$base_path")
+    do
+        for dev in $(xenstore-list "$base_path/$dom")
+        do
+            f=$(xenstore_read_default "$base_path/$dom/$dev/params" "")
+            f=$(echo "$f" | cut -d ":" -f 2)
+
+            if [ -n "$f" ] && [ "$file" = $f ] ; then
+                i=$(( i + 1 ))
+            fi
+        done
+    done
+
+    echo "$i"
+}
+
+# tap_shared is used to determine if a shared tap can be closed
+# Since a stubdom and a guest both use the same tap, it can only
+# be freed when there is a single one left.
+tap_shared() {
+    [ $( count_using "$file" ) -gt 1 ]
+}
+
+check_tap_sharing()
+{
+    local file="$1"
+    local mode="$2"
+    local dom
+    local dev
+
+    local base_path="$XENBUS_BASE_PATH/$XENBUS_TYPE"
+    for dom in $(xenstore-list "$base_path") ; do
+        for dev in $(xenstore-list "$base_path/$dom") ; do
+            local f=$(xenstore_read_default "$base_path/$dom/$dev/params" "")
+            f=$(echo "$f" | cut -d ":" -f 2)
+
+            if [ -n "$f" ] && [ "$file" = "$f" ] ; then
+                if [ "$mode" = 'w' ] ; then
+                    if ! same_vm $dom ; then
+                        echo "guest $f"
+                        return
+                    fi
+                else
+                    local m=$(xenstore_read_default "$base_path/$dom/$dev/mode"
+                                                    "")
+                    m=$(canonicalise_mode "$m")
+
+                    if [ "$m" = 'w' ] ; then
+                        if ! same_vm $dom ; then
+                            echo "guest $f"
+                            return
+                        fi
+                    fi
+                fi
+            fi
+        done
+    done
+
+    echo 'ok'
+}
+
+tap_create()
+{
+    if ! minor=$( tap-ctl allocate ) ; then
+        fatal "Could not allocate minor"
+    fi
+
+    # Handle with or without kernel blktap
+    minor=${minor#/run/blktap-control/tapdisk/tapdisk-}
+    minor=${minor#/dev/xen/blktap-2/tapdev}
+
+    # tap-ctl is spawning tapdisk which would hold the _lockfd open.
+    # Ensure it is closed before running tap-ctl spawn, which needs to be
+    # done in a subshell to continue holding the lock in the parent.
+    if ! pid=$( ( eval "exec $_lockfd>&-" ; tap-ctl spawn ) ) ; then
+        tap-ctl free -m "$minor"
+        fatal "Could not spawn tapdisk for $minor"
+    fi
+
+    if ! tap-ctl attach -p "$pid" -m "$minor" ; then
+        tap-ctl free -m "$minor"
+        fatal "Could not attach $pid and $minor"
+    fi
+
+    if ! tap-ctl open -p "$pid" -m "$minor" -a "$target" ; then
+        tap-ctl detach -p "$pid" -m "$minor"
+        tap-ctl free -m "$minor"
+        fatal "Could not open \"$target\""
     fi
 }
 
@@ -89,15 +187,54 @@  find_device()
 # the device
 add()
 {
-    dev=$(tap-ctl create -a $target)
-    write_dev $dev
+    local result
+
+    claim_lock "block"
+
+    if find_device; then
+        result=$( check_tap_sharing "$file" "$mode" )
+        if [ "$result" != "ok" ] ; then
+            do_ebusy "tap $type file $file in use " "$mode" "${result%% *}"
+        fi
+    else
+        tap_create
+    fi
+
+    # Create nbd unix path.  find_device/tap_create set pid & minor
+    dev=$( printf "/run/blktap-control/nbd%ld.%d" "$pid" "$minor" )
+
+    xenstore_write "$XENBUS_PATH/pid" "$pid"
+    xenstore_write "$XENBUS_PATH/minor" "$minor"
+    # dev, as a unix socket, would end up with major:minor 0:0 in
+    # physical-device if write_dev were used.  tapback would be thrown off by
+    # that incorrect minor, so just skip writing physical-device.
+    xenstore_write "$XENBUS_PATH/physical-device-path" "$dev"
+
+    success
+
+    release_lock "block"
 }
 
 # Disconnects the device
 remove()
 {
-    find_device
-    do_or_die tap-ctl destroy -p ${pid} -m ${minor} > /dev/null
+    local minor
+    local pid
+
+    claim_lock "block"
+
+    if tap_shared ; then
+        return
+    fi
+
+    minor=$( xenstore_read "$XENBUS_PATH/minor" )
+    pid=$( xenstore_read "$XENBUS_PATH/pid" )
+
+    [ -n "$minor" ] || fatal "minor missing"
+    [ -n "$pid" ] || fatal "pid missing"
+    do_or_die tap-ctl destroy -p "$pid" -m "$minor" > /dev/null
+
+    release_lock "block"
 }
 
 command=$1
@@ -110,6 +247,14 @@  parse_target "$target"
 
 check_tools || exit 1
 
+mode=$( xenstore_read $XENBUS_PATH/mode )
+mode=$( canonicalise_mode $mode )
+
+# needed for same_vm
+FRONTEND_ID=$(xenstore_read "$XENBUS_PATH/frontend-id")
+FRONTEND_UUID=$(xenstore_read_default \
+                    "/local/domain/$FRONTEND_ID/vm" 'unknown')
+
 case $command in
 add)
     add