Issue with AWUPF when using multiple controllers in a subsystem

Issue with AWUPF when using multiple controllers in a subsystem

[alan.adamson]

While testing atomic write parameters with multiple controllers within a 
subsystem, I found an issue:

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

[root@localhost ~]# nvme id-ctrl /dev/nvme1n2 | grep awupf
awupf     : 63
[root@localhost ~]# cat /sys/block/nvme1n2/queue/atomic_write_max_bytes
32768
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
awupf     : 15
[root@localhost ~]# cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
32768
[root@localhost ~]#

When dumping the awupf value with nvme-cli, the values match what was 
setup in qemu, but when each devices atomic queue limits was displayed, 
it didn't match its awupf. Currently the awupf is saved in the 
nvme_subsystem, but the awupf is specific to the controller, not the 
subsystem. Two controllers in a subsystem can have different atomic 
parameters.

The commit 81adb8633491 ("nvme: set physical block size and optimal I/O 
size") added subsys->awupf.

I think the awupf belongs in the nvme_ctrl structure.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a8259e3334ff..2483651817fb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2059,7 +2059,7 @@ 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;

                 nvme_update_atomic_write_disk_info(ns, id, lim, bs, 
atomic_bs);
         }
@@ -3032,7 +3032,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;
@@ -3443,6 +3442,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
         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 7be92d07430e..fddcd2de218c 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;      /* 0's based awupf value. */
  };

  static inline enum nvme_ctrl_state nvme_ctrl_state(struct nvme_ctrl *ctrl)
@@ -442,7 +443,6 @@ 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;


When I apply this fix, the problem is resolved.

[root@localhost ~]# nvme id-ctrl /dev/nvme1n2 | grep awupf
awupf     : 63
[root@localhost ~]# cat /sys/block/nvme1n2/queue/atomic_write_max_bytes
32768
[root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
awupf     : 15
[root@localhost ~]#  cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
8192
[root@localhost ~]#


Does this fix make sense?

Alan Adamson

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Chaitanya Kulkarni]

On 4/4/25 15:42, alan.adamson@oracle.com wrote:
> While testing atomic write parameters with multiple controllers within 
> a subsystem, I found an issue:
>
> QEMU-NVME Configuartion
> =================
> -device nvme-subsys,id=subsys0 \
> -device 
> nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 
> \
> -device 
> nvme,serial=deadbeef,id=nvme1,subsys=subsys0,atomic.dn=off,atomic.awun=127,atomic.awupf=63 
> \
> -drive id=ns1,file=/dev/nullb3,if=none \
> -drive id=ns2,file=/dev/nullb2,if=none \
> -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,shared=false \
> -device nvme-ns,drive=ns2,bus=nvme1,nsid=2,shared=false \
>
> [root@localhost ~]# nvme id-ctrl /dev/nvme1n2 | grep awupf
> awupf     : 63
> [root@localhost ~]# cat /sys/block/nvme1n2/queue/atomic_write_max_bytes
> 32768
> [root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
> awupf     : 15
> [root@localhost ~]# cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
> 32768
> [root@localhost ~]#
>
> When dumping the awupf value with nvme-cli, the values match what was 
> setup in qemu, but when each devices atomic queue limits was 
> displayed, it didn't match its awupf. Currently the awupf is saved in 
> the nvme_subsystem, but the awupf is specific to the controller, not 
> the subsystem. Two controllers in a subsystem can have different 
> atomic parameters.
>
> The commit 81adb8633491 ("nvme: set physical block size and optimal 
> I/O size") added subsys->awupf.
>
> I think the awupf belongs in the nvme_ctrl structure. 

AWUPF is a part of Identify controller data structure so Ideally
it should be part of the controller attribute.

 From NVM Express® Base Specification, Revision 2.2, March 11th,
2025 Page 319 :-

Figure 313: Identify – Identify Controller Data Structure,
I/O Command Set Independent

529:528

Atomic Write Unit Power Fail (AWUPF): This field is specific
to namespaces that are associated with command sets that
specify logical blocks (i.e., Command Set Identifier 0h or 2h),
and shall be cleared to 0h for namespaces that are not associated
with command sets that specify logical blocks. Refer to the
applicable I/O Command Set specification (e.g., the Atomic Operation
section of the NVM Command Set Specification).

-ck

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Christoph Hellwig]

On Tue, Apr 08, 2025 at 09:08:18PM +0000, Chaitanya Kulkarni wrote:
> >
> > I think the awupf belongs in the nvme_ctrl structure. 
> 
> AWUPF is a part of Identify controller data structure so Ideally
> it should be part of the controller attribute.

It is subsystem scoped really, and controllers should not differ
in the values as that would break multipath setups.

Unfortunately nvme doesn't have an identify subsystem and generally
is extremely sloppy about scopes.

Re: Issue with AWUPF when using multiple controllers in a subsystem

[John Garry]

On 09/04/2025 10:01, Christoph Hellwig wrote:
> On Tue, Apr 08, 2025 at 09:08:18PM +0000, Chaitanya Kulkarni wrote:
>>>
>>> I think the awupf belongs in the nvme_ctrl structure.
>>
>> AWUPF is a part of Identify controller data structure so Ideally
>> it should be part of the controller attribute.
> 
> It is subsystem scoped really, and controllers should not differ
> in the values as that would break multipath setups.
> 
> Unfortunately nvme doesn't have an identify subsystem and generally
> is extremely sloppy about scopes.
> 
> 

This, combined with no dedicated NVMe command to issue a write 
atomically (which could error for out-of-limits size/crossing boundary), 
is pretty concerning.

As an aside, I found the scope of the boundary definition to be quite 
vague as well.

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Christoph Hellwig]

