[PATCHv13 00/11] block write streams with nvme fdp

[PATCHv13 00/11] block write streams with nvme fdp

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Previous discussion threads:

  v12: https://lore.kernel.org/linux-nvme/20241206221801.790690-1-kbusch@meta.com/T/#u
  v11: https://lore.kernel.org/linux-nvme/20241206015308.3342386-1-kbusch@meta.com/T/#u

Changes from v12:

 - Removed statx. We need additional time to consider the best way to
   expose these attributes. Until then, applications can fallback to the
   sysfs block attribute to query write stream settings.

 - More verbose logging on error.

 - Added reviews.

 - Explicitly clear the return value to 0 for unsupported or
   unrecognized FDP configs; while it's not technically needed (it's
   already 0 in these paths), it makes it clear that a non-error return
   wasn't an accidental oversight.

 - Fixed the compiler warnings from unitialized variable; switched the
   do-while to a more clear for-loop.

 - Fixed long-line wrapping

 - Memory leak when cleaning up the namespace head.

 - Don't rescan FDP configuration if we successfully set it up before.
   The namespaces' FDP configuration is static.

 - Better function name querying the size granularity of the FDP reclaim
   unit.

Christoph Hellwig (7):
  fs: add a write stream field to the kiocb
  block: add a bi_write_stream field
  block: introduce a write_stream_granularity queue limit
  block: expose write streams for block device nodes
  nvme: add a nvme_get_log_lsi helper
  nvme: pass a void pointer to nvme_get/set_features for the result
  nvme: add FDP definitions

Keith Busch (4):
  block: introduce max_write_streams queue limit
  io_uring: enable per-io write streams
  nvme: register fdp parameters with the block layer
  nvme: use fdp streams if write stream is provided

 Documentation/ABI/stable/sysfs-block |  15 +++
 block/bio.c                          |   2 +
 block/blk-crypto-fallback.c          |   1 +
 block/blk-merge.c                    |   4 +
 block/blk-sysfs.c                    |   6 +
 block/bounce.c                       |   1 +
 block/fops.c                         |  23 ++++
 drivers/nvme/host/core.c             | 186 ++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h             |   7 +-
 include/linux/blk_types.h            |   1 +
 include/linux/blkdev.h               |  16 +++
 include/linux/fs.h                   |   1 +
 include/linux/nvme.h                 |  77 +++++++++++
 include/uapi/linux/io_uring.h        |   4 +
 io_uring/io_uring.c                  |   2 +
 io_uring/rw.c                        |   1 +
 16 files changed, 341 insertions(+), 6 deletions(-)

-- 
2.43.5

[PATCHv13 05/11] block: expose write streams for block device nodes

[Keith Busch]

From: Christoph Hellwig <hch@lst.de>

Use the per-kiocb write stream if provided, or map temperature hints to
write streams (which is a bit questionable, but this shows how it is
done).

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
[kbusch: removed statx reporting]
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/fops.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/block/fops.c b/block/fops.c
index 6d5c4fc5a2168..f16aa39bf5bad 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -73,6 +73,7 @@ static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
 	}
 	bio.bi_iter.bi_sector = pos >> SECTOR_SHIFT;
 	bio.bi_write_hint = file_inode(iocb->ki_filp)->i_write_hint;
+	bio.bi_write_stream = iocb->ki_write_stream;
 	bio.bi_ioprio = iocb->ki_ioprio;
 	if (iocb->ki_flags & IOCB_ATOMIC)
 		bio.bi_opf |= REQ_ATOMIC;
