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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Apr 22, 2024 at 11:11 AM Anthony PERARD
<anthony.perard@xxxxxxxxx> 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@xxxxxxxxxx>

Thanks,
Jason