Re: [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvmet: bi_write_stream lost on chained bios in nvmet_bdev_execute_rw()
--

commit ee4faa4396ddf960be1a3f58157a725fb3691c6f
Author: Guixin Liu <kanie@linux.alibaba.com>

nvmet: translate placement identifiers of writes to write streams

This commit translates NVMe Data Placement Directive placement identifiers
on write commands into block layer write streams on the backing block
device when FDP is enabled on the namespace.

> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index f2d9e8901df4e..ff480f9ab9906 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -300,6 +300,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>  	bio->bi_iter.bi_sector = sector;
>  	bio->bi_private = req;
>  	bio->bi_end_io = nvmet_bio_done;
> +	if (req->cmd->rw.opcode == nvme_cmd_write &&
> +	    nvmet_fdp_enabled(req->ns)) {
> +		bio->bi_write_stream = nvmet_fdp_write_stream(req);
> +	}
>  
>  	blk_start_plug(&plug);
>  	if (req->metadata_len)

[Severity: Medium]
What happens when a write command spans multiple bios in
nvmet_bdev_execute_rw()?

When req->sg_cnt exceeds BIO_MAX_VECS (or bio_add_page() otherwise fills
the current bio), the for_each_sg() loop allocates a new chained bio:

	for_each_sg(req->sg, sg, req->sg_cnt, i) {
		while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
				!= sg->length) {
			struct bio *prev = bio;
			...
			bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
					opf, GFP_KERNEL);
			bio->bi_iter.bi_sector = sector;

			bio_chain(bio, prev);
			submit_bio(prev);
		}

Since bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
not copy bi_write_stream from prev, won't all subsequent chained bios for
the write command be submitted with bi_write_stream set to 0 instead of
the stream returned by nvmet_fdp_write_stream(req), preventing them from
merging with the initial bio and routing the tail of the write to the
default stream instead of the host-selected reclaim unit?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=10