On 11/27/24 3:45 PM, Christoph Hellwig wrote:
> On Wed, Nov 27, 2024 at 03:43:11PM +0900, Damien Le Moal wrote:
>> I thought about that one. The problem with it is the significant performance
>> penalty that the context switch to the zone write plug BIO work causes. But
>> that is for qd=1 writes only... If that is acceptable, that solution is
>> actually the easiest. The overhead is not an issue for HDDs and for ZNS SSDs,
>> zone append to the rescue ! So maybe for now, it is best to just do that.
>
> The NOWAIT flag doesn't really make much sense for synchronous QD=1
> writes, the rationale for it is not block the submission thread when
> doing batch submissions with asynchronous completions.
>
> So I think this should be fine (famous last words..)
Got something simple and working.
Will run it through our test suite to make sure it does not regress anything.
The change is:
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 263e28b72053..648b8d2534a4 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -746,9 +746,25 @@ static bool blk_zone_wplug_handle_reset_all(struct bio *bio)
return false;
}
-static inline void blk_zone_wplug_add_bio(struct blk_zone_wplug *zwplug,
- struct bio *bio, unsigned int nr_segs)
+static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk,
+ struct blk_zone_wplug *zwplug)
{
+ /*
+ * 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));
+ refcount_inc(&zwplug->ref);
+ queue_work(disk->zone_wplugs_wq, &zwplug->bio_work);
+}
+
+static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
+ struct blk_zone_wplug *zwplug,
+ struct bio *bio, unsigned int nr_segs)
+{
+ bool schedule_bio_work = false;
+
/*
* Grab an extra reference on the BIO request queue usage counter.
* This reference will be reused to submit a request for the BIO for
@@ -764,6 +780,16 @@ static inline void blk_zone_wplug_add_bio(struct blk_zone_wplug *zwplug,
*/
bio_clear_polled(bio);
+ /*
+ * REQ_NOWAIT BIOs are always handled using the zone write plug BIO
+ * work, which can block. So clear the REQ_NOWAIT flag and schedule the
+ * work if this is the first BIO we are plugging.
+ */
+ if (bio->bi_opf & REQ_NOWAIT) {
+ schedule_bio_work = bio_list_empty(&zwplug->bio_list);
+ bio->bi_opf &= ~REQ_NOWAIT;
+ }
+
/*
* Reuse the poll cookie field to store the number of segments when
* split to the hardware limits.
@@ -777,6 +803,9 @@ static inline void blk_zone_wplug_add_bio(struct blk_zone_wplug *zwplug,
* at the tail of the list to preserve the sequential write order.
*/
bio_list_add(&zwplug->bio_list, bio);
+
+ if (schedule_bio_work)
+ disk_zone_wplug_schedule_bio_work(disk, zwplug);
}
/*
@@ -970,7 +999,10 @@ static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs)
zwplug = disk_get_and_lock_zone_wplug(disk, sector, gfp_mask, &flags);
if (!zwplug) {
- bio_io_error(bio);
+ if (bio->bi_opf & REQ_NOWAIT)
+ bio_wouldblock_error(bio);
+ else
+ bio_io_error(bio);
return true;
}
@@ -979,9 +1011,11 @@ static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs)
/*
* If the zone is already plugged or has a pending error, add the BIO
- * to the plug BIO list. Otherwise, plug and let the BIO execute.
+ * to the plug BIO list. Do the same for REQ_NOWAIT BIOs to ensure that
+ * we will not see a BLK_STS_AGAIN failure if we let the BIO execute.
+ * Otherwise, plug and let the BIO execute.
*/
- if (zwplug->flags & BLK_ZONE_WPLUG_BUSY)
+ if (zwplug->flags & BLK_ZONE_WPLUG_BUSY || (bio->bi_opf & REQ_NOWAIT))
goto plug;
/*
@@ -999,7 +1033,7 @@ static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs)
plug:
zwplug->flags |= BLK_ZONE_WPLUG_PLUGGED;
- blk_zone_wplug_add_bio(zwplug, bio, nr_segs);
+ disk_zone_wplug_add_bio(disk, zwplug, bio, nr_segs);
spin_unlock_irqrestore(&zwplug->lock, flags);
@@ -1083,19 +1117,6 @@ bool blk_zone_plug_bio(struct bio *bio, unsigned int nr_segs)
}
EXPORT_SYMBOL_GPL(blk_zone_plug_bio);
-static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk,
- struct blk_zone_wplug *zwplug)
-{
- /*
- * 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));
- refcount_inc(&zwplug->ref);
- queue_work(disk->zone_wplugs_wq, &zwplug->bio_work);
-}
-
static void disk_zone_wplug_unplug_bio(struct gendisk *disk,
struct blk_zone_wplug *zwplug)
{
--
Damien Le Moal
Western Digital Research
