Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[韦龙平(Groot)]

On Sun, 24 Nov 2024 21:00:52 -0800, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 11:22:51AM +0800, LongPing Wei wrote:
> > When dm level submit a bio with bi_size > max_sectors_kb of lower device,
> > blk_mq_submit_bio will split it into several ones with limited bi_size.
> > It may be a potential unordered issue as it breaks the ZWL design in dm
> > on 6.6.
> 
> What is ZWL?  And why mention 6.6?  And why do you care about ordering?
ZWL is Zone-Write-Locking and it was replaced by Zone-Write-Plugging on 6.12.

> There is no real ordering requirement in the block layer.
According to the design constraints of zoned device, write requests in the same
zone must be dispatched in order of address and there can be no holes in the middle

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Christoph Hellwig]

On Mon, Nov 25, 2024 at 05:58:22AM +0000, 韦龙平(Groot) wrote:
> > What is ZWL?  And why mention 6.6?  And why do you care about ordering?
> ZWL is Zone-Write-Locking and it was replaced by Zone-Write-Plugging on 6.12.

That's not a generally accepted acronym.  But more importantly it's all
gone now, so completely irrelevant.

> 
> > There is no real ordering requirement in the block layer.
> According to the design constraints of zoned device, write requests in the same
> zone must be dispatched in order of address and there can be no holes in the middle

Yes.  But none of that matters at the device mapper layer.  Either the
submitter needs to ensure ordering, or you must use zone append.

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Damien Le Moal]

On 11/25/24 3:01 PM, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 05:58:22AM +0000, 韦龙平(Groot) wrote:
>>> What is ZWL?  And why mention 6.6?  And why do you care about ordering?
>> ZWL is Zone-Write-Locking and it was replaced by Zone-Write-Plugging on 6.12.
> 
> That's not a generally accepted acronym.  But more importantly it's all
> gone now, so completely irrelevant.
> 
>>
>>> There is no real ordering requirement in the block layer.
>> According to the design constraints of zoned device, write requests in the same
>> zone must be dispatched in order of address and there can be no holes in the middle
> 
> Yes.  But none of that matters at the device mapper layer.  Either the
> submitter needs to ensure ordering, or you must use zone append.

100% agree. But in this case, dm-crypt sees a single BIO which gets split when
submitted to the underlying device. But the split issues the requests in
reverse order, which breaks write ordering. So this seems like an issue with
the block layer rather than dm-crypt. Though I do not understand why I have
never seen this issue before, despite all the testing we do with all the zone
block devices, dm and FS combinations we run every week (including f2fs). So I
am not sure that the story is complete here... Never had to play with that dm
max_io_len before to get things running correctly.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Christoph Hellwig]

On Mon, Nov 25, 2024 at 03:07:24PM +0900, Damien Le Moal wrote:
> 100% agree. But in this case, dm-crypt sees a single BIO which gets split when
> submitted to the underlying device. But the split issues the requests in
> reverse order, which breaks write ordering.

I'd like to see verification of this.  People who send these kinds of
odd old kernel reports tend to have weird patches applied to their tree
as well.

Without an upstream reproducer that actually shows issues I think
this can be safely ignored.

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[LongPing Wei]

On 2024/11/25 14:11, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 03:07:24PM +0900, Damien Le Moal wrote:
>> 100% agree. But in this case, dm-crypt sees a single BIO which gets split when
>> submitted to the underlying device. But the split issues the requests in
>> reverse order, which breaks write ordering.
> 
> I'd like to see verification of this.  People who send these kinds of
> odd old kernel reports tend to have weird patches applied to their tree
> as well.
> 
> Without an upstream reproducer that actually shows issues I think
> this can be safely ignored.
> 
Thanks for your suggestions.
I will try to make another patch of fault injection to reproduce it on qemu.

Re: dm-crypt: set max_io_len as chunk_sectors of zoned device

[Mike Snitzer]

