| 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 |
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 >
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,
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
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
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
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
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
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,
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
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
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 --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