[PATCH v3 0/2] NVMe Atomic Write fixes

[PATCH v3 0/2] NVMe Atomic Write fixes

[Alan Adamson]

v3: 2/2 - Add comment.
        - Include "Namespace will not be added" to error message.

v2: 1/2 - Change wording of patch subject and description

    2/2 - Rather than setting a controllers atomic write size to 512b if it doesn't
          match the subsystem atomic write size, if a controllers atomic write size
          is greater than he subsystem atomic write size, reject to probe.


This patch set includes 2 fixes for NVMe Atomic Writes that are reproducable
when CMIC.MCTRS (multi-controller support) is set:

    PATCH 1/2 - nvme: multipath: enable BLK_FEAT_ATOMIC_WRITES for multipathing
    PATCH 2/2 - nvme: all namespaces in a subsystem must adhere to a common atomic write size

QEMU v10.0 can be used to reproduce and validate the fixes.


nvme: multipath: enable BLK_FEAT_ATOMIC_WRITES for multipathing
--------------------------------------------------------------------------
- Include BLK_FEAT_ATOMIC_WRITES feature when allocating multipath disk (nvme_mpath_alloc_disk).


QEMU Config
===========
-device nvme,id=nvme-ctrl-0,serial=nvme-1,atomic.dn=off,atomic.awun=31,atomic.awupf=15 \
    -drive file=/dev/nullb2,if=none,id=nvm-1 \
    -device nvme-ns,drive=nvm-1,bus=nvme-ctrl-0,nsid=1 \
-device nvme,id=nvme-ctrl-1,serial=nvme-2,atomic.dn=off,atomic.awun=31,atomic.awupf=7 \
    -drive file=/dev/nullb3,if=none,id=nvm-2 \
    -device nvme-ns,drive=nvm-2,bus=nvme-ctrl-1,nsid=2 \
-device nvme,id=nvme-ctrl-2,serial=nvme-3,atomic.dn=off,atomic.awun=127,atomic.awupf=63 \
    -drive file=/dev/nullb4,if=none,id=nvm-3 \
    -device nvme-ns,drive=nvm-3,bus=nvme-ctrl-2 \


Before
======
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep cmic
cmic      : 0x2
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
awupf     : 63
[root@localhost ~]# nvme id-ns /dev/nvme1n1 | grep nawupf
nawupf  : 0
[root@localhost ~]# cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
0
[root@localhost ~]# 

AFTER
=====
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep cmic
cmic      : 0x2
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
awupf     : 63
[root@localhost ~]# nvme id-ns /dev/nvme1n1 | grep nawupf
nawupf  : 0
[root@localhost ~]#  cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
32768
[root@localhost ~]# 






nvme: all namespaces in a subsystem must adhere to a common atomic write size
------------------------------------------------------------------------------
- Replace awupf field in nvme_subsystem struct with atomic_bs.  The atomic_bs 
value with awupf or nawupf. (nvme_update_disk_info)
- When a namespace is added, the atomic write size (from awupf or nawupf) is
checked with subsys->atomic_bs.  If they don't match, the atomic write size is
set to 512 (1U << head->lba_shift) and a message in logged. (nvme_update_disk_info)


QEMU Config
===========
-device nvme-subsys,id=subsys0 \
    -device nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 \
        -drive id=ns1,file=/dev/nullb1,if=none \
        -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,zoned=false,shared=false \
    -device nvme,serial=deadbeef,id=nvme1,subsys=subsys0,atomic.dn=off,atomic.awun=63,atomic.awupf=31 \
        -drive id=ns2,file=/dev/nullb2,if=none \
        -device nvme-ns,drive=ns2,bus=nvme1,nsid=2,zoned=false,shared=false \


