diff mbox series

[for-4.19] hotplug: Restore block-tap phy compatibility (again)

Message ID 20240715234631.4468-1-jandryuk@gmail.com (mailing list archive)
State New
Headers show
Series [for-4.19] hotplug: Restore block-tap phy compatibility (again) | expand

Commit Message

Jason Andryuk July 15, 2024, 11:46 p.m. UTC
From: Jason Andryuk <jason.andryuk@amd.com>

"$dev" needs to be set correctly for backendtype=phy as well as
backendtype=tap.  Move the setting into the conditional, so it can be
handled properly for each.

(dev could be captured during tap-ctl allocate for blktap module, but it
would not be set properly for the find_device case.  The backendtype=tap
case would need to be handled regardless.)

Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")
Fixes: 76a484193d ("hotplug: Update block-tap")

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
Tested with backendtype=tap & tapback and backendtype=phy & blktap
module.
---
 tools/hotplug/Linux/block-tap | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Jan Beulich July 16, 2024, 6:53 a.m. UTC | #1
On 16.07.2024 01:46, Jason Andryuk wrote:
> From: Jason Andryuk <jason.andryuk@amd.com>
> 
> "$dev" needs to be set correctly for backendtype=phy as well as
> backendtype=tap.  Move the setting into the conditional, so it can be
> handled properly for each.
> 
> (dev could be captured during tap-ctl allocate for blktap module, but it
> would not be set properly for the find_device case.  The backendtype=tap
> case would need to be handled regardless.)
> 
> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")
> Fixes: 76a484193d ("hotplug: Update block-tap")
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

Please don't forget to Cc Oleksii for anything you still want to go into
4.19 at this point.

Jan

> ---
> Tested with backendtype=tap & tapback and backendtype=phy & blktap
> module.
> ---
>  tools/hotplug/Linux/block-tap | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
> index 5165f459c8..a0b3924370 100755
> --- a/tools/hotplug/Linux/block-tap
> +++ b/tools/hotplug/Linux/block-tap
> @@ -204,13 +204,13 @@ add()
>          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"
>  
>      if [ "$XENBUS_TYPE" = "vbd3" ] ; then
> +        # Create nbd unix path.  find_device/tap_create set pid & minor
> +        dev=$( printf "/run/blktap-control/nbd%ld.%d" "$pid" "$minor" )
> +
>          # $dev, as a unix socket, has major:minor 0:0.  If write_dev writes
>          # physical-device, tapback would use that incorrect minor 0.  So don't
>          # write physical-device.
> @@ -218,6 +218,9 @@ add()
>  
>          success
>      else
> +        # Construct dev path from minor
> +        dev="/dev/xen/blktap-2/tapdev$minor"
> +        [ -b "$dev" ] || fatal "blktap \"$dev\" is not a block dev"
>          write_dev "$dev"
>      fi
>
Anthony PERARD July 23, 2024, 3:04 p.m. UTC | #2
On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
> "$dev" needs to be set correctly for backendtype=phy as well as
> backendtype=tap.  Move the setting into the conditional, so it can be
> handled properly for each.
> 
> (dev could be captured during tap-ctl allocate for blktap module, but it
> would not be set properly for the find_device case.  The backendtype=tap
> case would need to be handled regardless.)
> 
> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")

Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy compatibility") ?

> Fixes: 76a484193d ("hotplug: Update block-tap")
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

With the fixes tag fix:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

> ---
> Tested with backendtype=tap & tapback and backendtype=phy & blktap
> module.

Thanks for the extra testing.

Cheers,
Jason Andryuk July 23, 2024, 3:04 p.m. UTC | #3
On 2024-07-23 11:04, Anthony PERARD wrote:
> On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
>> "$dev" needs to be set correctly for backendtype=phy as well as
>> backendtype=tap.  Move the setting into the conditional, so it can be
>> handled properly for each.
>>
>> (dev could be captured during tap-ctl allocate for blktap module, but it
>> would not be set properly for the find_device case.  The backendtype=tap
>> case would need to be handled regardless.)
>>
>> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")
> 
> Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy compatibility") ?

Yes!  Thanks for checking that - I must have grabbed the hash from a 
local branch.

>> Fixes: 76a484193d ("hotplug: Update block-tap")
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> With the fixes tag fix:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks again.

Oleksii, this is a fix (for an incomplete fix) for 4.19.  76a484193d 
broke compatibility for block-tap with the blktap2 kernel model (when 
adding support for tapback).  This finishes restoring blktap2 support.

