| Message ID | 20220615101920.329421-13-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | support non power of 2 zoned device | expand |
On 6/15/22 19:19, Pankaj Raghav wrote: > dm_zone_endio() updates the bi_sector of orig bio for zoned devices that > uses either native append or append emulation and it is called before the > endio of the target. But target endio can still update the clone bio > after dm_zone_endio is called, thereby, the orig bio does not contain > the updated information anymore. Call dm_zone_endio for zoned devices > after calling the target's endio function > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > @Damien and @Hannes: I couldn't come up with a testcase that uses endio callback and > zone append or append emulation for zoned devices to test for > regression in this change. It would be great if you can suggest > something. This change is required for the npo2 target as we update the > clone bio sector in the endio callback function and the orig bio should > be updated only after the endio callback for zone appends. Running zonefs tests on top of dm-crypt will exercise DM zone append emulation. > > drivers/md/dm.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 3f17fe1de..3a74e1038 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -1025,10 +1025,6 @@ static void clone_endio(struct bio *bio) > disable_write_zeroes(md); > } > > - if (static_branch_unlikely(&zoned_enabled) && > - unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) > - dm_zone_endio(io, bio); > - > if (endio) { > int r = endio(ti, bio, &error); > switch (r) { > @@ -1057,6 +1053,10 @@ static void clone_endio(struct bio *bio) > } > } > > + if (static_branch_unlikely(&zoned_enabled) && > + unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev))) -> bdev_is_zoned(bio->bi_bdev) > + dm_zone_endio(io, bio); > + > if (static_branch_unlikely(&swap_bios_enabled) && > unlikely(swap_bios_limit(ti, bio))) > up(&md->swap_bios_semaphore);
On 2022-06-15 13:01, Damien Le Moal wrote: > On 6/15/22 19:19, Pankaj Raghav wrote: >> dm_zone_endio() updates the bi_sector of orig bio for zoned devices that >> uses either native append or append emulation and it is called before the >> endio of the target. But target endio can still update the clone bio >> after dm_zone_endio is called, thereby, the orig bio does not contain >> the updated information anymore. Call dm_zone_endio for zoned devices >> after calling the target's endio function >> >> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> >> --- >> @Damien and @Hannes: I couldn't come up with a testcase that uses endio callback and >> zone append or append emulation for zoned devices to test for >> regression in this change. It would be great if you can suggest >> something. This change is required for the npo2 target as we update the >> clone bio sector in the endio callback function and the orig bio should >> be updated only after the endio callback for zone appends. > > Running zonefs tests on top of dm-crypt will exercise DM zone append > emulation. > Thanks. However, I am facing issues creating a dm-crypt target with a zoned device. Steps: - cryptsetup luksFormat <zns-device> is throwing a bunch of IO errors with the following error message: Device wipe error, offset 32768. Cannot wipe header on device <zns-device>. I can observe the same behavior in both v5.18 and next-20220615 with cryptsetup 2.4.3.The same step is working correctly on a normal NVMe device. Am I doing something wrong? ZNS info: zsze 128M and zcap 128M with 50 zones >> >> drivers/md/dm.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/md/dm.c b/drivers/md/dm.c >> index 3f17fe1de..3a74e1038 100644 >> --- a/drivers/md/dm.c >> +++ b/drivers/md/dm.c >> @@ -1025,10 +1025,6 @@ static void clone_endio(struct bio *bio) >> disable_write_zeroes(md); >> } >> >> - if (static_branch_unlikely(&zoned_enabled) && >> - unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) >> - dm_zone_endio(io, bio); >> - >> if (endio) { >> int r = endio(ti, bio, &error); >> switch (r) { >> @@ -1057,6 +1053,10 @@ static void clone_endio(struct bio *bio) >> } >> } >> >> + if (static_branch_unlikely(&zoned_enabled) && >> + unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) > > blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev))) -> > bdev_is_zoned(bio->bi_bdev) > Ok. Even though I just moved the statements, I think this is trivial enough to take it along. >> + dm_zone_endio(io, bio); >> + >> if (static_branch_unlikely(&swap_bios_enabled) && >> unlikely(swap_bios_limit(ti, bio))) >> up(&md->swap_bios_semaphore); > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 6/16/22 21:24, Pankaj Raghav wrote: > On 2022-06-15 13:01, Damien Le Moal wrote: >> On 6/15/22 19:19, Pankaj Raghav wrote: >>> dm_zone_endio() updates the bi_sector of orig bio for zoned devices that >>> uses either native append or append emulation and it is called before the >>> endio of the target. But target endio can still update the clone bio >>> after dm_zone_endio is called, thereby, the orig bio does not contain >>> the updated information anymore. Call dm_zone_endio for zoned devices >>> after calling the target's endio function >>> >>> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> >>> --- >>> @Damien and @Hannes: I couldn't come up with a testcase that uses endio callback and >>> zone append or append emulation for zoned devices to test for >>> regression in this change. It would be great if you can suggest >>> something. This change is required for the npo2 target as we update the >>> clone bio sector in the endio callback function and the orig bio should >>> be updated only after the endio callback for zone appends. >> >> Running zonefs tests on top of dm-crypt will exercise DM zone append >> emulation. >> > Thanks. However, I am facing issues creating a dm-crypt target with a > zoned device. Steps: > - cryptsetup luksFormat <zns-device> luks format is not supported because cryptsetup does not write the metadata sequentially. I am working on fixing that. Use the plain format. > > is throwing a bunch of IO errors with the following error message: > Device wipe error, offset 32768. > Cannot wipe header on device <zns-device>. > > I can observe the same behavior in both v5.18 and next-20220615 with > cryptsetup 2.4.3.The same step is working correctly on a normal NVMe device. > Am I doing something wrong? > ZNS info: zsze 128M and zcap 128M with 50 zones >>> >>> drivers/md/dm.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c >>> index 3f17fe1de..3a74e1038 100644 >>> --- a/drivers/md/dm.c >>> +++ b/drivers/md/dm.c >>> @@ -1025,10 +1025,6 @@ static void clone_endio(struct bio *bio) >>> disable_write_zeroes(md); >>> } >>> >>> - if (static_branch_unlikely(&zoned_enabled) && >>> - unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) >>> - dm_zone_endio(io, bio); >>> - >>> if (endio) { >>> int r = endio(ti, bio, &error); >>> switch (r) { >>> @@ -1057,6 +1053,10 @@ static void clone_endio(struct bio *bio) >>> } >>> } >>> >>> + if (static_branch_unlikely(&zoned_enabled) && >>> + unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) >> >> blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev))) -> >> bdev_is_zoned(bio->bi_bdev) >> > Ok. Even though I just moved the statements, I think this is trivial > enough to take it along. >>> + dm_zone_endio(io, bio); >>> + >>> if (static_branch_unlikely(&swap_bios_enabled) && >>> unlikely(swap_bios_limit(ti, bio))) >>> up(&md->swap_bios_semaphore); >> >>
diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 3f17fe1de..3a74e1038 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1025,10 +1025,6 @@ static void clone_endio(struct bio *bio) disable_write_zeroes(md); } - if (static_branch_unlikely(&zoned_enabled) && - unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) - dm_zone_endio(io, bio); - if (endio) { int r = endio(ti, bio, &error); switch (r) { @@ -1057,6 +1053,10 @@ static void clone_endio(struct bio *bio) } } + if (static_branch_unlikely(&zoned_enabled) && + unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) + dm_zone_endio(io, bio); + if (static_branch_unlikely(&swap_bios_enabled) && unlikely(swap_bios_limit(ti, bio))) up(&md->swap_bios_semaphore);
dm_zone_endio() updates the bi_sector of orig bio for zoned devices that uses either native append or append emulation and it is called before the endio of the target. But target endio can still update the clone bio after dm_zone_endio is called, thereby, the orig bio does not contain the updated information anymore. Call dm_zone_endio for zoned devices after calling the target's endio function Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> --- @Damien and @Hannes: I couldn't come up with a testcase that uses endio callback and zone append or append emulation for zoned devices to test for regression in this change. It would be great if you can suggest something. This change is required for the npo2 target as we update the clone bio sector in the endio callback function and the orig bio should be updated only after the endio callback for zone appends. drivers/md/dm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)