@@ -206,6 +207,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 	for (;;) {
 		bio->bi_iter.bi_sector = pos >> SECTOR_SHIFT;
 		bio->bi_write_hint = file_inode(iocb->ki_filp)->i_write_hint;
+		bio->bi_write_stream = iocb->ki_write_stream;
 		bio->bi_private = dio;
 		bio->bi_end_io = blkdev_bio_end_io;
 		bio->bi_ioprio = iocb->ki_ioprio;
@@ -333,6 +335,7 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 	dio->iocb = iocb;
 	bio->bi_iter.bi_sector = pos >> SECTOR_SHIFT;
 	bio->bi_write_hint = file_inode(iocb->ki_filp)->i_write_hint;
+	bio->bi_write_stream = iocb->ki_write_stream;
 	bio->bi_end_io = blkdev_bio_end_io_async;
 	bio->bi_ioprio = iocb->ki_ioprio;
 
@@ -398,6 +401,26 @@ static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	if (blkdev_dio_invalid(bdev, iocb, iter))
 		return -EINVAL;
 
+	if (iov_iter_rw(iter) == WRITE) {
+		u16 max_write_streams = bdev_max_write_streams(bdev);
+
+		if (iocb->ki_write_stream) {
+			if (iocb->ki_write_stream > max_write_streams)
+				return -EINVAL;
+		} else if (max_write_streams) {
+			enum rw_hint write_hint =
+				file_inode(iocb->ki_filp)->i_write_hint;
+
+			/*
+			 * Just use the write hint as write stream for block
+			 * device writes.  This assumes no file system is
+			 * mounted that would use the streams differently.
+			 */
+			if (write_hint <= max_write_streams)
+				iocb->ki_write_stream = write_hint;
+		}
+	}
+
 	nr_pages = bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS + 1);
 	if (likely(nr_pages <= BIO_MAX_VECS)) {
 		if (is_sync_kiocb(iocb))
-- 
2.43.5

[PATCHv13 06/11] io_uring: enable per-io write streams

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Allow userspace to pass a per-I/O write stream in the SQE:

      __u8 write_stream;

The __u8 type matches the size the filesystems and block layer support.

Application can query the supported values from the statx
max_write_streams field. Unsupported values are ignored by file
operations that do not support write streams or rejected with an error
by those that support them.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/uapi/linux/io_uring.h | 4 ++++
 io_uring/io_uring.c           | 2 ++
 io_uring/rw.c                 | 1 +
 3 files changed, 7 insertions(+)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 38f0d6b10eaf7..986a480e3b9c2 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -92,6 +92,10 @@ struct io_uring_sqe {
 			__u16	addr_len;
 			__u16	__pad3[1];
 		};
+		struct {
+			__u8	write_stream;
+			__u8	__pad4[3];
+		};
 	};
 	union {
 		struct {
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index ae36aa702f463..b561c5e8879ac 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3869,6 +3869,8 @@ static int __init io_uring_init(void)
 	BUILD_BUG_SQE_ELEM(44, __s32,  splice_fd_in);
 	BUILD_BUG_SQE_ELEM(44, __u32,  file_index);
 	BUILD_BUG_SQE_ELEM(44, __u16,  addr_len);
+	BUILD_BUG_SQE_ELEM(44, __u8,   write_stream);
+	BUILD_BUG_SQE_ELEM(45, __u8,   __pad4[0]);
 	BUILD_BUG_SQE_ELEM(46, __u16,  __pad3[0]);
 	BUILD_BUG_SQE_ELEM(48, __u64,  addr3);
 	BUILD_BUG_SQE_ELEM_SIZE(48, 0, cmd);
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 5b24fd8b69f62..416ccd89a77ed 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -316,6 +316,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	}
 	rw->kiocb.dio_complete = NULL;
 	rw->kiocb.ki_flags = 0;
+	rw->kiocb.ki_write_stream = READ_ONCE(sqe->write_stream);
 
 	rw->addr = READ_ONCE(sqe->addr);
 	rw->len = READ_ONCE(sqe->len);
-- 
2.43.5

[PATCHv13 10/11] nvme: register fdp parameters with the block layer

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Register the device data placement limits if supported. This is just
registering the limits with the block layer. Nothing beyond reporting
these attributes is happening in this patch.

Merges parts from a patch by Christoph Hellwig <hch@lst.de>
Link: https://lore.kernel.org/linux-nvme/20241119121632.1225556-15-hch@lst.de/ 

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 139 +++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |   2 +
 2 files changed, 141 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2a3585a3fa59..f7aeda601fcd6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -38,6 +38,8 @@ struct nvme_ns_info {
 	u32 nsid;
 	__le32 anagrpid;
 	u8 pi_offset;
+	u16 endgid;
+	u64 runs;
 	bool is_shared;
 	bool is_readonly;
 	bool is_ready;
@@ -1613,6 +1615,7 @@ static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
 	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
 	info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 	info->is_ready = true;
+	info->endgid = le16_to_cpu(id->endgid);
 	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
 		dev_info(ctrl->device,
 			 "Ignoring bogus Namespace Identifiers\n");
@@ -1653,6 +1656,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
 		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
 		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
 		info->no_vwc = id->nsfeat & NVME_NS_VWC_NOT_PRESENT;
+		info->endgid = le16_to_cpu(id->endgid);
 	}
 	kfree(id);
 	return ret;
@@ -2147,6 +2151,127 @@ static int nvme_update_ns_info_generic(struct nvme_ns *ns,
 	return ret;
 }
 
+static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
+				      struct nvme_ns_info *info, u8 fdp_idx)
+{
+	struct nvme_fdp_config_log hdr, *h;
+	struct nvme_fdp_config_desc *desc;
+	size_t size = sizeof(hdr);
+	int i, n, ret;
+	void *log;
+
+	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
+			       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
+	if (ret) {
+		dev_warn(ctrl->device,
+			 "FDP configs log header status:0x%x endgid:%x\n", ret,
+			 info->endgid);
+		return ret;
+	}
+
+	size = le32_to_cpu(hdr.sze);
+	h = kzalloc(size, GFP_KERNEL);
+	if (!h) {
+		dev_warn(ctrl->device,
+			 "failed to allocate %lu bytes for FDP config log\n",
+			 size);
+		return -ENOMEM;
+	}
+
+	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
+			       NVME_CSI_NVM, h, size, 0, info->endgid);
+	if (ret) {
+		dev_warn(ctrl->device,
+			 "FDP configs log status:0x%x endgid:%x\n", ret,
+			 info->endgid);
+		goto out;
+	}
+
+	n = le16_to_cpu(h->numfdpc) + 1;
+	if (fdp_idx > n) {
+		dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
+			 fdp_idx, n);
+		/* Proceed without registering FDP streams */
+		ret = 0;
+		goto out;
+	}
+
+	log = h + 1;
+	desc = log;
+	for (i = 0; i < fdp_idx; i++) {
+		log += le16_to_cpu(desc->dsze);
+		desc = log;
+	}
+
+	if (le32_to_cpu(desc->nrg) > 1) {
+		dev_warn(ctrl->device, "FDP NRG > 1 not supported\n");
+		ret = 0;
+		goto out;
+	}
+
+	info->runs = le64_to_cpu(desc->runs);
+out:
+	kfree(h);
+	return ret;
+}
+
+static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
+{
+	struct nvme_ns_head *head = ns->head;
+	struct nvme_ctrl *ctrl = ns->ctrl;
+	struct nvme_fdp_ruh_status *ruhs;
+	struct nvme_fdp_config fdp;
+	struct nvme_command c = {};
+	size_t size;
+	int ret;
+
+	/*
+	 * The FDP configuration is static for the lifetime of the namespace,
+	 * so return immediately if we've already registered this namespaces's
+	 * streams.
+	 */
+	if (head->nr_plids)
+		return 0;
+
+	ret = nvme_get_features(ctrl, NVME_FEAT_FDP, info->endgid, NULL, 0,
+				&fdp);
+	if (ret) {
+		dev_warn(ctrl->device, "FDP get feature status:0x%x\n", ret);
+		return ret;
+	}
+
+	if (!(fdp.flags & FDPCFG_FDPE))
+		return 0;
+
+	ret = nvme_query_fdp_granularity(ctrl, info, fdp.fdpcidx);
+	if (!info->runs)
+		return ret;
+
+	size = struct_size(ruhs, ruhsd, S8_MAX - 1);
+	ruhs = kzalloc(size, GFP_KERNEL);
+	if (!ruhs) {
+		dev_warn(ctrl->device,
+			 "failed to allocate %lu bytes for FDP io-mgmt\n",
+			 size);
+		return -ENOMEM;
+	}
+
+	c.imr.opcode = nvme_cmd_io_mgmt_recv;
+	c.imr.nsid = cpu_to_le32(head->ns_id);
+	c.imr.mo = NVME_IO_MGMT_RECV_MO_RUHS;
+	c.imr.numd = cpu_to_le32(nvme_bytes_to_numd(size));
+	ret = nvme_submit_sync_cmd(ns->queue, &c, ruhs, size);
+	if (ret) {
+		dev_warn(ctrl->device, "FDP io-mgmt status:0x%x\n", ret);
+		goto free;
+	}
+
+	head->nr_plids = le16_to_cpu(ruhs->nruhsd);
+free:
+	kfree(ruhs);
+	return ret;
+}
+
 static int nvme_update_ns_info_block(struct nvme_ns *ns,
 		struct nvme_ns_info *info)
 {
@@ -2183,6 +2308,12 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 			goto out;
 	}
 