I realize it's late in the release if you don't want to take it.

Regards,
Jason
Oleksii Kurochko July 23, 2024, 5:31 p.m. UTC | #4
On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
> On 2024-07-23 11:04, Anthony PERARD wrote:
> > On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
> > > "$dev" needs to be set correctly for backendtype=phy as well as
> > > backendtype=tap.  Move the setting into the conditional, so it
> > > can be
> > > handled properly for each.
> > > 
> > > (dev could be captured during tap-ctl allocate for blktap module,
> > > but it
> > > would not be set properly for the find_device case.  The
> > > backendtype=tap
> > > case would need to be handled regardless.)
> > > 
> > > Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy
> > > compatibility")
> > 
> > Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy
> > compatibility") ?
> 
> Yes!  Thanks for checking that - I must have grabbed the hash from a 
> local branch.
> 
> > > Fixes: 76a484193d ("hotplug: Update block-tap")
> > > 
> > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> > 
> > With the fixes tag fix:
> > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
> 
> Thanks again.
> 
> Oleksii, this is a fix (for an incomplete fix) for 4.19.  76a484193d 
> broke compatibility for block-tap with the blktap2 kernel model (when
> adding support for tapback).  This finishes restoring blktap2
> support.
> 
> I realize it's late in the release if you don't want to take it.
It's pretty late but I just wanted to clarify:
1. Is so critical that we should have this in 4.19?
2. If we won't take it now, then will it be backported anyway?

~ Oleksii
Andrew Cooper July 23, 2024, 5:34 p.m. UTC | #5
On 23/07/2024 6:31 pm, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
>> On 2024-07-23 11:04, Anthony PERARD wrote:
>>> On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
>>>> "$dev" needs to be set correctly for backendtype=phy as well as
>>>> backendtype=tap.  Move the setting into the conditional, so it
>>>> can be
>>>> handled properly for each.
>>>>
>>>> (dev could be captured during tap-ctl allocate for blktap module,
>>>> but it
>>>> would not be set properly for the find_device case.  The
>>>> backendtype=tap
>>>> case would need to be handled regardless.)
>>>>
>>>> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy
>>>> compatibility")
>>> Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy
>>> compatibility") ?
>> Yes!  Thanks for checking that - I must have grabbed the hash from a 
>> local branch.
>>
>>>> Fixes: 76a484193d ("hotplug: Update block-tap")
>>>>
>>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>> With the fixes tag fix:
>>> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
>> Thanks again.
>>
>> Oleksii, this is a fix (for an incomplete fix) for 4.19.  76a484193d 
>> broke compatibility for block-tap with the blktap2 kernel model (when
>> adding support for tapback).  This finishes restoring blktap2
>> support.
>>
>> I realize it's late in the release if you don't want to take it.
> It's pretty late but I just wanted to clarify:
> 1. Is so critical that we should have this in 4.19?
> 2. If we won't take it now, then will it be backported anyway?

2) Yes it will get backported.  In fact I'm about to commit it to staging.

1) It's a bug in a new feature for 4.19, so if we don't take this bugfix
then we'll have to strip it from the release notes.

~Andrew
Jason Andryuk July 23, 2024, 6:05 p.m. UTC | #6
On 2024-07-23 13:34, Andrew Cooper wrote:
> On 23/07/2024 6:31 pm, oleksii.kurochko@gmail.com wrote:
>> On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
>>> On 2024-07-23 11:04, Anthony PERARD wrote:
>>>> On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
>>>>> "$dev" needs to be set correctly for backendtype=phy as well as
>>>>> backendtype=tap.  Move the setting into the conditional, so it
>>>>> can be
>>>>> handled properly for each.
>>>>>
>>>>> (dev could be captured during tap-ctl allocate for blktap module,
>>>>> but it
>>>>> would not be set properly for the find_device case.  The
>>>>> backendtype=tap
>>>>> case would need to be handled regardless.)
>>>>>
>>>>> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy
>>>>> compatibility")
>>>> Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy
>>>> compatibility") ?
>>> Yes!  Thanks for checking that - I must have grabbed the hash from a
>>> local branch.
>>>
>>>>> Fixes: 76a484193d ("hotplug: Update block-tap")
>>>>>
>>>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>>> With the fixes tag fix:
>>>> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
>>> Thanks again.
>>>
>>> Oleksii, this is a fix (for an incomplete fix) for 4.19.  76a484193d
>>> broke compatibility for block-tap with the blktap2 kernel model (when
>>> adding support for tapback).  This finishes restoring blktap2
>>> support.
>>>
>>> I realize it's late in the release if you don't want to take it.
>> It's pretty late but I just wanted to clarify:
>> 1. Is so critical that we should have this in 4.19?
>> 2. If we won't take it now, then will it be backported anyway?
> 
> 2) Yes it will get backported.  In fact I'm about to commit it to staging.
> 
> 1) It's a bug in a new feature for 4.19, so if we don't take this bugfix
> then we'll have to strip it from the release notes.

