| Message ID | 20240325044452.3125418-4-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Zone write plugging | expand |
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.
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.
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.
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.
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 --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) {
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(-)