Re: [PATCH 2/2] block: remove bio_add_zone_append_page

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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