It's a bug in the old feature.  The new feature - tapback daemon 
support, backendtype=tap - works with what's in the 4.19 tree.  It's the 
old kernel module support - backendtype=phy,script=block-tap - that was 
broken when adding tapback support.  This patch fixes the old support.

The change is localized in the block-tap script and requires explicit 
configuration (script=block-tap) to use.  So it seems to me to be a 
lower risk to take it even though it is late in the cycle.

Regards,
Jason
Oleksii Kurochko July 24, 2024, 7:06 a.m. UTC | #7
On Tue, 2024-07-23 at 14:05 -0400, Jason Andryuk wrote:
> On 2024-07-23 13:34, Andrew Cooper wrote:
> > On 23/07/2024 6:31 pm, oleksii.kurochko@gmail.com wrote:
> > > On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
> > > > On 2024-07-23 11:04, Anthony PERARD wrote:
> > > > > On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk
> > > > > wrote:
> > > > > > "$dev" needs to be set correctly for backendtype=phy as
> > > > > > well as
> > > > > > backendtype=tap.  Move the setting into the conditional, so
> > > > > > it
> > > > > > can be
> > > > > > handled properly for each.
> > > > > > 
> > > > > > (dev could be captured during tap-ctl allocate for blktap
> > > > > > module,
> > > > > > but it
> > > > > > would not be set properly for the find_device case.  The
> > > > > > backendtype=tap
> > > > > > case would need to be handled regardless.)
> > > > > > 
> > > > > > Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy
> > > > > > compatibility")
> > > > > Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy
> > > > > compatibility") ?
> > > > Yes!  Thanks for checking that - I must have grabbed the hash
> > > > from a
> > > > local branch.
> > > > 
> > > > > > Fixes: 76a484193d ("hotplug: Update block-tap")
> > > > > > 
> > > > > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> > > > > With the fixes tag fix:
> > > > > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
> > > > Thanks again.
> > > > 
> > > > Oleksii, this is a fix (for an incomplete fix) for 4.19. 
> > > > 76a484193d
> > > > broke compatibility for block-tap with the blktap2 kernel model
> > > > (when
> > > > adding support for tapback).  This finishes restoring blktap2
> > > > support.
> > > > 
> > > > I realize it's late in the release if you don't want to take
> > > > it.
> > > It's pretty late but I just wanted to clarify:
> > > 1. Is so critical that we should have this in 4.19?
> > > 2. If we won't take it now, then will it be backported anyway?
> > 
> > 2) Yes it will get backported.  In fact I'm about to commit it to
> > staging.
> > 
> > 1) It's a bug in a new feature for 4.19, so if we don't take this
> > bugfix
> > then we'll have to strip it from the release notes.
> 
> It's a bug in the old feature.  The new feature - tapback daemon 
> support, backendtype=tap - works with what's in the 4.19 tree.  It's
> the 
> old kernel module support - backendtype=phy,script=block-tap - that
> was 
> broken when adding tapback support.  This patch fixes the old
> support.
> 
> The change is localized in the block-tap script and requires explicit
> configuration (script=block-tap) to use.  So it seems to me to be a 
> lower risk to take it even though it is late in the cycle.
Agree, if it is by default is disabled then I think we can have this
patch in 4.19:
 Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii
Anthony PERARD July 24, 2024, 11:50 a.m. UTC | #8
On Tue, Jul 23, 2024 at 07:31:58PM +0200, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
> > Oleksii, this is a fix (for an incomplete fix) for 4.19.  76a484193d 
> > broke compatibility for block-tap with the blktap2 kernel model (when
> > adding support for tapback).  This finishes restoring blktap2
> > support.
> > 
> > I realize it's late in the release if you don't want to take it.
> It's pretty late but I just wanted to clarify:
> 1. Is so critical that we should have this in 4.19?

I don't think it's critical enough to justify delaying the release.
blktap2 is I think an out-of-tree kernel module so probably not very
used.
We might want to at least put a note in "known issue" about blktap2
support by the "block-tap" script been broken.

> 2. If we won't take it now, then will it be backported anyway?

