[PATCH 1/2] nvme: refactor the atomic write unit detection

[John Garry]

On 11/06/2025 06:54, Christoph Hellwig wrote:
> Move all the code out of nvme_update_disk_info into the helper, and
> rename the helper to have a somewhat less clumsy name.

I suppose that the name nvme_update_atomic_write_disk_info just comes 
from extending the caller name, nvme_update_disk_info

> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>

Regardless of a couple of nits below:
Reviewed-by: John Garry <john.g.garry at oracle.com>

> ---
>   drivers/nvme/host/core.c | 72 +++++++++++++++++++++-------------------
>   1 file changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 724f5732786c..520fb5f1e214 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2015,21 +2015,51 @@ static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
>   }
>   
>   
> -static void nvme_update_atomic_write_disk_info(struct nvme_ns *ns,
> -			struct nvme_id_ns *id, struct queue_limits *lim,
> -			u32 bs, u32 atomic_bs)
> +static u32 nvme_configure_atomic_write(struct nvme_ns *ns,
> +		struct nvme_id_ns *id, struct queue_limits *lim, u32 bs)
>   {
> -	unsigned int boundary = 0;
> +	u32 atomic_bs, boundary = 0;
>   
> -	if (id->nsfeat & NVME_NS_FEAT_ATOMICS && id->nawupf) {
> -		if (le16_to_cpu(id->nabspf))
> +	/*
> +	 * We do not support an offset for the atomic boundaries.
> +	 */
> +	if (id->nabo)
> +		return bs;
> +
> +	if ((id->nsfeat & NVME_NS_FEAT_ATOMICS) && id->nawupf) {
> +		/*
> +		 * Use the per-namespace atomic write unit when available.
> +		 */
> +		atomic_bs = (1 + le16_to_cpu(id->nawupf)) * bs;
> +		if (id->nabspf)
>   			boundary = (le16_to_cpu(id->nabspf) + 1) * bs;
> +	} else {
> +		/*
> +		 * Use the controller wide atomic write unit.  This sucks

nit: I found the wording a bit ambiguous, as it sounds like the value in 
the controller-wide atomic write unit itself is somehow bad (which I 
assume is not the intention). How about rewording slightly:

Having to use this value sucks, as this limit is defined in terms of 
logical...

> +		 * because the limit is defined in terms of logical blocks while
> +		 * namespaces can have different formats, and because there is
> +		 * no clear language in the specification prohibiting different
> +		 * values for different controllers in the subsystem.
> +		 */
> +		atomic_bs = (1 + ns->ctrl->awupf) * bs;
> +	}
> +
> +	if (!ns->ctrl->subsys->atomic_bs) {
> +		ns->ctrl->subsys->atomic_bs = atomic_bs;
> +	} else if (ns->ctrl->subsys->atomic_bs != atomic_bs) {
> +		dev_err_ratelimited(ns->ctrl->device,
> +			"%s: Inconsistent Atomic Write Size, Namespace will not be added: Subsystem=%d bytes, Controller/Namespace=%d bytes\n",

nit on pre-existing code: /s/Atomic Write Size/Atomic Write Sizes/

And, again for pre-existing code, maybe stop using the camelcase