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 <[email protected]>
---
@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(-)
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);
--
2.25.1
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 <[email protected]>
> ---
> @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);
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
>> ---
>> @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);
>
>
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 <[email protected]>
>>> ---
>>> @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);
>>
>>
--
Damien Le Moal
Western Digital Research