Yes, we want to backport that as soon as 4.19 tree is open for
backports, to have the fix in 4.19.1.

Cheers,
Jan Beulich July 24, 2024, 12:41 p.m. UTC | #9
On 23.07.2024 17:04, Anthony PERARD wrote:
> On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
>> "$dev" needs to be set correctly for backendtype=phy as well as
>> backendtype=tap.  Move the setting into the conditional, so it can be
>> handled properly for each.
>>
>> (dev could be captured during tap-ctl allocate for blktap module, but it
>> would not be set properly for the find_device case.  The backendtype=tap
>> case would need to be handled regardless.)
>>
>> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")
> 
> Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy compatibility") ?

Which, when replacing it, made noticeable that the above and ...

>> Fixes: 76a484193d ("hotplug: Update block-tap")

... this hash also were too short. Please make sure you use 12 digits, as
indicated by docs/process/sending-patches.pandoc.

Jan
Jason Andryuk July 24, 2024, 1:18 p.m. UTC | #10
On 2024-07-24 08:41, Jan Beulich wrote:
> On 23.07.2024 17:04, Anthony PERARD wrote:
>> On Mon, Jul 15, 2024 at 07:46:31PM -0400, Jason Andryuk wrote:
>>> "$dev" needs to be set correctly for backendtype=phy as well as
>>> backendtype=tap.  Move the setting into the conditional, so it can be
>>> handled properly for each.
>>>
>>> (dev could be captured during tap-ctl allocate for blktap module, but it
>>> would not be set properly for the find_device case.  The backendtype=tap
>>> case would need to be handled regardless.)
>>>
>>> Fixes: 6fcdc84927 ("hotplug: Restore block-tap phy compatibility")
>>
>> Do you mean f16ac12bd418 ("hotplug: Restore block-tap phy compatibility") ?
> 
> Which, when replacing it, made noticeable that the above and ...
> 
>>> Fixes: 76a484193d ("hotplug: Update block-tap")
> 
> ... this hash also were too short. Please make sure you use 12 digits, as
> indicated by docs/process/sending-patches.pandoc.

Sorry about that.  I'll make sure to use 12 in the future.  Thanks for 
catching and fixing up.

Regards,
Jason
Oleksii Kurochko July 24, 2024, 3:37 p.m. UTC | #11
On Wed, 2024-07-24 at 11:50 +0000, Anthony PERARD wrote:
> On Tue, Jul 23, 2024 at 07:31:58PM +0200,
> oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-07-23 at 11:04 -0400, Jason Andryuk wrote:
> > > Oleksii, this is a fix (for an incomplete fix) for 4.19. 
> > > 76a484193d 
> > > broke compatibility for block-tap with the blktap2 kernel model
> > > (when
> > > adding support for tapback).  This finishes restoring blktap2
> > > support.
> > > 
> > > I realize it's late in the release if you don't want to take it.
> > It's pretty late but I just wanted to clarify:
> > 1. Is so critical that we should have this in 4.19?
> 
> I don't think it's critical enough to justify delaying the release.
> blktap2 is I think an out-of-tree kernel module so probably not very
> used.
> We might want to at least put a note in "known issue" about blktap2
> support by the "block-tap" script been broken.
It was merged and I think it won't delay a release as this feature is
disabled by default.

~ Oleksii

> 
> > 2. If we won't take it now, then will it be backported anyway?
> 
> Yes, we want to backport that as soon as 4.19 tree is open for
> backports, to have the fix in 4.19.1.
> 
> Cheers,
>
diff mbox series

Patch

diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
index 5165f459c8..a0b3924370 100755
--- a/tools/hotplug/Linux/block-tap
+++ b/tools/hotplug/Linux/block-tap
@@ -204,13 +204,13 @@  add()
         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"
 
     if [ "$XENBUS_TYPE" = "vbd3" ] ; then
+        # Create nbd unix path.  find_device/tap_create set pid & minor
+        dev=$( printf "/run/blktap-control/nbd%ld.%d" "$pid" "$minor" )
+
         # $dev, as a unix socket, has major:minor 0:0.  If write_dev writes
         # physical-device, tapback would use that incorrect minor 0.  So don't
         # write physical-device.
@@ -218,6 +218,9 @@  add()
 
         success
     else
+        # Construct dev path from minor
+        dev="/dev/xen/blktap-2/tapdev$minor"
+        [ -b "$dev" ] || fatal "blktap \"$dev\" is not a block dev"
         write_dev "$dev"
     fi