+	if (ns->ctrl->ctratt & NVME_CTRL_ATTR_FDPS) {
+		ret = nvme_query_fdp_info(ns, info);
+		if (ret < 0)
+			goto out;
+	}
+
 	blk_mq_freeze_queue(ns->disk->queue);
 	ns->head->lba_shift = id->lbaf[lbaf].ds;
 	ns->head->nuse = le64_to_cpu(id->nuse);
@@ -2216,6 +2347,12 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	if (!nvme_init_integrity(ns->head, &lim, info))
 		capacity = 0;
 
+	lim.max_write_streams = ns->head->nr_plids;
+	if (lim.max_write_streams)
+		lim.write_stream_granularity = max(info->runs, U32_MAX);
+	else
+		lim.write_stream_granularity = 0;
+
 	ret = queue_limits_commit_update(ns->disk->queue, &lim);
 	if (ret) {
 		blk_mq_unfreeze_queue(ns->disk->queue);
@@ -2318,6 +2455,8 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 			ns->head->disk->flags |= GENHD_FL_HIDDEN;
 		else
 			nvme_init_integrity(ns->head, &lim, info);
+		lim.max_write_streams = ns_lim->max_write_streams;
+		lim.write_stream_granularity = ns_lim->write_stream_granularity;
 		ret = queue_limits_commit_update(ns->head->disk->queue, &lim);
 
 		set_capacity_and_notify(ns->head->disk, get_capacity(ns->disk));
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c1995d89ffdb8..4b412cd8001f1 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -491,6 +491,8 @@ struct nvme_ns_head {
 	struct device		cdev_device;
 
 	struct gendisk		*disk;
+
+	u16			nr_plids;
 #ifdef CONFIG_NVME_MULTIPATH
 	struct bio_list		requeue_list;
 	spinlock_t		requeue_lock;
-- 
2.43.5

Re: [PATCHv13 05/11] block: expose write streams for block device nodes

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On 10/12/24 11:47AM, Keith Busch wrote:
>From: Christoph Hellwig <hch@lst.de>
>
>Use the per-kiocb write stream if provided, or map temperature hints to
>write streams (which is a bit questionable, but this shows how it is
>done).
>
>Reviewed-by: Hannes Reinecke <hare@suse.de>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>[kbusch: removed statx reporting]
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 10/12/24 11:47AM, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Register the device data placement limits if supported. This is just
>registering the limits with the block layer. Nothing beyond reporting
>these attributes is happening in this patch.
>
>Merges parts from a patch by Christoph Hellwig <hch@lst.de>
>Link: https://lore.kernel.org/linux-nvme/20241119121632.1225556-15-hch@lst.de/
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/core.c | 139 +++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h |   2 +
> 2 files changed, 141 insertions(+)
>
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index c2a3585a3fa59..f7aeda601fcd6 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>+	/*
>+	 * The FDP configuration is static for the lifetime of the namespace,
>+	 * so return immediately if we've already registered this namespaces's
Nit: namespace's

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv13 06/11] io_uring: enable per-io write streams

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

On 10/12/24 11:47AM, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Allow userspace to pass a per-I/O write stream in the SQE:
>
>      __u8 write_stream;
>
>The __u8 type matches the size the filesystems and block layer support.
>
>Application can query the supported values from the statx
s/statx/sysfs

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv13 00/11] block write streams with nvme fdp

[Christoph Hellwig]

On Tue, Dec 10, 2024 at 11:47:11AM -0800, Keith Busch wrote:
> Changes from v12:

The changes looks good to me.  I'll ty to send out an API for querying
the paramters in the next so that we don't merge the granularity as dead
code, but I'll need a bit more time to also write proper tests and
stuff.  For that it probably makes sense to expose these streams using
null_blk so that we can actually have a block test.  If someone feels
like speeding things up I'd also be happy for someone else to take
that work.

Re: [PATCHv13 04/11] block: introduce a write_stream_granularity queue limit

[John Garry]

>   
>   	unsigned int		max_open_zones;
>   	unsigned int		max_active_zones;
> @@ -1249,6 +1250,12 @@ static inline unsigned short bdev_max_write_streams(struct block_device *bdev)
>   	return bdev_limits(bdev)->max_write_streams;
>   }
>   
> +static inline unsigned int
> +bdev_write_stream_granularity(struct block_device *bdev)

is this referenced anywhere?

I see that Nitesh mentioned something similar also previously

> +{
> +	return bdev_limits(bdev)->write_stream_granularity;
> +}
> +
>   static inline unsigned queue_logical_block_size(const struct request_queue *q)
>   {
>   	return q->limits.logical_block_size;

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[John Garry]

On 10/12/2024 19:47, Keith Busch wrote:
>   
> +static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
> +				      struct nvme_ns_info *info, u8 fdp_idx)
> +{
> +	struct nvme_fdp_config_log hdr, *h;
> +	struct nvme_fdp_config_desc *desc;
> +	size_t size = sizeof(hdr);
> +	int i, n, ret;
> +	void *log;
> +
> +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> +			       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "FDP configs log header status:0x%x endgid:%x\n", ret,
> +			 info->endgid);

About endgid, I guess that there is a good reason but sometimes "0x" is 
prefixed for hex prints and sometimes not. Maybe no prefix is used when 
we know that the variable is to hold a value from a HW register / memory 
structure - I don't know.

further nitpicking: And ret holds a kernel error code - the driver seems 
inconsistent for printing this. Sometimes it's %d and sometimes 0x%x.


> +		return ret;
> +	}
> +
> +	size = le32_to_cpu(hdr.sze);
> +	h = kzalloc(size, GFP_KERNEL);
> +	if (!h) {
> +		dev_warn(ctrl->device,
> +			 "failed to allocate %lu bytes for FDP config log\n",
> +			 size);

do we normally print ENOMEM messages? I see that the bytes is printed, 
but I assume that this is a sane value (of little note).

> +		return -ENOMEM;
> +	}
> +
> +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> +			       NVME_CSI_NVM, h, size, 0, info->endgid);
> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "FDP configs log status:0x%x endgid:%x\n", ret,
> +			 info->endgid);
> +		goto out;
> +	}
> +
> +	n = le16_to_cpu(h->numfdpc) + 1;
> +	if (fdp_idx > n) {
> +		dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
> +			 fdp_idx, n);
> +		/* Proceed without registering FDP streams */> +		ret = 0;

nit: maybe you want to be explicit, but logically ret is already 0

> +		goto out;
> +	}
> +
> +	log = h + 1;
> +	desc = log;
> +	for (i = 0; i < fdp_idx; i++) {
> +		log += le16_to_cpu(desc->dsze);
> +		desc = log;
> +	}
> +
> +	if (le32_to_cpu(desc->nrg) > 1) {
> +		dev_warn(ctrl->device, "FDP NRG > 1 not supported\n");
> +		ret = 0;

Same here

> +		goto out;
> +	}
> +
> +	info->runs = le64_to_cpu(desc->runs);
> +out:
> +	kfree(h);
> +	return ret;
> +}

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[Hannes Reinecke]