BEFORE
======
[root@localhost ~]# nvme id-ctrl /dev/nvme0n1 | grep cmic
cmic      : 0x2
[root@localhost ~]#  nvme id-ctrl /dev/nvme0n1 | grep awupf
awupf     : 15
[root@localhost ~]# nvme id-ns /dev/nvme0n1 | grep nawupf
nawupf  : 0
[root@localhost ~]# cat /sys/block/nvme0n1/queue/atomic_write_max_bytes
8192
[root@localhost ~]# nvme id-ctrl /dev/nvme0n2 | grep cmic
cmic      : 0x2
[root@localhost ~]# nvme id-ctrl /dev/nvme0n2 | grep awupf
awupf     : 31
[root@localhost ~]# nvme id-ns /dev/nvme0n2 | grep nawupf
nawupf  : 0
[root@localhost ~]# cat /sys/block/nvme0n2/queue/atomic_write_max_bytes
8192
[root@localhost ~]# 

AFTER
=====
[root@localhost ~]# lsblk
NAME        MAJ:MIN RM  SIZE RO TYPE MOUNTPOINTS
sda           8:0    0   40G  0 disk 
├─sda1        8:1    0    1G  0 part /boot
└─sda2        8:2    0   39G  0 part 
  └─ol-root 252:0    0   39G  0 lvm  /
sr0          11:0    1 1024M  0 rom  
nvme1n2     259:1    0  250G  0 disk 
[root@localhost ~]# nvme id-ctrl /dev/nvme0n1 | grep cmic
cmic      : 0x2
[root@localhost ~]# nvme id-ctrl /dev/nvme0n1 | grep awupf
awupf     : 15
[root@localhost ~]# nvme id-ns /dev/nvme0n1 | grep nawupf
nawupf  : 0
[root@localhost ~]# cat /sys/block/nvme0n1/queue/atomic_write_max_bytes
8192

Console output:
[    2.848854] nvme0c1n2: Inconsistent Atomic Write Size, Namespace will not be added: Subsystem=8192 bytes, Controller/Namespace=16384 bytes

-- 
2.43.5

[PATCH v3 1/2] nvme: multipath: enable BLK_FEAT_ATOMIC_WRITES for multipathing

[Alan Adamson]

A change to QEMU resulted in all nvme controllers (single and
multi-controller subsystems) to have its CMIC.MCTRS bit set which indicates
the subsystem supports multiple controllers and it is possible a namespace
can be shared between those multiple controllers in a multipathed
configuration.

When a namespace of a CMIC.MCTRS enabled subsystem is allocated, a
multipath node is created.  The queue limits for this node are inherited
from the namespace being allocated. When inheriting queue limits, the
features being inherited need to be specified. The atomic write feature
(BLK_FEAT_ATOMIC_WRITES) was not specified so the atomic queue limits
were not inherited by the multipath disk node which resulted in the sysfs
atomic write attributes being zeroed. The fix is to include
BLK_FEAT_ATOMIC_WRITES in the list of features to be inherited.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 drivers/nvme/host/multipath.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 250f3da67cc9..cf0ef4745564 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -638,7 +638,8 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 
 	blk_set_stacking_limits(&lim);
 	lim.dma_alignment = 3;
-	lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT | BLK_FEAT_POLL;
+	lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT |
+		BLK_FEAT_POLL | BLK_FEAT_ATOMIC_WRITES;
 	if (head->ids.csi == NVME_CSI_ZNS)
 		lim.features |= BLK_FEAT_ZONED;
 
-- 
2.43.5

Re: [PATCH v3 1/2] nvme: multipath: enable BLK_FEAT_ATOMIC_WRITES for multipathing

[Chaitanya Kulkarni]

