Re: [PATCH 06/26] block: Introduce zone write plugging

[Damien Le Moal <dlemoal@kernel.org>]

On 2/4/24 12:56, Ming Lei wrote:
> On Fri, Feb 02, 2024 at 04:30:44PM +0900, Damien Le Moal wrote:
>> +/*
>> + * Zone write plug flags bits:
>> + *  - BLK_ZONE_WPLUG_CONV: Indicate that the zone is a conventional one. Writes
>> + *    to these zones are never plugged.
>> + *  - BLK_ZONE_WPLUG_PLUGGED: Indicate that the zone write plug is plugged,
>> + *    that is, that write BIOs are being throttled due to a write BIO already
>> + *    being executed or the zone write plug bio list is not empty.
>> + */
>> +#define BLK_ZONE_WPLUG_CONV	(1U << 0)
>> +#define BLK_ZONE_WPLUG_PLUGGED	(1U << 1)
> 
> BLK_ZONE_WPLUG_PLUGGED == !bio_list_empty(&zwplug->bio_list), so looks
> this flag isn't necessary.

No, it is. As the description says, the flag not only indicates that there are
plugged BIOs, but it also indicates that there is a write for the zone
in-flight. And that can happen even with the BIO list being empty. E.g. for a
qd=1 workload of small BIOs, no BIO will ever be added to the BIO list, but the
zone still must be marked as "plugged" when a write BIO is issued for it.

>> +static inline void blk_zone_wplug_add_bio(struct blk_zone_wplug *zwplug,
>> +					  struct bio *bio, unsigned int nr_segs)
>> +{
>> +	/*
>> +	 * Keep a reference on the BIO request queue usage. This reference will
>> +	 * be dropped either if the BIO is failed or after it is issued and
>> +	 * completes.
>> +	 */
>> +	percpu_ref_get(&bio->bi_bdev->bd_disk->queue->q_usage_counter);
> 
> It is fragile to get nested usage_counter, and same with grabbing/releasing it
> from different contexts or even functions, and it could be much better to just
> let block layer maintain it.
> 
> From patch 23's change:
> 
> +	 * Zoned block device information. Reads of this information must be
> +	 * protected with blk_queue_enter() / blk_queue_exit(). Modifying this
> 
> Anytime if there is in-flight bio, the block device is opened, so both gendisk and
> request_queue are live, so not sure if this .q_usage_counter protection
> is needed.

Hannes also commented about this. Let me revisit this.

>> +	/*
>> +	 * blk-mq devices will reuse the reference on the request queue usage
>> +	 * we took when the BIO was plugged, but the submission path for
>> +	 * BIO-based devices will not do that. So drop this reference here.
>> +	 */
>> +	if (bio->bi_bdev->bd_has_submit_bio)
>> +		blk_queue_exit(bio->bi_bdev->bd_disk->queue);
> 
> But I don't see where this reference is reused for blk-mq in this patch,
> care to point it out?

This patch modifies blk_mq_submit_bio() to add a "goto new_request" at the top
for any BIO flagged with BIO_FLAG_ZONE_WRITE_PLUGGING. So when a plugged BIO is
unplugged and submitted again, the reference that was taken in
blk_zone_wplug_add_bio() is reused for the new request for that BIO.


-- 
Damien Le Moal
Western Digital Research