On Mon, Nov 25, 2024 at 03:49:01PM +0800, LongPing Wei wrote:
> On 2024/11/25 14:11, Christoph Hellwig wrote:
> > On Mon, Nov 25, 2024 at 03:07:24PM +0900, Damien Le Moal wrote:
> > > 100% agree. But in this case, dm-crypt sees a single BIO which gets split when
> > > submitted to the underlying device. But the split issues the requests in
> > > reverse order, which breaks write ordering.
> > 
> > I'd like to see verification of this.  People who send these kinds of
> > odd old kernel reports tend to have weird patches applied to their tree
> > as well.
> > 
> > Without an upstream reproducer that actually shows issues I think
> > this can be safely ignored.
> > 
> Thanks for your suggestions.
> I will try to make another patch of fault injection to reproduce it on qemu.
> 

If/when you do find it is relevant for latest upstream and/or
6.6-only: please add a "Fixes:" tag and also be explicit about which
kernel(s) your fix is applicable to.

Mike

Re: dm-crypt: set max_io_len as chunk_sectors of zoned device

[LongPing Wei]

On 2024/12/11 0:20, Mike Snitzer wrote:
> On Mon, Nov 25, 2024 at 03:49:01PM +0800, LongPing Wei wrote:
>> On 2024/11/25 14:11, Christoph Hellwig wrote:
>>> On Mon, Nov 25, 2024 at 03:07:24PM +0900, Damien Le Moal wrote:
>>>> 100% agree. But in this case, dm-crypt sees a single BIO which gets split when
>>>> submitted to the underlying device. But the split issues the requests in
>>>> reverse order, which breaks write ordering.
>>>
>>> I'd like to see verification of this.  People who send these kinds of
>>> odd old kernel reports tend to have weird patches applied to their tree
>>> as well.
>>>
>>> Without an upstream reproducer that actually shows issues I think
>>> this can be safely ignored.
>>>
>> Thanks for your suggestions.
>> I will try to make another patch of fault injection to reproduce it on qemu.
>>
> 
> If/when you do find it is relevant for latest upstream and/or
> 6.6-only: please add a "Fixes:" tag and also be explicit about which
> kernel(s) your fix is applicable to.
> 
> Mike
Hi, Mike

We met this issue only on 6.6 now, as we work on android devices with 
linux-6.6 and zoned storage device.
We will got android devices with linux-6.12 and zoned storage device 
after half a year and then we can try to reproduce it with scheduler 
being set to none.
I have tried to reproduce it with QEMU and nulblk, but failed as our 
test suite is deeply coupled with our android devices.

Thanks
LongPing

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[LongPing Wei]

On 2024/11/25 14:01, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 05:58:22AM +0000, 韦龙平(Groot) wrote:
>>> What is ZWL?  And why mention 6.6?  And why do you care about ordering?
>> ZWL is Zone-Write-Locking and it was replaced by Zone-Write-Plugging on 6.12.
> 
> That's not a generally accepted acronym.  But more importantly it's all
> gone now, so completely irrelevant.
> 
>>
>>> There is no real ordering requirement in the block layer.
>> According to the design constraints of zoned device, write requests in the same
>> zone must be dispatched in order of address and there can be no holes in the middle

__bio_split_to_limits will split read bios with max_sectors too, split
read bios in dm-level won't increase the number of requests dispatched 
to scsi devices.

> Yes.  But none of that matters at the device mapper layer.  Either the
> submitter needs to ensure ordering, or you must use zone append.

Just like what was said in "dm crypt: Fix zoned block device support":
> Zone append BIOs (REQ_OP_ZONE_APPEND) always specify the start sector
> of the zone to be written instead of the actual sector location to
> write. The write location is determined by the device and returned to
> the host upon completion of the operation. This interface, while simple
> and efficient for writing into sequential zones of a zoned block
> device, is incompatible with the use of sector values to calculate a
> cypher block IV. All data written in a zone end up using the same IV
> values corresponding to the first sectors of the zone, but read
> operation will specify any sector within the zone resulting in an IV
> mismatch between encryption and decryption.
> 
> To solve this problem, report to DM core that zone append operations are
> not supported. This result in the zone append operations being emulated
> using regular write operations.

Zone append writes to zoned device will be converted to normal write 
requests and submitted one by one by zone-write-locking OR 
zone-write-plugging.
This patch just want zone-append-emulation which was implemented in
dm-level could works fine even if dm-crypt got a write or zone-append 
bio with size greater than max_sectors.

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Damien Le Moal]