On 5/8/25 15:38, Alan Adamson wrote:
> A change to QEMU resulted in all nvme controllers (single and
> multi-controller subsystems) to have its CMIC.MCTRS bit set which indicates
> the subsystem supports multiple controllers and it is possible a namespace
> can be shared between those multiple controllers in a multipathed
> configuration.
>
> When a namespace of a CMIC.MCTRS enabled subsystem is allocated, a
> multipath node is created.  The queue limits for this node are inherited
> from the namespace being allocated. When inheriting queue limits, the
> features being inherited need to be specified. The atomic write feature
> (BLK_FEAT_ATOMIC_WRITES) was not specified so the atomic queue limits
> were not inherited by the multipath disk node which resulted in the sysfs
> atomic write attributes being zeroed. The fix is to include
> BLK_FEAT_ATOMIC_WRITES in the list of features to be inherited.
>
> Signed-off-by: Alan Adamson<alan.adamson@oracle.com>
> Reviewed-by: John Garry<john.g.garry@oracle.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[Alan Adamson]

The first namespace configured in a subsystem sets the subsystem's
atomic write size based on its AWUPF or NAWUPF. Subsequent namespaces
must have an atomic write size (per their AWUPF or NAWUPF) less than or
equal to the subsystem's atomic write size, or their probing will be
rejected.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 drivers/nvme/host/core.c | 29 ++++++++++++++++++++++++++---
 drivers/nvme/host/nvme.h |  3 ++-
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eb6ea8acb3cc..f34cf69fab8d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2059,7 +2059,20 @@ static bool nvme_update_disk_info(struct nvme_ns *ns, struct nvme_id_ns *id,
 		if (id->nsfeat & NVME_NS_FEAT_ATOMICS && id->nawupf)
 			atomic_bs = (1 + le16_to_cpu(id->nawupf)) * bs;
 		else
-			atomic_bs = (1 + ns->ctrl->subsys->awupf) * bs;
+			atomic_bs = (1 + ns->ctrl->awupf) * bs;
+
+		/*
+		 * Set subsystem atomic bs.
+		 */
+		if (ns->ctrl->subsys->atomic_bs) {
+			if (atomic_bs > ns->ctrl->subsys->atomic_bs) {
+				pr_err_ratelimited("%s: Inconsistent Atomic Write Size, Namespace will not be added: Subsystem=%d bytes, Controller/Namespace=%d bytes\n",
+					ns->disk ? ns->disk->disk_name : "?",
+					ns->ctrl->subsys->atomic_bs,
+					atomic_bs);
+			}
+		} else
+			ns->ctrl->subsys->atomic_bs = atomic_bs;
 
 		nvme_update_atomic_write_disk_info(ns, id, lim, bs, atomic_bs);
 	}
@@ -2201,6 +2214,17 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	nvme_set_chunk_sectors(ns, id, &lim);
 	if (!nvme_update_disk_info(ns, id, &lim))
 		capacity = 0;
+
+	/*
+	 * Validate the max atomic write size fits within the subsystem's
+	 * atomic write capabilities.
+	 */
+	if (lim.atomic_write_hw_max > ns->ctrl->subsys->atomic_bs) {
+		blk_mq_unfreeze_queue(ns->disk->queue, memflags);
+		ret = -ENXIO;
+		goto out;
+	}
+
 	nvme_config_discard(ns, &lim);
 	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
 	    ns->head->ids.csi == NVME_CSI_ZNS)
@@ -3031,7 +3055,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		kfree(subsys);
 		return -EINVAL;
 	}
-	subsys->awupf = le16_to_cpu(id->awupf);
 	nvme_mpath_default_iopolicy(subsys);
 
 	subsys->dev.class = &nvme_subsys_class;
@@ -3441,7 +3464,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		dev_pm_qos_expose_latency_tolerance(ctrl->device);
 	else if (!ctrl->apst_enabled && prev_apst_enabled)
 		dev_pm_qos_hide_latency_tolerance(ctrl->device);
-
+	ctrl->awupf = le16_to_cpu(id->awupf);
 out_free:
 	kfree(id);
 	return ret;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 51e078642127..ff1d94468613 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -410,6 +410,7 @@ struct nvme_ctrl {
 
 	enum nvme_ctrl_type cntrltype;
 	enum nvme_dctype dctype;
+	u16 awupf;
 };
 
 static inline enum nvme_ctrl_state nvme_ctrl_state(struct nvme_ctrl *ctrl)