On Thu, Apr 10, 2025 at 10:09:53AM +0100, John Garry wrote:
> This, combined with no dedicated NVMe command to issue a write atomically
> (which could error for out-of-limits size/crossing boundary), is pretty
> concerning.

Agreed.  Given that Oracle is actually a major user of NVMe, can you
bring that to the working group's attention?  That works much better
than a random Linux maintainer.

> As an aside, I found the scope of the boundary definition to be quite vague
> as well.

It used to be really horrible, but got a major rework for multiple
atomicy in NVMe 2.2 (or was it 2.1?).  Which version did you check?

Re: Issue with AWUPF when using multiple controllers in a subsystem

[John Garry]

On 10/04/2025 10:19, Christoph Hellwig wrote:
> On Thu, Apr 10, 2025 at 10:09:53AM +0100, John Garry wrote:
>> This, combined with no dedicated NVMe command to issue a write atomically
>> (which could error for out-of-limits size/crossing boundary), is pretty
>> concerning.
> 
> Agreed.  Given that Oracle is actually a major user of NVMe, can you
> bring that to the working group's attention?  That works much better
> than a random Linux maintainer.

Sure, I'll ask someone.

> 
>> As an aside, I found the scope of the boundary definition to be quite vague
>> as well.
> 
> It used to be really horrible, but got a major rework for multiple
> atomicy in NVMe 2.2 (or was it 2.1?). 

I am not really talking about multiple atomicy here, but something more 
basic...

> Which version did you check?

I am specifically referring to NSFEAT.NSABP, which is now described in 
NVM Command Set Specification 1.1.

I just found it odd that NSABP is related to boundary, but describes 
whether NAWUN et al fields are used or not (but not whether NABSPF is 
valid). Having said that, I'd think that NABSPF is always valid from 
2.1.4.4 Atomic Boundaries "The namespace supports Atomic Boundaries if
NABSN or NABSPF are set to non-zero values".

But from "Figure 4: Atomicity Parameters for Single Atomicity Mode", 
"Namespace Atomic Boundary Parameters" cell, it tells to refer to 
"Identify Namespace data structure" which describes NSFEAT.NSABP; 
however it does also describe NABSPF.

I am getting the feeling that the kernel driver should not check 
NSFEAT.NSABP on whether NABSPF is valid (which it does today).

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Chaitanya Kulkarni]