On 12/10/24 20:47, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Register the device data placement limits if supported. This is just
> registering the limits with the block layer. Nothing beyond reporting
> these attributes is happening in this patch.
> 
> Merges parts from a patch by Christoph Hellwig <hch@lst.de>
> Link: https://lore.kernel.org/linux-nvme/20241119121632.1225556-15-hch@lst.de/
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 139 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |   2 +
>   2 files changed, 141 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c2a3585a3fa59..f7aeda601fcd6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -38,6 +38,8 @@ struct nvme_ns_info {
>   	u32 nsid;
>   	__le32 anagrpid;
>   	u8 pi_offset;
> +	u16 endgid;
> +	u64 runs;
>   	bool is_shared;
>   	bool is_readonly;
>   	bool is_ready;
> @@ -1613,6 +1615,7 @@ static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
>   	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
>   	info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
>   	info->is_ready = true;
> +	info->endgid = le16_to_cpu(id->endgid);
>   	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
>   		dev_info(ctrl->device,
>   			 "Ignoring bogus Namespace Identifiers\n");
> @@ -1653,6 +1656,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
>   		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
>   		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
>   		info->no_vwc = id->nsfeat & NVME_NS_VWC_NOT_PRESENT;
> +		info->endgid = le16_to_cpu(id->endgid);
>   	}
>   	kfree(id);
>   	return ret;
> @@ -2147,6 +2151,127 @@ static int nvme_update_ns_info_generic(struct nvme_ns *ns,
>   	return ret;
>   }
>   
> +static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
> +				      struct nvme_ns_info *info, u8 fdp_idx)
> +{
> +	struct nvme_fdp_config_log hdr, *h;
> +	struct nvme_fdp_config_desc *desc;
> +	size_t size = sizeof(hdr);
> +	int i, n, ret;
> +	void *log;
> +
> +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> +			       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "FDP configs log header status:0x%x endgid:%x\n", ret,
> +			 info->endgid);
> +		return ret;
> +	}
> +
> +	size = le32_to_cpu(hdr.sze);

