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

[Bart Van Assche <bvanassche@acm.org>]

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.