diff mbox series

[v2,03/28] block: Introduce blk_zone_update_request_bio()

Message ID 20240325044452.3125418-4-dlemoal@kernel.org (mailing list archive)
State Superseded
Headers show
Series Zone write plugging | expand

Commit Message

Damien Le Moal March 25, 2024, 4:44 a.m. UTC
On completion of a zone append request, the request sector indicates the
location of the written data. This value must be returned to the user
through the BIO iter sector. This is done in 2 places: in
blk_complete_request() and in blk_update_request(). Introduce the inline
helper function blk_zone_update_request_bio() to avoid duplicating
this BIO update for zone append requests, and to compile out this
helper call when CONFIG_BLK_DEV_ZONED is not enabled.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-mq.c | 11 +++++------
 block/blk.h    | 19 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 7 deletions(-)

Comments

Bart Van Assche March 25, 2024, 7:52 p.m. UTC | #1
On 3/24/24 21:44, Damien Le Moal wrote:
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8aeb8e96f1a7..9e6e2a9a147c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -820,11 +820,11 @@ static void blk_complete_request(struct request *req)
>   		/* Completion has already been traced */
>   		bio_clear_flag(bio, BIO_TRACE_COMPLETION);
>   
> -		if (req_op(req) == REQ_OP_ZONE_APPEND)
> -			bio->bi_iter.bi_sector = req->__sector;
> -
> -		if (!is_flush)
> +		if (!is_flush) {
> +			blk_zone_update_request_bio(req, bio);
>   			bio_endio(bio);
> +		}

The above change includes a behavior change. It seems wrong to me not
to call blk_zone_update_request_bio() for REQ_OP_ZONE_APPEND requests if
RQF_FLUSH_SEQ has been set.

Thanks,

Bart.
Damien Le Moal March 25, 2024, 11:23 p.m. UTC | #2
On 3/26/24 04:52, Bart Van Assche wrote:
> On 3/24/24 21:44, Damien Le Moal wrote:
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 8aeb8e96f1a7..9e6e2a9a147c 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -820,11 +820,11 @@ static void blk_complete_request(struct request *req)
>>   		/* Completion has already been traced */
>>   		bio_clear_flag(bio, BIO_TRACE_COMPLETION);
>>   
>> -		if (req_op(req) == REQ_OP_ZONE_APPEND)
>> -			bio->bi_iter.bi_sector = req->__sector;
>> -
>> -		if (!is_flush)
>> +		if (!is_flush) {
>> +			blk_zone_update_request_bio(req, bio);
>>   			bio_endio(bio);
>> +		}
> 
> The above change includes a behavior change. It seems wrong to me not
> to call blk_zone_update_request_bio() for REQ_OP_ZONE_APPEND requests if
> RQF_FLUSH_SEQ has been set.

REQ_OP_ZONE_APPEND + RQF_FLUSH_SEQ is not something supported, and this patch
series is not changing that. The reason is that the flush machinery is not
zone-append aware and will break if such request is issued for a device that
does not support fua. We probably should check for this, but that is not
something for this series to do and should be a separate fix.
Christoph Hellwig March 26, 2024, 6:37 a.m. UTC | #3
On Tue, Mar 26, 2024 at 08:23:19AM +0900, Damien Le Moal wrote:
> REQ_OP_ZONE_APPEND + RQF_FLUSH_SEQ is not something supported, and this patch
> series is not changing that. The reason is that the flush machinery is not
> zone-append aware and will break if such request is issued for a device that
> does not support fua. We probably should check for this, but that is not
> something for this series to do and should be a separate fix.

Btw, I don't think we're even catching this right now.  Would be great
to have a submission path check for it.
Damien Le Moal March 26, 2024, 7:47 a.m. UTC | #4
On 3/26/24 15:37, Christoph Hellwig wrote:
> On Tue, Mar 26, 2024 at 08:23:19AM +0900, Damien Le Moal wrote:
>> REQ_OP_ZONE_APPEND + RQF_FLUSH_SEQ is not something supported, and this patch
>> series is not changing that. The reason is that the flush machinery is not
>> zone-append aware and will break if such request is issued for a device that
>> does not support fua. We probably should check for this, but that is not
>> something for this series to do and should be a separate fix.
> 
> Btw, I don't think we're even catching this right now.  Would be great
> to have a submission path check for it.

Yep, we are not checking this, but we should. Will send a separate patch for this.
Hannes Reinecke March 27, 2024, 7:01 a.m. UTC | #5
On 3/25/24 05:44, Damien Le Moal wrote:
> On completion of a zone append request, the request sector indicates the
> location of the written data. This value must be returned to the user
> through the BIO iter sector. This is done in 2 places: in
> blk_complete_request() and in blk_update_request(). Introduce the inline
> helper function blk_zone_update_request_bio() to avoid duplicating
> this BIO update for zone append requests, and to compile out this
> helper call when CONFIG_BLK_DEV_ZONED is not enabled.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   block/blk-mq.c | 11 +++++------
>   block/blk.h    | 19 ++++++++++++++++++-
>   2 files changed, 23 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8aeb8e96f1a7..9e6e2a9a147c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -820,11 +820,11 @@  static void blk_complete_request(struct request *req)
 		/* Completion has already been traced */
 		bio_clear_flag(bio, BIO_TRACE_COMPLETION);
 
-		if (req_op(req) == REQ_OP_ZONE_APPEND)
-			bio->bi_iter.bi_sector = req->__sector;
-
-		if (!is_flush)
+		if (!is_flush) {
+			blk_zone_update_request_bio(req, bio);
 			bio_endio(bio);
+		}
+
 		bio = next;
 	} while (bio);
 
@@ -926,8 +926,7 @@  bool blk_update_request(struct request *req, blk_status_t error,
 
 		/* Don't actually finish bio if it's part of flush sequence */
 		if (!bio->bi_iter.bi_size && !is_flush) {
-			if (req_op(req) == REQ_OP_ZONE_APPEND)
-				bio->bi_iter.bi_sector = req->__sector;
+			blk_zone_update_request_bio(req, bio);
 			bio_endio(bio);
 		}
 
diff --git a/block/blk.h b/block/blk.h
index 5cac4e29ae17..a12cde1d45de 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -409,12 +409,29 @@  static inline struct bio *blk_queue_bounce(struct bio *bio,
 
 #ifdef CONFIG_BLK_DEV_ZONED
 void disk_free_zone_bitmaps(struct gendisk *disk);
+static inline void blk_zone_update_request_bio(struct request *rq,
+					       struct bio *bio)
+{
+	/*
+	 * For zone append requests, the request sector indicates the location
+	 * at which the BIO data was written. Return this value to the BIO
+	 * issuer through the BIO iter sector.
+	 */
+	if (req_op(rq) == REQ_OP_ZONE_APPEND)
+		bio->bi_iter.bi_sector = rq->__sector;
+}
 int blkdev_report_zones_ioctl(struct block_device *bdev, unsigned int cmd,
 		unsigned long arg);
 int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
 		unsigned int cmd, unsigned long arg);
 #else /* CONFIG_BLK_DEV_ZONED */
-static inline void disk_free_zone_bitmaps(struct gendisk *disk) {}
+static inline void disk_free_zone_bitmaps(struct gendisk *disk)
+{
+}
+static inline void blk_zone_update_request_bio(struct request *rq,
+					       struct bio *bio)
+{
+}
 static inline int blkdev_report_zones_ioctl(struct block_device *bdev,
 		unsigned int cmd, unsigned long arg)
 {