@@ -442,11 +443,11 @@ struct nvme_subsystem {
 	u8			cmic;
 	enum nvme_subsys_type	subtype;
 	u16			vendor_id;
-	u16			awupf;	/* 0's based awupf value. */
 	struct ida		ns_ida;
 #ifdef CONFIG_NVME_MULTIPATH
 	enum nvme_iopolicy	iopolicy;
 #endif
+	u32			atomic_bs;
 };
 
 /*
-- 
2.43.5

Re: [PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[John Garry]

On 08/05/2025 23:38, Alan Adamson wrote:

Sorry for slow response. Generally I think that this is ok, but some 
minor comments below. I might be off the mark with some of them.

> The first namespace configured in a subsystem sets the subsystem's
> atomic write size based on its AWUPF or NAWUPF. Subsequent namespaces
> must have an atomic write size (per their AWUPF or NAWUPF) less than or
> equal to the subsystem's atomic write size, or their probing will be
> rejected.
> 
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---
>   drivers/nvme/host/core.c | 29 ++++++++++++++++++++++++++---
>   drivers/nvme/host/nvme.h |  3 ++-
>   2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index eb6ea8acb3cc..f34cf69fab8d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2059,7 +2059,20 @@ static bool nvme_update_disk_info(struct nvme_ns *ns, struct nvme_id_ns *id,
>   		if (id->nsfeat & NVME_NS_FEAT_ATOMICS && id->nawupf)
>   			atomic_bs = (1 + le16_to_cpu(id->nawupf)) * bs;
>   		else
> -			atomic_bs = (1 + ns->ctrl->subsys->awupf) * bs;
> +			atomic_bs = (1 + ns->ctrl->awupf) * bs;
> +
> +		/*
> +		 * Set subsystem atomic bs.
> +		 */
> +		if (ns->ctrl->subsys->atomic_bs) {
> +			if (atomic_bs > ns->ctrl->subsys->atomic_bs) {

I am not sure why use > and not !=

> +				pr_err_ratelimited(

could dev_err() be used?

"%s: Inconsistent Atomic Write Size,

we could mention that this is the powerfail atomic size, and not 
"normal". But I suppose that we don't use "normal" values anywhere, so 
it could be assumed.

> Namespace will not be added: Subsystem=%d bytes, Controller/Namespace=%d bytes\n",
> +					ns->disk ? ns->disk->disk_name : "?",
> +					ns->ctrl->subsys->atomic_bs,
> +					atomic_bs);
> +			}
> +		} else
> +			ns->ctrl->subsys->atomic_bs = atomic_bs;
>   
>   		nvme_update_atomic_write_disk_info(ns, id, lim, bs, atomic_bs);
>   	}
> @@ -2201,6 +2214,17 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   	nvme_set_chunk_sectors(ns, id, &lim);
>   	if (!nvme_update_disk_info(ns, id, &lim))
>   		capacity = 0;
> +
> +	/*
> +	 * Validate the max atomic write size fits within the subsystem's
> +	 * atomic write capabilities.
> +	 */
> +	if (lim.atomic_write_hw_max > ns->ctrl->subsys->atomic_bs) {
 > +		blk_mq_unfreeze_queue(ns->disk->queue, memflags);> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
>   	nvme_config_discard(ns, &lim);
>   	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>   	    ns->head->ids.csi == NVME_CSI_ZNS)
> @@ -3031,7 +3055,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   		kfree(subsys);
>   		return -EINVAL;
>   	}
> -	subsys->awupf = le16_to_cpu(id->awupf);
>   	nvme_mpath_default_iopolicy(subsys);
>   
>   	subsys->dev.class = &nvme_subsys_class;
> @@ -3441,7 +3464,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>   		dev_pm_qos_expose_latency_tolerance(ctrl->device);
>   	else if (!ctrl->apst_enabled && prev_apst_enabled)
>   		dev_pm_qos_hide_latency_tolerance(ctrl->device);
> -
> +	ctrl->awupf = le16_to_cpu(id->awupf);
>   out_free:
>   	kfree(id);
>   	return ret;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 51e078642127..ff1d94468613 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -410,6 +410,7 @@ struct nvme_ctrl {
>   
>   	enum nvme_ctrl_type cntrltype;
>   	enum nvme_dctype dctype;
> +	u16 awupf;

was it intentional to lose the " 0's based awupf value" comment?

>   };
>   
>   static inline enum nvme_ctrl_state nvme_ctrl_state(struct nvme_ctrl *ctrl)
> @@ -442,11 +443,11 @@ struct nvme_subsystem {
>   	u8			cmic;
>   	enum nvme_subsys_type	subtype;
>   	u16			vendor_id;
> -	u16			awupf;	/* 0's based awupf value. */
>   	struct ida		ns_ida;
>   #ifdef CONFIG_NVME_MULTIPATH
>   	enum nvme_iopolicy	iopolicy;
>   #endif
> +	u32			atomic_bs;
>   };
>   
>   /*

Re: [PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[Christoph Hellwig]

On Tue, May 13, 2025 at 10:45:33AM +0100, John Garry wrote:
>> +		 * Set subsystem atomic bs.
>> +		 */
>> +		if (ns->ctrl->subsys->atomic_bs) {
>> +			if (atomic_bs > ns->ctrl->subsys->atomic_bs) {
>
> I am not sure why use > and not !=
>
>> +				pr_err_ratelimited(
>
> could dev_err() be used?

Yes and yes.

>
> "%s: Inconsistent Atomic Write Size,
>
> we could mention that this is the powerfail atomic size, and not "normal". 
> But I suppose that we don't use "normal" values anywhere, so it could be 
> assumed.

I'll keep it as-is, the message is already borderling too long as is.

>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -410,6 +410,7 @@ struct nvme_ctrl {
>>     	enum nvme_ctrl_type cntrltype;
>>   	enum nvme_dctype dctype;
>> +	u16 awupf;
>
> was it intentional to lose the " 0's based awupf value" comment?

I'll add it back.

Can I get a formal review tag after this?

Re: [PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[John Garry]

On 14/05/2025 06:35, Christoph Hellwig wrote:
>> I am not sure why use > and not !=
>>
>>> +				pr_err_ratelimited(
>> could dev_err() be used?
> Yes and yes.
> 
>> "%s: Inconsistent Atomic Write Size,
>>
>> we could mention that this is the powerfail atomic size, and not "normal".
>> But I suppose that we don't use "normal" values anywhere, so it could be
>> assumed.
> I'll keep it as-is, the message is already borderling too long as is.
> 
>>> --- a/drivers/nvme/host/nvme.h
>>> +++ b/drivers/nvme/host/nvme.h
>>> @@ -410,6 +410,7 @@ struct nvme_ctrl {
>>>      	enum nvme_ctrl_type cntrltype;
>>>    	enum nvme_dctype dctype;
>>> +	u16 awupf;
>> was it intentional to lose the " 0's based awupf value" comment?
> I'll add it back.
> 
> Can I get a formal review tag after this?

ok

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[Christoph Hellwig]

Thanks!

Re: [PATCH v3 2/2] nvme: all namespaces in a subsystem must adhere to a common atomic write size

[Chaitanya Kulkarni]

On 5/8/25 15:38, Alan Adamson wrote:
> The first namespace configured in a subsystem sets the subsystem's
> atomic write size based on its AWUPF or NAWUPF. Subsequent namespaces
> must have an atomic write size (per their AWUPF or NAWUPF) less than or
> equal to the subsystem's atomic write size, or their probing will be
> rejected.
>
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---
>

with that are posted on this patch :-

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v3 0/2] NVMe Atomic Write fixes

[Christoph Hellwig]

Thanks!

While I'd like to have more review I've added this to nvme-6.15
for now to get some buildbot exposure.  More reviews would still be
very welcome.