create-ns: align the namespaces to 1Mib boundaries when using SI suffixes #2095

maurizio-lombardi · 2023-10-18T14:04:03Z

Some controllers refuse to create namespaces with a size not aligned to a 1 megabyte boundary, this happens when using the -S and -C options.

$ nvme create-ns /dev/nvme0 -S 80M -C 80M -f 0
NVMe status: Invalid Field in Command: A reserved coded value or an unsupported value in a defined field(0x2)

Fix this problem by modifying parse_lba_num_si() to align the values to 1 Megabyte boundaries.

ikegami-t · 2023-10-24T15:07:21Z

The -S and -C options were added for the issue #1747 proposed to accept SI units. Is this PR changes still able to keep and follow the issue requirement? By the way the -s and -c options can be used to set the ~~binaly~~ binary units instead as below I think. But those binary units values are not divided by the LBA size as different from the -S and -C options. Is it acceptable to use the binary units options instead of the PR changes?

$ nvme create-ns /dev/nvme0 -s 80Mi -c 80Mi -f 0

igaw · 2023-11-02T16:16:46Z

I think we should have one pair of options which take value verbatim without any modifications and one set which do the right thing. The second set is for the casual user which just wants to create a new NS and doesn't want to figure all the low level things, e.g. I want a 8GB namespace.

IIRC, this is what we had in mind when we added the --nsze-si/-S and --ncap-si/-C option. This patch just brings it a bit closer to just do the right thing.
SI units

ikegami-t · 2023-11-03T16:10:41Z

By the way for example is there any user to use the -S and -C options to align the values to 512 Kilobyte boundaries but not 1 Megabyte boundaries. If so in the case the options are not able to use for the case with the PR changes.

maurizio-lombardi · 2023-11-06T16:00:23Z

By the way for example is there any user to use the -S and -C options to align the values to 512 Kilobyte boundaries but not 1 Megabyte boundaries. If so in the case the options are not able to use for the case with the PR changes.

That is true, but it's not a bit of a corner case? Is there a real need to create namespaces of < 1Mbyte size?
If yes, in that case, the user can still use the --nsze and --ncap parameters to create small namespaces

HaroPanosyan · 2023-11-06T17:19:19Z

Not sure if namespace granularity (CNS 16h) is being accounted for in current implementation, but if controller supports it then granularity can be accounted for when creating namespaces (with same matching indices 0-15 as for LBA formats).

igaw · 2023-11-07T07:51:27Z

namespace granularity (CNS 16h)

Not yet. For anyone looking for the documentation it is in NVM Command Set Specification (4.1.5.6 Namespace Granularity List (CNS 16h)). Sounds reasonable to use it if it's available.

HaroPanosyan · 2023-11-07T16:03:43Z

namespace granularity (CNS 16h)

Not yet. For anyone looking for the documentation it is in NVM Command Set Specification (4.1.5.6 Namespace Granularity List (CNS 16h)). Sounds reasonable to use it if it's available.

Yup, CNS 01h will report if granularity is supported at bit 7 for "Controller Attributes (CTRATT)"

ikegami-t · 2023-11-08T17:39:34Z

If the granularity is not support should we keep the current command options behavir? Or the controllers needed the size aligned to a 1 megabyte boundary are supported the granularity?

igaw · 2023-11-10T10:11:20Z

If the granularity is not support should we keep the current command options behavir? Or the controllers needed the size aligned to a 1 megabyte boundary are supported the granularity?

I think it makes sense to align it to 1 MB boundary in absence of the granularity information. The worst case scenario is that we waste 1MB disk space. If you look at partitioning tools, it is not uncommon that some space is unused to alignment constrains.

As I said, I see these options here as the user friendly command line options to create namespaces. You should not need to know the spec at all to create a new namespace. As long we have the low level command line arguments around, there is always the possibility to configure a namespace exactly as you want it do be.

ikegami-t · 2023-11-11T06:03:23Z

Agreed. Thanks for your explanation.

igaw · 2023-11-17T09:42:15Z

@maurizio-lombardi do you plan to add the Granularity discovery bit?

maurizio-lombardi · 2023-11-17T09:59:52Z