size should be bounded to avoid overly large memory allocations when the 
header is garbled.

> +	h = kzalloc(size, GFP_KERNEL);
> +	if (!h) {
> +		dev_warn(ctrl->device,
> +			 "failed to allocate %lu bytes for FDP config log\n",
> +			 size);
> +		return -ENOMEM;
> +	}
> +
> +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> +			       NVME_CSI_NVM, h, size, 0, info->endgid);
> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "FDP configs log status:0x%x endgid:%x\n", ret,
> +			 info->endgid);
> +		goto out;
> +	}
> +
> +	n = le16_to_cpu(h->numfdpc) + 1;
> +	if (fdp_idx > n) {
> +		dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
> +			 fdp_idx, n);
> +		/* Proceed without registering FDP streams */
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	log = h + 1;
> +	desc = log;
> +	for (i = 0; i < fdp_idx; i++) {
> +		log += le16_to_cpu(desc->dsze);
> +		desc = log;

Check for the size of 'h' to ensure that you are not
falling over the end ...


Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on next-20241211]
[cannot apply to brauner-vfs/vfs.all hch-configfs/for-next linus/master v6.13-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/fs-add-a-write-stream-field-to-the-kiocb/20241211-080803
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20241210194722.1905732-11-kbusch%40meta.com
patch subject: [PATCHv13 10/11] nvme: register fdp parameters with the block layer
config: i386-buildonly-randconfig-002-20241211 (https://download.01.org/0day-ci/archive/20241211/202412112244.XIKhzaWR-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241211/202412112244.XIKhzaWR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412112244.XIKhzaWR-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/async.h:14,
                    from drivers/nvme/host/core.c:7:
   drivers/nvme/host/core.c: In function 'nvme_query_fdp_granularity':
>> drivers/nvme/host/core.c:2176:26: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
    2176 |                          "failed to allocate %lu bytes for FDP config log\n",
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:156:61: note: in expansion of macro 'dev_fmt'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                             ^~~~~~~
   drivers/nvme/host/core.c:2175:17: note: in expansion of macro 'dev_warn'
    2175 |                 dev_warn(ctrl->device,
         |                 ^~~~~~~~
   drivers/nvme/host/core.c:2176:48: note: format string is defined here
    2176 |                          "failed to allocate %lu bytes for FDP config log\n",
         |                                              ~~^
         |                                                |
         |                                                long unsigned int
         |                                              %u
   In file included from include/linux/device.h:15,
                    from include/linux/async.h:14,
                    from drivers/nvme/host/core.c:7:
   drivers/nvme/host/core.c: In function 'nvme_query_fdp_info':
   drivers/nvme/host/core.c:2254:26: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
    2254 |                          "failed to allocate %lu bytes for FDP io-mgmt\n",
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:156:61: note: in expansion of macro 'dev_fmt'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                             ^~~~~~~
   drivers/nvme/host/core.c:2253:17: note: in expansion of macro 'dev_warn'
    2253 |                 dev_warn(ctrl->device,
         |                 ^~~~~~~~
   drivers/nvme/host/core.c:2254:48: note: format string is defined here
    2254 |                          "failed to allocate %lu bytes for FDP io-mgmt\n",
         |                                              ~~^
         |                                                |
         |                                                long unsigned int
         |                                              %u


vim +2176 drivers/nvme/host/core.c

  2153	
  2154	static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
  2155					      struct nvme_ns_info *info, u8 fdp_idx)
  2156	{
  2157		struct nvme_fdp_config_log hdr, *h;
  2158		struct nvme_fdp_config_desc *desc;
  2159		size_t size = sizeof(hdr);
  2160		int i, n, ret;
  2161		void *log;
  2162	
  2163		ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
  2164				       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
  2165		if (ret) {
  2166			dev_warn(ctrl->device,
  2167				 "FDP configs log header status:0x%x endgid:%x\n", ret,
  2168				 info->endgid);
  2169			return ret;
  2170		}
  2171	
  2172		size = le32_to_cpu(hdr.sze);
  2173		h = kzalloc(size, GFP_KERNEL);
  2174		if (!h) {
  2175			dev_warn(ctrl->device,
> 2176				 "failed to allocate %lu bytes for FDP config log\n",
  2177				 size);
  2178			return -ENOMEM;
  2179		}
  2180	
  2181		ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
  2182				       NVME_CSI_NVM, h, size, 0, info->endgid);
  2183		if (ret) {
  2184			dev_warn(ctrl->device,
  2185				 "FDP configs log status:0x%x endgid:%x\n", ret,
  2186				 info->endgid);
  2187			goto out;
  2188		}
  2189	
  2190		n = le16_to_cpu(h->numfdpc) + 1;
  2191		if (fdp_idx > n) {
  2192			dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
  2193				 fdp_idx, n);
  2194			/* Proceed without registering FDP streams */
  2195			ret = 0;
  2196			goto out;
  2197		}
  2198	
  2199		log = h + 1;
  2200		desc = log;
  2201		for (i = 0; i < fdp_idx; i++) {
  2202			log += le16_to_cpu(desc->dsze);
  2203			desc = log;
  2204		}
  2205	
  2206		if (le32_to_cpu(desc->nrg) > 1) {
  2207			dev_warn(ctrl->device, "FDP NRG > 1 not supported\n");
  2208			ret = 0;
  2209			goto out;
  2210		}
  2211	
  2212		info->runs = le64_to_cpu(desc->runs);
  2213	out:
  2214		kfree(h);
  2215		return ret;
  2216	}
  2217	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[Keith Busch]

On Wed, Dec 11, 2024 at 09:30:37AM +0000, John Garry wrote:
> On 10/12/2024 19:47, Keith Busch wrote:
> > +static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
> > +				      struct nvme_ns_info *info, u8 fdp_idx)
> > +{
> > +	struct nvme_fdp_config_log hdr, *h;
> > +	struct nvme_fdp_config_desc *desc;
> > +	size_t size = sizeof(hdr);
> > +	int i, n, ret;
> > +	void *log;
> > +
> > +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> > +			       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
> > +	if (ret) {
> > +		dev_warn(ctrl->device,
> > +			 "FDP configs log header status:0x%x endgid:%x\n", ret,
> > +			 info->endgid);
> 
> About endgid, I guess that there is a good reason but sometimes "0x" is
> prefixed for hex prints and sometimes not. Maybe no prefix is used when we
> know that the variable is to hold a value from a HW register / memory
> structure - I don't know.

%d for endgid is probably a better choice.
 
> further nitpicking: And ret holds a kernel error code - the driver seems
> inconsistent for printing this. Sometimes it's %d and sometimes 0x%x.

It's either an -errno or an nvme status. "%x" is easier to decode if
it's an nvme status, which is probably the more interesting case to
debug.
 
> > +		return ret;
> > +	}
> > +
> > +	size = le32_to_cpu(hdr.sze);
> > +	h = kzalloc(size, GFP_KERNEL);
> > +	if (!h) {
> > +		dev_warn(ctrl->device,
> > +			 "failed to allocate %lu bytes for FDP config log\n",
> > +			 size);
> 
> do we normally print ENOMEM messages? I see that the bytes is printed, but I
> assume that this is a sane value (of little note).

I suppose not.

> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> > +			       NVME_CSI_NVM, h, size, 0, info->endgid);
> > +	if (ret) {
> > +		dev_warn(ctrl->device,
> > +			 "FDP configs log status:0x%x endgid:%x\n", ret,
> > +			 info->endgid);
> > +		goto out;
> > +	}
> > +
> > +	n = le16_to_cpu(h->numfdpc) + 1;
> > +	if (fdp_idx > n) {
> > +		dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
> > +			 fdp_idx, n);
> > +		/* Proceed without registering FDP streams */> +		ret = 0;
> 
> nit: maybe you want to be explicit, but logically ret is already 0

Yeah, we know its zero already, but there are static analyisis tools
that think returning without setting an error return code was a mistake,
and that we really meant to return something else like -EINVAL. We
definitely want to return 0 here, so this setting exists only to prevent
future "help".

Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on next-20241211]
[cannot apply to brauner-vfs/vfs.all hch-configfs/for-next linus/master v6.13-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/fs-add-a-write-stream-field-to-the-kiocb/20241211-080803
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20241210194722.1905732-11-kbusch%40meta.com
patch subject: [PATCHv13 10/11] nvme: register fdp parameters with the block layer
config: arm-randconfig-003-20241211 (https://download.01.org/0day-ci/archive/20241212/202412120111.L2w9GCZd-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 2dc22615fd46ab2566d0f26d5ba234ab12dc4bf8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241212/202412120111.L2w9GCZd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412120111.L2w9GCZd-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/nvme/host/core.c:8:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/arm/include/asm/cacheflush.h:10:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/nvme/host/core.c:2177:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    2176 |                          "failed to allocate %lu bytes for FDP config log\n",
         |                                              ~~~
         |                                              %zu
    2177 |                          size);
         |                          ^~~~
   include/linux/dev_printk.h:156:70: note: expanded from macro 'dev_warn'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                     ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/nvme/host/core.c:2255:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    2254 |                          "failed to allocate %lu bytes for FDP io-mgmt\n",
         |                                              ~~~
         |                                              %zu
    2255 |                          size);
         |                          ^~~~
   include/linux/dev_printk.h:156:70: note: expanded from macro 'dev_warn'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                     ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   3 warnings generated.


