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

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

[Kanchan Joshi]

For
> 16 files changed, 341 insertions(+), 6 deletions(-)

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

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

[Hannes Reinecke]

On 12/11/24 19:35, 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.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 145 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |   2 +
>   2 files changed, 147 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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: [PATCHv14 10/11] nvme: register fdp parameters with the block layer

[Nitesh Shetty]

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

On 11/12/24 10:35AM, 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.
>
>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: [PATCHv14 10/11] nvme: register fdp parameters with the block layer

[Christoph Hellwig]

On Wed, Dec 11, 2024 at 10:35:13AM -0800, Keith Busch wrote:
> +	size = le32_to_cpu(hdr.sze);
> +	if (size > PAGE_SIZE * MAX_ORDER_NR_PAGES) {
> +		dev_warn(ctrl->device, "FDP config size too large:%zu\n",
> +			 size);
> +		return 0;
> +	h = vmalloc(size);
> +	if (!h)
> +		return -ENOMEM;

Isn't an unconditional vmalloc here for something that usually should
have less than a handful of descriptors a little aggressive?  I'd use
kvmalloc here to get the best of both worlds, and the free path seems
to already use kvfree anyway.

Otherwise the incremental changes vs the previous version for the entire
series look good to me.