@maurizio-lombardi do you plan to add the Granularity discovery bit?

I am going to look at how to do that.

igaw · 2023-12-07T13:26:53Z

@maurizio-lombardi any updates?

maurizio-lombardi · 2023-12-07T14:00:41Z

7. 12. 2023 v 14:27, Daniel Wagner ***@***.***>: @maurizio-lombardi any updates?Hello, sorry I was busy with other things, I will come up with something as soon as possible —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

maurizio-lombardi · 2023-12-12T16:56:44Z

I updated the patch but I didn't have the opportunity to test it yet so it may not work as intended. I plan to test it this week

nvme.c

igaw · 2024-01-22T13:34:24Z

I've lost the overview here. Is the patch ready to be merged (tested)? It looks it is in shape.

The only small thing is that the documentation is not mentioning that it will be align on nszegran/ncapgran or fallback on 1MB alignment bondary.

maurizio-lombardi · 2024-01-23T10:24:47Z

I've lost the overview here. Is the patch ready to be merged (tested)? It looks it is in shape.

It is not, on one of my machines I get quite high values from the granularity list, for example 4 Gb of preferred granularity. (desc->nszegran = 4294967296 desc->ncapgran = 4294967296). I didn't expect the values to be so high and this makes some u32 variables to overflow, leading to divisions by zero and crashes.
Plus it makes it impossible to create, for example, a namespace with size 100Mb because it rounds up to a much larger value.

maurizio-lombardi · 2024-01-23T10:27:15Z

It is not, on one of my machines I get quite high values from the granularity list.

By the way, the specification says that the list gives us a preferred granularity, but doesn't say that we are obliged to respect it

igaw · 2024-01-23T13:56:58Z

Uff, this is a big alignment requirement! Does this mean we stick your initial idea to align to 1 MB ? Not really sure what to do here.

maurizio-lombardi · 2024-01-23T13:59:37Z

Uff, this is a big alignment requirement! Does this mean we stick your initial idea to align to 1 MB ? Not really sure what to do here.

Me neither, I am tempted to align to 1 Mb unless the controller suggests that a smaller value is preferred.

igaw · 2024-01-23T14:07:43Z

Maybe we should just do this and see if someone complains :)

maurizio-lombardi · 2024-01-23T14:40:09Z

Maybe we should just do this and see if someone complains :)

Will modify the patch and go ahead then, unless @ikegami-t disagrees

ikegami-t · 2024-01-24T00:58:00Z

Agreed. Thank you.

ikegami-t

Basically looks good but only some minor comments let me mention.

ikegami-t · 2024-01-28T15:26:13Z

nvme.c