vim +2177 drivers/nvme/host/core.c

  2153	
  2154	static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
  2155					      struct nvme_ns_info *info, u8 fdp_idx)
  2156	{
  2157		struct nvme_fdp_config_log hdr, *h;
  2158		struct nvme_fdp_config_desc *desc;
  2159		size_t size = sizeof(hdr);
  2160		int i, n, ret;
  2161		void *log;
  2162	
  2163		ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
  2164				       NVME_CSI_NVM, &hdr, size, 0, info->endgid);
  2165		if (ret) {
  2166			dev_warn(ctrl->device,
  2167				 "FDP configs log header status:0x%x endgid:%x\n", ret,
  2168				 info->endgid);
  2169			return ret;
  2170		}
  2171	
  2172		size = le32_to_cpu(hdr.sze);
  2173		h = kzalloc(size, GFP_KERNEL);
  2174		if (!h) {
  2175			dev_warn(ctrl->device,
  2176				 "failed to allocate %lu bytes for FDP config log\n",
> 2177				 size);
  2178			return -ENOMEM;
  2179		}
  2180	
  2181		ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
  2182				       NVME_CSI_NVM, h, size, 0, info->endgid);
  2183		if (ret) {
  2184			dev_warn(ctrl->device,
  2185				 "FDP configs log status:0x%x endgid:%x\n", ret,
  2186				 info->endgid);
  2187			goto out;
  2188		}
  2189	
  2190		n = le16_to_cpu(h->numfdpc) + 1;
  2191		if (fdp_idx > n) {
  2192			dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
  2193				 fdp_idx, n);
  2194			/* Proceed without registering FDP streams */
  2195			ret = 0;
  2196			goto out;
  2197		}
  2198	
  2199		log = h + 1;
  2200		desc = log;
  2201		for (i = 0; i < fdp_idx; i++) {
  2202			log += le16_to_cpu(desc->dsze);
  2203			desc = log;
  2204		}
  2205	
  2206		if (le32_to_cpu(desc->nrg) > 1) {
  2207			dev_warn(ctrl->device, "FDP NRG > 1 not supported\n");
  2208			ret = 0;
  2209			goto out;
  2210		}
  2211	
  2212		info->runs = le64_to_cpu(desc->runs);
  2213	out:
  2214		kfree(h);
  2215		return ret;
  2216	}
  2217	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCHv13 00/11] block write streams with nvme fdp

