[PATCH 1/2] nvme: refactor the atomic write unit detection
John Garry
john.g.garry at oracle.com
Fri Jun 13 00:52:07 PDT 2025
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
More information about the Linux-nvme
mailing list