On 11/25/24 2:58 PM, 韦龙平(Groot) wrote:
> On Sun, 24 Nov 2024 21:00:52 -0800, Christoph Hellwig wrote:
>> On Mon, Nov 25, 2024 at 11:22:51AM +0800, LongPing Wei wrote:
>>> When dm level submit a bio with bi_size > max_sectors_kb of lower device,
>>> blk_mq_submit_bio will split it into several ones with limited bi_size.
>>> It may be a potential unordered issue as it breaks the ZWL design in dm
>>> on 6.6.
>>
>> What is ZWL?  And why mention 6.6?  And why do you care about ordering?
> ZWL is Zone-Write-Locking and it was replaced by Zone-Write-Plugging on 6.12.

6.10.

>> There is no real ordering requirement in the block layer.
> According to the design constraints of zoned device, write requests in the same
> zone must be dispatched in order of address and there can be no holes in the middle


-- 
Damien Le Moal
Western Digital Research

[PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[LongPing Wei]

When dm level submit a bio with bi_size > max_sectors_kb of lower device,
blk_mq_submit_bio will split it into several ones with limited bi_size.
It may be a potential unordered issue as it breaks the ZWL design in dm
on 6.6.
It would be better to split bio by dm_split_and_process_bio in dm level.
Considering the design of __max_io_len in dm.c, setting max_io_len for the
target is needed to make __max_io_len return a len <= max_sectors.

Signed-off-by: LongPing Wei <weilongping@oppo.com>
---
A potential case is:
f2fs submit a bio to zoned device with 1024KiB size, and the max sectors is 1024(512KiB).

ZWL of dm will just re-submit it to the lower device, then the bios will be splited into two bios.

They would be submitted to differect software queue as the context may run on different cpus.

If the previous one is on CPU0 and the later one is on CPU1, then the software queue of CPU1 dispatch the request before CPU0, the issue will be triggered.

CPU0:                     CPU1:
f2fs_submit_bio[task x]
dm_submit_bio[task x]
blk_mq_split_bios[task x]
submit_bio_noacct[task x]
                          submit_bio_noacct[task x]
                          dispatch requrest in queue[kworker1]
dispatch requrest in queue[kworker0]
---
 drivers/md/dm-crypt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 1ae2c71bb383..9b487ccb1ebc 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -3417,6 +3417,8 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		 */
 		DMDEBUG("Zone append operations will be emulated");
 		ti->emulate_zone_append = true;
+		WARN_ON(dm_set_target_max_io_len(ti,
+			cc->dev->bdev->bd_queue->limits.chunk_sectors));
 	}
 
 	if (crypt_integrity_aead(cc) || cc->integrity_iv_size) {
-- 
2.34.1

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Christoph Hellwig]

On Mon, Nov 25, 2024 at 11:22:51AM +0800, LongPing Wei wrote:
> When dm level submit a bio with bi_size > max_sectors_kb of lower device,
> blk_mq_submit_bio will split it into several ones with limited bi_size.
> It may be a potential unordered issue as it breaks the ZWL design in dm
> on 6.6.

What is ZWL?  And why mention 6.6?  And why do you care about ordering?
There is no real ordering requirement in the block layer.

Re: [PATCH] dm-crypt: set max_io_len as chunk_sectors of zoned device

[Damien Le Moal]

On 11/25/24 2:00 PM, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 11:22:51AM +0800, LongPing Wei wrote:
>> When dm level submit a bio with bi_size > max_sectors_kb of lower device,
>> blk_mq_submit_bio will split it into several ones with limited bi_size.
>> It may be a potential unordered issue as it breaks the ZWL design in dm
>> on 6.6.
> 
> What is ZWL?  And why mention 6.6?  And why do you care about ordering?
> There is no real ordering requirement in the block layer.
> 

ZWL == zone write locking. That is, the mq-deadline request-based write request
ordering control we had before zone write plugging.
This is about ordering of write requests resulting from a split BIO.

-- 
Damien Le Moal
Western Digital Research