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

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