[PATCH 11/11] nvmet: add an FDP write translation tracepoint

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] nvmet: trace_nvmet_fdp_write_translate() called on non-directive writes
- [Medium] nvmet: missing bi_write_stream copy on chained bios in nvmet_bdev_execute_rw()
--

commit 910a51c080606805fab754e08158c76023d49e00
Author: Guixin Liu <kanie at linux.alibaba.com>

nvmet: add an FDP write translation tracepoint

This commit adds the nvmet_fdp_write_translate tracepoint to the bdev write
path to report the placement identifier from a write command and the backing
write stream it is translated to.

> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index ff480f9ab9906..139be9edad969 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c

[ ... ]

> @@ -302,7 +303,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>  	bio->bi_end_io = nvmet_bio_done;
>  	if (req->cmd->rw.opcode == nvme_cmd_write &&
>  	    nvmet_fdp_enabled(req->ns)) {
> +		u16 pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
> +
>  		bio->bi_write_stream = nvmet_fdp_write_stream(req);
> +		trace_nvmet_fdp_write_translate(req, pid, bio->bi_write_stream);
>  	}

[Severity: Medium]
Should trace_nvmet_fdp_write_translate() only be called when the write
command actually carries the Data Placement Directive?

The commit message notes that the tracepoint "reports the placement
identifier carried by a write command with the Data Placement Directive
and the backing write stream it is translated to."

In drivers/nvme/target/nvmet.h, nvmet_fdp_write_stream() checks the
directive type before treating the upper 16 bits of req->cmd->rw.dsmgmt
as a placement identifier:

static inline u8 nvmet_fdp_write_stream(struct nvmet_req *req)
{
	u16 control = le16_to_cpu(req->cmd->rw.control);
	u16 pid;

	if ((control & 0xf0) != (NVME_RW_DTYPE_DPLCMT & 0xf0))
		return 0;

	pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;

In nvmet_bdev_execute_rw(), however, trace_nvmet_fdp_write_translate() is
invoked for every nvme_cmd_write to an FDP-enabled namespace without
checking whether req->cmd->rw.control specifies NVME_RW_DTYPE_DPLCMT.

When a host issues an ordinary non-directive write (or a write with a
different directive type) to an FDP-enabled namespace, won't this still
fire the tracepoint and report the upper 16 bits of req->cmd->rw.dsmgmt
as pid alongside write_stream=0, making non-directive writes
indistinguishable from directive writes whose placement identifier failed
translation?

[Severity: Medium]
Also in nvmet_bdev_execute_rw(), what happens when a write with the Data
Placement Directive spans more than BIO_MAX_VECS (256) scatterlist
segments and has to be split across chained bios?

Later in nvmet_bdev_execute_rw(), if bio_add_page() cannot fit all
segments into the initial bio, a continuation bio is allocated and
chained:

	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);
		}

Because bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
not copy prev->bi_write_stream, the continuation bios keep
bi_write_stream == 0 while trace_nvmet_fdp_write_translate() reports that
the request was translated to the initial bio's stream.

As a result, blk_rq_merge_ok() will reject merging the continuation bios
with the initial bio due to the mismatched bi_write_stream, and
nvme_setup_rw() will submit the remainder of the write to the backing
device without NVME_RW_DTYPE_DPLCMT (placing it into default stream 0).
Should prev->bi_write_stream be copied to each chained bio in
nvmet_bdev_execute_rw()?

>  
>  	blk_start_plug(&plug);

[ ... ]

>
>
>

[]

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