On 4/4/25 15:42, alan.adamson@oracle.com wrote:
> While testing atomic write parameters with multiple controllers within 
> a subsystem, I found an issue:
>
> QEMU-NVME Configuartion
> =================
> -device nvme-subsys,id=subsys0 \
> -device 
> nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 
> \
> -device 
> nvme,serial=deadbeef,id=nvme1,subsys=subsys0,atomic.dn=off,atomic.awun=127,atomic.awupf=63 
> \
> -drive id=ns1,file=/dev/nullb3,if=none \
> -drive id=ns2,file=/dev/nullb2,if=none \
> -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,shared=false \
> -device nvme-ns,drive=ns2,bus=nvme1,nsid=2,shared=false \
>
> [root@localhost ~]# nvme id-ctrl /dev/nvme1n2 | grep awupf
> awupf     : 63
> [root@localhost ~]# cat /sys/block/nvme1n2/queue/atomic_write_max_bytes
> 32768
> [root@localhost ~]# nvme id-ctrl /dev/nvme1n1 | grep awupf
> awupf     : 15
> [root@localhost ~]# cat /sys/block/nvme1n1/queue/atomic_write_max_bytes
> 32768
> [root@localhost ~]#
>
> When dumping the awupf value with nvme-cli, the values match what was 
> setup in qemu, but when each devices atomic queue limits was 
> displayed, it didn't match its awupf. Currently the awupf is saved in 
> the nvme_subsystem, but the awupf is specific to the controller, not 
> the subsystem. Two controllers in a subsystem can have different 
> atomic parameters.
>
> The commit 81adb8633491 ("nvme: set physical block size and optimal 
> I/O size") added subsys->awupf.
>
> I think the awupf belongs in the nvme_ctrl structure. 

AWUPF is a part of Identify controller data structure so Ideally
it should be part of the controller attribute.

 From NVM Express® Base Specification, Revision 2.2, March 11th,
2025 Page 319 :-

Figure 313: Identify – Identify Controller Data Structure,
I/O Command Set Independent

529:528

Atomic Write Unit Power Fail (AWUPF): This field is specific
to namespaces that are associated with command sets that
specify logical blocks (i.e., Command Set Identifier 0h or 2h),
and shall be cleared to 0h for namespaces that are not associated
with command sets that specify logical blocks. Refer to the
applicable I/O Command Set specification (e.g., the Atomic Operation
section of the NVM Command Set Specification).

-ck

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Christoph Hellwig]

On Fri, Apr 04, 2025 at 03:42:50PM -0700, alan.adamson@oracle.com wrote:
> When dumping the awupf value with nvme-cli, the values match what was setup
> in qemu, but when each devices atomic queue limits was displayed, it didn't
> match its awupf. Currently the awupf is saved in the nvme_subsystem, but the
> awupf is specific to the controller, not the subsystem. Two controllers in a
> subsystem can have different atomic parameters.
> 
> The commit 81adb8633491 ("nvme: set physical block size and optimal I/O
> size") added subsys->awupf.
> 
> I think the awupf belongs in the nvme_ctrl structure.

No, that would be extremely dangerous.  But as NVMe has not explicit
language to forbid different values for different controllers (even
if that is really stupid) we'll need to reject adding controllers
that have lower than the initial value to not break setups.

Sigh..  I'll also reach out to NVMe to see if we can ECN this, but
we'll still need to work around this.

Alternatively we could stop supporting AWUPF entirely and require
NAWUPF which must be the same for all attachments of the controller.

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Chaitanya Kulkarni]