+				/* Only the first descriptor is valid */
+				index = 0;
+			} else if (index > gr_list->num_descriptors) {
+				/* The descriptor will contain only zeroes


Should be split the line to start multiple lines comment as ~~belwo~~ below.

- /* The descriptor will contain only zeroes + /* + * The descriptor will contain only zeroes

nvme.c

ikegami-t · 2024-01-28T15:46:39Z

nvme.c

+		return err;
+	}
+
+	align_nsze = align_ncap = 1 << 20; /* Default 1 Mb */


The value can be initialized by the variable definitions as for example. By the way 1 Mb should be 1 MiB. (The lowercase character b also should be uppercase charadter B. This is also same for the commit message summary 1Mib and description 1Mb.)

__u32 nsid: __u32 align_nsze = 1 << 20; /* Default 1 MiB */ __u32 align_ncap = align_nsze;

Ok this one, I will change it

nvme.c

…ixes Some controllers refuse to create namespaces with a size not aligned to a 1 MiB boundary, this happens when using the -S and -C options. $ nvme create-ns /dev/nvme0 -S 80M -C 80M -f 0 NVMe status: Invalid Field in Command: A reserved coded value or an unsupported value in a defined field(0x2) Fix this problem by modifying create_ns() parse_lba_num_si() to align the values to 1 MiB boundaries. If granularity is supported, we will use the specified values, if they are smaller than 1 MiB. Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>

igaw · 2024-02-05T14:54:27Z

Thanks!

maurizio-lombardi force-pushed the create_ns_1mb_v2 branch from edbc0d6 to e68e99b Compare December 12, 2023 16:55

ikegami-t reviewed Dec 14, 2023

View reviewed changes

nvme.c Show resolved Hide resolved

nvme.c Show resolved Hide resolved

maurizio-lombardi force-pushed the create_ns_1mb_v2 branch 3 times, most recently from 046b788 to d5df782 Compare January 9, 2024 07:46

maurizio-lombardi force-pushed the create_ns_1mb_v2 branch from d5df782 to daadfd0 Compare January 26, 2024 15:36

ikegami-t reviewed Jan 28, 2024

View reviewed changes

maurizio-lombardi force-pushed the create_ns_1mb_v2 branch from daadfd0 to ce4bd36 Compare February 5, 2024 14:16

maurizio-lombardi force-pushed the create_ns_1mb_v2 branch from ce4bd36 to 4285759 Compare February 5, 2024 14:32

igaw merged commit 3d1243e into linux-nvme:master Feb 5, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-ns: align the namespaces to 1Mib boundaries when using SI suffixes #2095

create-ns: align the namespaces to 1Mib boundaries when using SI suffixes #2095

maurizio-lombardi commented Oct 18, 2023

ikegami-t commented Oct 24, 2023 •

edited

Loading

igaw commented Nov 2, 2023

ikegami-t commented Nov 3, 2023

maurizio-lombardi commented Nov 6, 2023

HaroPanosyan commented Nov 6, 2023

igaw commented Nov 7, 2023 •

edited

Loading

HaroPanosyan commented Nov 7, 2023

ikegami-t commented Nov 8, 2023

igaw commented Nov 10, 2023

ikegami-t commented Nov 11, 2023

igaw commented Nov 17, 2023

maurizio-lombardi commented Nov 17, 2023

igaw commented Dec 7, 2023

maurizio-lombardi commented Dec 7, 2023 via email

maurizio-lombardi commented Dec 12, 2023

igaw commented Jan 22, 2024 •

edited

Loading

maurizio-lombardi commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

igaw commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

igaw commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

ikegami-t commented Jan 24, 2024

ikegami-t left a comment

ikegami-t Jan 28, 2024 •

edited

Loading

ikegami-t Jan 28, 2024 •

edited

Loading

maurizio-lombardi Feb 1, 2024

igaw commented Feb 5, 2024

create-ns: align the namespaces to 1Mib boundaries when using SI suffixes #2095

create-ns: align the namespaces to 1Mib boundaries when using SI suffixes #2095

Conversation

maurizio-lombardi commented Oct 18, 2023

ikegami-t commented Oct 24, 2023 • edited Loading

igaw commented Nov 2, 2023

ikegami-t commented Nov 3, 2023

maurizio-lombardi commented Nov 6, 2023

HaroPanosyan commented Nov 6, 2023

igaw commented Nov 7, 2023 • edited Loading

HaroPanosyan commented Nov 7, 2023

ikegami-t commented Nov 8, 2023

igaw commented Nov 10, 2023

ikegami-t commented Nov 11, 2023

igaw commented Nov 17, 2023

maurizio-lombardi commented Nov 17, 2023

igaw commented Dec 7, 2023

maurizio-lombardi commented Dec 7, 2023 via email

maurizio-lombardi commented Dec 12, 2023

igaw commented Jan 22, 2024 • edited Loading

maurizio-lombardi commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

igaw commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

igaw commented Jan 23, 2024

maurizio-lombardi commented Jan 23, 2024

ikegami-t commented Jan 24, 2024

ikegami-t left a comment

Choose a reason for hiding this comment

ikegami-t Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

ikegami-t Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

maurizio-lombardi Feb 1, 2024

Choose a reason for hiding this comment

igaw commented Feb 5, 2024

ikegami-t commented Oct 24, 2023 •

edited

Loading

igaw commented Nov 7, 2023 •

edited

Loading

igaw commented Jan 22, 2024 •

edited

Loading

ikegami-t Jan 28, 2024 •

edited

Loading

ikegami-t Jan 28, 2024 •

edited

Loading