| Message ID | 20241030051859.280923-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] block: remove zone append special casing from the direct I/O path | expand |
On 10/29/24 22:18, Christoph Hellwig wrote: > This is only used by the nvmet zns passthrough code, which can trivially > just use bio_add_pc_page and do the sanity check for the max zone append > limit itself. > > All future zoned file systems should follow the btrfs lead and let the > upper layers fill up bios unlimited by hardware constraints and split > them to the limits in the I/O submission handler. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 33 --------------------------------- > drivers/nvme/target/zns.c | 21 +++++++++++++-------- > include/linux/bio.h | 2 -- > 3 files changed, 13 insertions(+), 43 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 6a60d62a529d..daceb0a5c1d7 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1064,39 +1064,6 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, > } > EXPORT_SYMBOL(bio_add_pc_page); > > -/** > - * bio_add_zone_append_page - attempt to add page to zone-append bio > - * @bio: destination bio > - * @page: page to add > - * @len: vec entry length > - * @offset: vec entry offset > - * > - * Attempt to add a page to the bio_vec maplist of a bio that will be submitted > - * for a zone-append request. This can fail for a number of reasons, such as the > - * bio being full or the target block device is not a zoned block device or > - * other limitations of the target block device. The target block device must > - * allow bio's up to PAGE_SIZE, so it is always possible to add a single page > - * to an empty bio. > - * > - * Returns: number of bytes added to the bio, or 0 in case of a failure. > - */ > -int bio_add_zone_append_page(struct bio *bio, struct page *page, > - unsigned int len, unsigned int offset) > -{ > - struct request_queue *q = bdev_get_queue(bio->bi_bdev); > - bool same_page = false; > - > - if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_ZONE_APPEND)) > - return 0; > - > - if (WARN_ON_ONCE(!bdev_is_zoned(bio->bi_bdev))) > - return 0; > - > - return bio_add_hw_page(q, bio, page, len, offset, > - queue_max_zone_append_sectors(q), &same_page); > -} > -EXPORT_SYMBOL_GPL(bio_add_zone_append_page); > - > /** > * __bio_add_page - add page(s) to a bio in a new segment > * @bio: destination bio > diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c > index af9e13be7678..3aef35b05111 100644 > --- a/drivers/nvme/target/zns.c > +++ b/drivers/nvme/target/zns.c > @@ -537,6 +537,7 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) > u16 status = NVME_SC_SUCCESS; > unsigned int total_len = 0; > struct scatterlist *sg; > + u32 data_len = nvmet_rw_data_len(req); > struct bio *bio; > int sg_cnt; > > @@ -544,6 +545,13 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) > if (!nvmet_check_transfer_len(req, nvmet_rw_data_len(req))) > return; > > + if (data_len > > + bdev_max_zone_append_sectors(req->ns->bdev) << SECTOR_SHIFT) { > + req->error_loc = offsetof(struct nvme_rw_command, length); > + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR; > + goto out; > + } > + > if (!req->sg_cnt) { > nvmet_req_complete(req, 0); > return; > @@ -576,20 +584,17 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) > bio->bi_opf |= REQ_FUA; > > for_each_sg(req->sg, sg, req->sg_cnt, sg_cnt) { > - struct page *p = sg_page(sg); > - unsigned int l = sg->length; > - unsigned int o = sg->offset; > - unsigned int ret; > + unsigned int len = sg->length; > > - ret = bio_add_zone_append_page(bio, p, l, o); > - if (ret != sg->length) { > + if (bio_add_pc_page(bdev_get_queue(bio->bi_bdev), bio, > + sg_page(sg), len, sg->offset) != len) { bio_add_pc_page() comment :- This should only be used by passthrough bios. Sorry I didn't understand use of passthru bio interface here. From host/nvme.h:nvme_req_op(): REQ_OP_DRV_OUT/REQ_OP_DRV_IN are the passthru requests, and nvme_req_op() is used in nvmet_passthru_execute_cmd() to map the incoming nvme passthru command into block layer passthru request i.e. REQ_OP_DRV_IN or REQ_OP_DRV_OUT:- nvme/target/passthru.c :- 319 rq = blk_mq_alloc_request(q, nvme_req_op(req->cmd), 0); In nvmet_bdev_execute_zone_append() bio->bi_opf set to opf local var that is initialized at the start of the function to :- const blk_opf_t opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE; Hence this is not a passthru request but zone append request ? if that is true should we just use the bio_add_hw_page() ? since bio_add_pc_page() is a simple wrapper over bio_add_hw_page() without the additional checks present in bio_add_zone_append_page() ? unless for some reason I failed to understand REQ_OP_ZONE_APPEND is categorized here as a passthru request, then sorry for wasting your time ... -ck
On Wed, Oct 30, 2024 at 07:30:49AM +0000, Chaitanya Kulkarni wrote: > if that is true should we just use the bio_add_hw_page() ? since > bio_add_pc_page() is a simple wrapper over bio_add_hw_page() without > the additional checks present in bio_add_zone_append_page() ? bio_add_hw_page is currently static. But just renaming bio_add_pc_page to bio_add_hw_page and finding a different name for the version with the same_page argument sounds like a good idea, but that's for a follow on patch. I can look into that. > unless for some reason I failed to understand REQ_OP_ZONE_APPEND > is categorized here as a passthru request, then sorry for wasting > your time ... It is not a passthrough request, but it is treated as one.
On 10/30/24 06:47, Christoph Hellwig wrote: > On Wed, Oct 30, 2024 at 07:30:49AM +0000, Chaitanya Kulkarni wrote: >> if that is true should we just use the bio_add_hw_page() ? since >> bio_add_pc_page() is a simple wrapper over bio_add_hw_page() without >> the additional checks present in bio_add_zone_append_page() ? > bio_add_hw_page is currently static. But just renaming bio_add_pc_page > to bio_add_hw_page and finding a different name for the version with > the same_page argument sounds like a good idea, but that's for a follow > on patch. I can look into that. > sounds like a plan, looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
diff --git a/block/bio.c b/block/bio.c index 6a60d62a529d..daceb0a5c1d7 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1064,39 +1064,6 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, } EXPORT_SYMBOL(bio_add_pc_page); -/** - * bio_add_zone_append_page - attempt to add page to zone-append bio - * @bio: destination bio - * @page: page to add - * @len: vec entry length - * @offset: vec entry offset - * - * Attempt to add a page to the bio_vec maplist of a bio that will be submitted - * for a zone-append request. This can fail for a number of reasons, such as the - * bio being full or the target block device is not a zoned block device or - * other limitations of the target block device. The target block device must - * allow bio's up to PAGE_SIZE, so it is always possible to add a single page - * to an empty bio. - * - * Returns: number of bytes added to the bio, or 0 in case of a failure. - */ -int bio_add_zone_append_page(struct bio *bio, struct page *page, - unsigned int len, unsigned int offset) -{ - struct request_queue *q = bdev_get_queue(bio->bi_bdev); - bool same_page = false; - - if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_ZONE_APPEND)) - return 0; - - if (WARN_ON_ONCE(!bdev_is_zoned(bio->bi_bdev))) - return 0; - - return bio_add_hw_page(q, bio, page, len, offset, - queue_max_zone_append_sectors(q), &same_page); -} -EXPORT_SYMBOL_GPL(bio_add_zone_append_page); - /** * __bio_add_page - add page(s) to a bio in a new segment * @bio: destination bio diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c index af9e13be7678..3aef35b05111 100644 --- a/drivers/nvme/target/zns.c +++ b/drivers/nvme/target/zns.c @@ -537,6 +537,7 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) u16 status = NVME_SC_SUCCESS; unsigned int total_len = 0; struct scatterlist *sg; + u32 data_len = nvmet_rw_data_len(req); struct bio *bio; int sg_cnt; @@ -544,6 +545,13 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) if (!nvmet_check_transfer_len(req, nvmet_rw_data_len(req))) return; + if (data_len > + bdev_max_zone_append_sectors(req->ns->bdev) << SECTOR_SHIFT) { + req->error_loc = offsetof(struct nvme_rw_command, length); + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR; + goto out; + } + if (!req->sg_cnt) { nvmet_req_complete(req, 0); return; @@ -576,20 +584,17 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req) bio->bi_opf |= REQ_FUA; for_each_sg(req->sg, sg, req->sg_cnt, sg_cnt) { - struct page *p = sg_page(sg); - unsigned int l = sg->length; - unsigned int o = sg->offset; - unsigned int ret; + unsigned int len = sg->length; - ret = bio_add_zone_append_page(bio, p, l, o); - if (ret != sg->length) { + if (bio_add_pc_page(bdev_get_queue(bio->bi_bdev), bio, + sg_page(sg), len, sg->offset) != len) { status = NVME_SC_INTERNAL; goto out_put_bio; } - total_len += sg->length; + total_len += len; } - if (total_len != nvmet_rw_data_len(req)) { + if (total_len != data_len) { status = NVME_SC_INTERNAL | NVME_STATUS_DNR; goto out_put_bio; } diff --git a/include/linux/bio.h b/include/linux/bio.h index faceadb040f9..4a1bf43ca53d 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -418,8 +418,6 @@ bool __must_check bio_add_folio(struct bio *bio, struct folio *folio, size_t len, size_t off); extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, unsigned int, unsigned int); -int bio_add_zone_append_page(struct bio *bio, struct page *page, - unsigned int len, unsigned int offset); void __bio_add_page(struct bio *bio, struct page *page, unsigned int len, unsigned int off); void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
This is only used by the nvmet zns passthrough code, which can trivially just use bio_add_pc_page and do the sanity check for the max zone append limit itself. All future zoned file systems should follow the btrfs lead and let the upper layers fill up bios unlimited by hardware constraints and split them to the limits in the I/O submission handler. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 33 --------------------------------- drivers/nvme/target/zns.c | 21 +++++++++++++-------- include/linux/bio.h | 2 -- 3 files changed, 13 insertions(+), 43 deletions(-)