Re: [PATCH v21 07/12] blk-zoned: Support pipelining of zoned writes

Bart Van Assche
2025-07-18T16:30:00Z

On 7/18/25 12:38 AM, Damien Le Moal wrote:
> On 7/18/25 05:58, Bart Van Assche wrote:
>> @@ -768,14 +771,19 @@ static bool blk_zone_wplug_handle_reset_all(struct bio *bio)
>> static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk,
>> struct blk_zone_wplug *zwplug)
>> {
>> + lockdep_assert_held(&zwplug->lock);
>
> Unrelated change. Please move this to a prep patch.

I will drop this change since I don't really need this change.

>> +
>> /*
>> * Take a reference on the zone write plug and schedule the submission
>> * of the next plugged BIO. blk_zone_wplug_bio_work() will release the
>> * reference we take here.
>> */
>> - WARN_ON_ONCE(!(zwplug->flags & BLK_ZONE_WPLUG_PLUGGED));
>
> Why do you remove this warning ?

This warning probably can be retained. I will look into restoring it.

>> @@ -972,9 +980,12 @@ static bool blk_zone_wplug_prepare_bio(struct blk_zone_wplug *zwplug,
>> return true;
>> }
>>
>> -static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs)
>> +static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs,
>> + int from_cpu)
>> {
>> struct gendisk *disk = bio->bi_bdev->bd_disk;
>> + const bool ordered_hwq = bio_op(bio) != REQ_OP_ZONE_APPEND &&
>> + disk->queue->limits.features & BLK_FEAT_ORDERED_HWQ;
>
> This is not correct. If the BIO is a zone append and
> blk_zone_wplug_handle_write() is called, it means that we need to handle the BIO
> using zone append emulation, that is, the BIO will be a regular write. So you
> must treat it as if it originally was a regular write.

Hmm ... my understanding is that zone append emulation and also the
conversion of REQ_OP_ZONE_APPEND into REQ_OP_WRITE happens after the
above code has been executed, namely by blk_zone_wplug_prepare_bio().
From that function:

[ ... ]
if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
bio->bi_opf &= ~REQ_OP_MASK;
bio->bi_opf |= REQ_OP_WRITE | REQ_NOMERGE;
[ ... ]

Did I perhaps misunderstand your comment?

>> + if (refcount_read(&zwplug->ref) == 2)
>> + zwplug->from_cpu = -1;
>
> This needs a comment explaining why you use the plug ref count instead of
> unconditionally clearing from_cpu.

I'm considering to add the following comment:

/*
* zwplug->from_cpu must not change while one or more writes are pending
* for the zone associated with zwplug. zwplug->ref is 2 when the plug
* is unused (one reference taken when the plug was allocated and
* another reference taken by the caller context). Reset
* zwplug->from_cpu if no more writes are pending.
*/

Thanks,

Bart.