2022-07-27 17:54:20

by Pankaj Raghav

[permalink] [raw]
Subject: [PATCH v8 10/11] dm: call dm_zone_endio after the target endio callback for zoned devices

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]>
---
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 03ac6143b8aa..bc410ee04004 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1123,10 +1123,6 @@ static void clone_endio(struct bio *bio)
disable_write_zeroes(md);
}

- if (static_branch_unlikely(&zoned_enabled) &&
- unlikely(bdev_is_zoned(bio->bi_bdev)))
- dm_zone_endio(io, bio);
-
if (endio) {
int r = endio(ti, bio, &error);
switch (r) {
@@ -1155,6 +1151,10 @@ static void clone_endio(struct bio *bio)
}
}

+ if (static_branch_unlikely(&zoned_enabled) &&
+ unlikely(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);
--
2.25.1


2022-07-28 03:30:36

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH v8 10/11] dm: call dm_zone_endio after the target endio callback for zoned devices

On 7/28/22 01:22, 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]>
> ---
> 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 03ac6143b8aa..bc410ee04004 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1123,10 +1123,6 @@ static void clone_endio(struct bio *bio)
> disable_write_zeroes(md);
> }
>
> - if (static_branch_unlikely(&zoned_enabled) &&
> - unlikely(bdev_is_zoned(bio->bi_bdev)))
> - dm_zone_endio(io, bio);
> -
> if (endio) {
> int r = endio(ti, bio, &error);
> switch (r) {
> @@ -1155,6 +1151,10 @@ static void clone_endio(struct bio *bio)
> }
> }
>
> + if (static_branch_unlikely(&zoned_enabled) &&
> + unlikely(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);

This patch seems completely unrelated to the series topic. Is that a bug
fix ? How do you trigger it ? Our tests do not show any issues here...
If this triggers only with non power of 2 zone size devices, then this
should be squashed in patch 8. And patch 9 could also be squashed with
patch 8 too.

--
Damien Le Moal
Western Digital Research

2022-07-29 09:06:42

by Pankaj Raghav

[permalink] [raw]
Subject: Re: [PATCH v8 10/11] dm: call dm_zone_endio after the target endio callback for zoned devices

>> if (endio) {
>> int r = endio(ti, bio, &error);
>> switch (r) {
>> @@ -1155,6 +1151,10 @@ static void clone_endio(struct bio *bio)
>> }
>> }
>>
>> + if (static_branch_unlikely(&zoned_enabled) &&
>> + unlikely(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);
>
> This patch seems completely unrelated to the series topic. Is that a bug
> fix ? How do you trigger it ? Our tests do not show any issues here...
> If this triggers only with non power of 2 zone size devices, then this
> should be squashed in patch 8. And patch 9 could also be squashed with
> patch 8 too.
>
The targets that support zoned devices such as dm-zoned, dm-linear, and
dm-crypt do not have an endio function, and even if they do (such as
dm-flakey), they don't modify the bio->bi_iter.bi_sector of the cloned
bio that is used to update the orig_bio's bi_sector in dm_zone_endio
function.

This path is triggered only for the new target dm-po2zone and not for
npo2 zone size devices in general. I will mention this is a prep patch
for the new target because I wouldn't call it a bug per se.