[Kanchan Joshi]

On 12/11/2024 12:43 PM, Christoph Hellwig wrote:
> The changes looks good to me.  I'll ty to send out an API for querying
> the paramters in the next so that we don't merge the granularity as dead
> code

What do you have in mind for this API. New fcntl or ioctl?
With ability limited to querying only or....

Re: [PATCHv13 00/11] block write streams with nvme fdp

[Christoph Hellwig]

On Thu, Dec 12, 2024 at 11:21:07AM +0530, Kanchan Joshi wrote:
> On 12/11/2024 12:43 PM, Christoph Hellwig wrote:
> > The changes looks good to me.  I'll ty to send out an API for querying
> > the paramters in the next so that we don't merge the granularity as dead
> > code
> 
> What do you have in mind for this API. New fcntl or ioctl?
> With ability limited to querying only or....

Yes, new fcntl or ioctl, query the number of streams and (nominal)
stream granularity as a start.  There is a few other things that
might be useful:

 - check if the size is just nominal or not, aka if the device can
   shorten it.  FDP currently allows for that, but given that
   notifications for that are out of bounds it makes a complete mess
   of software actually trying to use it for more than simple hot/cold
   separation like cachelib, so we find a way to query that in the
   spec.
 - reporting of the remaining capacity per stream, although I'm not
   sure if that should be in the same ioctl/fcntl, or better done
   using a separate interface that has the stream number as input
   and the capacity as out, which would seem a lot simpler