On 4/9/25 02:05, Christoph Hellwig wrote:
> On Fri, Apr 04, 2025 at 03:42:50PM -0700, alan.adamson@oracle.com wrote:
>> When dumping the awupf value with nvme-cli, the values match what was setup
>> in qemu, but when each devices atomic queue limits was displayed, it didn't
>> match its awupf. Currently the awupf is saved in the nvme_subsystem, but the
>> awupf is specific to the controller, not the subsystem. Two controllers in a
>> subsystem can have different atomic parameters.
>>
>> The commit 81adb8633491 ("nvme: set physical block size and optimal I/O
>> size") added subsys->awupf.
>>
>> I think the awupf belongs in the nvme_ctrl structure.
> No, that would be extremely dangerous.  But as NVMe has not explicit
> language to forbid different values for different controllers (even
> if that is really stupid) we'll need to reject adding controllers
> that have lower than the initial value to not break setups.
>
> Sigh..  I'll also reach out to NVMe to see if we can ECN this, but
> we'll still need to work around this.

I wonder how many of these fields suffering from similar scope issue,
hope we could fix that in one ECN if there other fields suffering from
this issues.

> Alternatively we could stop supporting AWUPF entirely and require
> NAWUPF which must be the same for all attachments of the controller.
>
>

if we stop supporting AWUPF and move to NAWUPF then will it break where
exiting controller's that doesn't support NAWUPF but only supports
controller wide AWUPF, IOW NSFEAT bit 1 == 0 meaning per-namespace
atomic info is not valid ? [1]

Looking at the spec NAWUPF values can differ for namespaces attached
to the same controller and those values can also differ from
AWUPF ? [2]

I couldn't find anything where it states these values must be the same.

Perhaps using the minimum of all AWUPF and rejecting any ctrl which are
lower than the initial value is right fix ?

-ck

[1]

nvme_update_disk_info()

...

2056         /*
2057          * Bit 1 indicates whether NAWUPF is defined for this namespace
2058          * and whether it should be used instead of AWUPF. If NAWUPF ==
2059          * 0 then AWUPF must be used instead.
2060          */
2061         if (id->nsfeat & NVME_NS_FEAT_ATOMICS && id->nawupf)
2062                 atomic_bs = (1 + le16_to_cpu(id->nawupf)) * bs;
2063         else
2064                 atomic_bs = (1 + ns->ctrl->subsys->awupf) * bs;
2065

...

[2]

NVM Express NVM Command Set Specification, Revision 1.1 (2024.08.05)
Section: |2.1.4.3 AWUPF/NAWUPF*:-
*
|An NVM subsystem may report per namespace values for
these atomicity parameters that are specific to the namespace and
are indicated in the Identify Namespace data structure..."

"... If an NVM subsystem reports a per namespace value, then that
value shall be *greater than or equal to the corresponding baseline
value* indicated in the Identify Controller data structure
data structure (refer to Figure 117)"/

/My interpretation :-

The spec *allows different values* for:
|AWUPF| (controller-level baseline)
|NAWUPF| (namespace-specific override)
The only requirement is:
|||NAWUPF ≥ AWUPF
|There is *no mandate* that |NAWUPF == AWUPF


|

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Christoph Hellwig]

On Thu, Apr 10, 2025 at 01:33:38AM +0000, Chaitanya Kulkarni wrote:
> if we stop supporting AWUPF and move to NAWUPF then will it break where
> exiting controller's that doesn't support NAWUPF but only supports
> controller wide AWUPF, IOW NSFEAT bit 1 == 0 meaning per-namespace
> atomic info is not valid ? [1]

Yes.

> Looking at the spec NAWUPF values can differ for namespaces attached
> to the same controller and those values can also differ from
> AWUPF ? [2]

That's the whole point of NAWUPF.

> Perhaps using the minimum of all AWUPF and rejecting any ctrl which are
> lower than the initial value is right fix ?

That's what I mean in my previous mail.

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Keith Busch]

On Thu, Apr 10, 2025 at 01:32:33AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 10, 2025 at 01:33:38AM +0000, Chaitanya Kulkarni wrote:
>
> > Perhaps using the minimum of all AWUPF and rejecting any ctrl which are
> > lower than the initial value is right fix ?
> 
> That's what I mean in my previous mail.

You'll get different results depending on the non-deterministic order we
enumerate. Can't we just change the stacked limit to the smallest
observed value as we go?

Re: Issue with AWUPF when using multiple controllers in a subsystem

[Christoph Hellwig]

On Thu, Apr 10, 2025 at 10:07:22AM +0100, Keith Busch wrote:
> You'll get different results depending on the non-deterministic order we
> enumerate. Can't we just change the stacked limit to the smallest
> observed value as we go?

That means discovering a new controller could lower the limit while
a file system is using it.  Not a good idea.  That's why I suggested
only supporting the namespace variant ASAP until we actually grow
users relying on it.