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
