7:07 p.m.
Commit a7320456f1bc "ndctl: add sector_size to 'ndctl list' output"
mishandles the case where a namespace does not specify a sector_size,
but otherwise supports a sector_size selection:
# ndctl list --namespace=namespace1.0
{
"dev":"namespace1.0",
"mode":"memory",
"size":32763805696,
"uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
"sector_size":-1,
"blockdev":"pmem1",
"numa_node":0
}
Fix this and clean up the output to only provide a sector_size in the
non-default (i.e. != 512 bytes) case.
Fixes: a7320456f1bc ("ndctl: add sector_size to 'ndctl list' output")
Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams(a)intel.com>
---
util/json.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/util/json.c b/util/json.c
index 17d8f135b686..e7f789b36385 100644
--- a/util/json.c
+++ b/util/json.c
@@ -646,6 +646,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace
*ndns,
struct json_object *jndns = json_object_new_object();
struct json_object *jobj, *jbbs = NULL;
unsigned long long size = ULLONG_MAX;
+ unsigned int sector_size = UINT_MAX;
enum ndctl_namespace_mode mode;
const char *bdev = NULL, *name;
unsigned int bb_count = 0;
@@ -769,29 +770,23 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace
*ndns,
} else
bdev = ndctl_namespace_get_block_device(ndns);
- jobj = NULL;
- if (btt) {
- jobj = json_object_new_int(ndctl_btt_get_sector_size(btt));
- if (!jobj)
- goto err;
- } else if (!dax) {
- unsigned int sector_size = ndctl_namespace_get_sector_size(ndns);
-
- /*
- * The kernel will default to a 512 byte sector size on PMEM
- * namespaces that don't explicitly have a sector size. This
- * happens because they use pre-v1.2 labels or because they
- * don't have a label space (devtype=nd_namespace_io).
- */
- if (!sector_size)
- sector_size = 512;
+ if (btt)
+ sector_size = ndctl_btt_get_sector_size(btt);
+ else if (!dax)
+ sector_size = ndctl_namespace_get_sector_size(ndns);
+ /*
+ * The kernel will default to a 512 byte sector size on PMEM
+ * namespaces that don't explicitly have a sector size. This
+ * happens because they use pre-v1.2 labels or because they
+ * don't have a label space (devtype=nd_namespace_io).
+ */
+ if (sector_size < UINT_MAX && sector_size > 512) {
jobj = json_object_new_int(sector_size);
if (!jobj)
goto err;
- }
- if (jobj)
json_object_object_add(jndns, "sector_size", jobj);
+ }
if (bdev && bdev[0]) {
jobj = json_object_new_string(bdev);
7:39 p.m.
Dan Williams <dan.j.williams(a)intel.com> writes:
Commit a7320456f1bc "ndctl: add sector_size to 'ndctl
list' output"
mishandles the case where a namespace does not specify a sector_size,
but otherwise supports a sector_size selection:
# ndctl list --namespace=namespace1.0
{
"dev":"namespace1.0",
"mode":"memory",
"size":32763805696,
"uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
"sector_size":-1,
"blockdev":"pmem1",
"numa_node":0
}
Fix this and clean up the output to only provide a sector_size in the
non-default (i.e. != 512 bytes) case.
That sounds confusing. Why not just always print out the sector size?
-Jeff
Fixes: a7320456f1bc ("ndctl: add sector_size to 'ndctl list' output")
Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams(a)intel.com>
---
util/json.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/util/json.c b/util/json.c
index 17d8f135b686..e7f789b36385 100644
--- a/util/json.c
+++ b/util/json.c
@@ -646,6 +646,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace
*ndns,
struct json_object *jndns = json_object_new_object();
struct json_object *jobj, *jbbs = NULL;
unsigned long long size = ULLONG_MAX;
+ unsigned int sector_size = UINT_MAX;
enum ndctl_namespace_mode mode;
const char *bdev = NULL, *name;
unsigned int bb_count = 0;
@@ -769,29 +770,23 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace
*ndns,
} else
bdev = ndctl_namespace_get_block_device(ndns);
- jobj = NULL;
- if (btt) {
- jobj = json_object_new_int(ndctl_btt_get_sector_size(btt));
- if (!jobj)
- goto err;
- } else if (!dax) {
- unsigned int sector_size = ndctl_namespace_get_sector_size(ndns);
-
- /*
- * The kernel will default to a 512 byte sector size on PMEM
- * namespaces that don't explicitly have a sector size. This
- * happens because they use pre-v1.2 labels or because they
- * don't have a label space (devtype=nd_namespace_io).
- */
- if (!sector_size)
- sector_size = 512;
+ if (btt)
+ sector_size = ndctl_btt_get_sector_size(btt);
+ else if (!dax)
+ sector_size = ndctl_namespace_get_sector_size(ndns);
+ /*
+ * The kernel will default to a 512 byte sector size on PMEM
+ * namespaces that don't explicitly have a sector size. This
+ * happens because they use pre-v1.2 labels or because they
+ * don't have a label space (devtype=nd_namespace_io).
+ */
+ if (sector_size < UINT_MAX && sector_size > 512) {
jobj = json_object_new_int(sector_size);
if (!jobj)
goto err;
- }
- if (jobj)
json_object_object_add(jndns, "sector_size", jobj);
+ }
if (bdev && bdev[0]) {
jobj = json_object_new_string(bdev);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm(a)lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
7:44 p.m.
On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer(a)redhat.com> wrote:
Dan Williams <dan.j.williams(a)intel.com> writes:
> Commit a7320456f1bc "ndctl: add sector_size to 'ndctl list'
output"
> mishandles the case where a namespace does not specify a sector_size,
> but otherwise supports a sector_size selection:
>
> # ndctl list --namespace=namespace1.0
> {
> "dev":"namespace1.0",
> "mode":"memory",
> "size":32763805696,
> "uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
> "sector_size":-1,
> "blockdev":"pmem1",
> "numa_node":0
> }
>
> Fix this and clean up the output to only provide a sector_size in the
> non-default (i.e. != 512 bytes) case.
That sounds confusing. Why not just always print out the sector size?
I'm assuming the users that will go through the hassle of specifying a
non-default sector_size are few and far between. This helps with the
useful information density on a single screen for humans looking at
json output.
7:58 p.m.
Dan Williams <dan.j.williams(a)intel.com> writes:
On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer(a)redhat.com>
wrote:
> Dan Williams <dan.j.williams(a)intel.com> writes:
>
>> Commit a7320456f1bc "ndctl: add sector_size to 'ndctl list'
output"
>> mishandles the case where a namespace does not specify a sector_size,
>> but otherwise supports a sector_size selection:
>>
>> # ndctl list --namespace=namespace1.0
>> {
>> "dev":"namespace1.0",
>> "mode":"memory",
>> "size":32763805696,
>> "uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
>> "sector_size":-1,
>> "blockdev":"pmem1",
>> "numa_node":0
>> }
>>
>> Fix this and clean up the output to only provide a sector_size in the
>> non-default (i.e. != 512 bytes) case.
>
> That sounds confusing. Why not just always print out the sector size?
I'm assuming the users that will go through the hassle of specifying a
non-default sector_size are few and far between. This helps with the
useful information density on a single screen for humans looking at
json output.
I think that, to the novice, seeing one namespace print out sector size
and another not do the same, that would be confusing. Also, you'd have
to know what the default is.
Cheers,
Jeff
9:23 p.m.
On Fri, Mar 9, 2018 at 11:58 AM, Jeff Moyer <jmoyer(a)redhat.com> wrote:
Dan Williams <dan.j.williams(a)intel.com> writes:
> On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer(a)redhat.com> wrote:
>> Dan Williams <dan.j.williams(a)intel.com> writes:
>>
>>> Commit a7320456f1bc "ndctl: add sector_size to 'ndctl list'
output"
>>> mishandles the case where a namespace does not specify a sector_size,
>>> but otherwise supports a sector_size selection:
>>>
>>> # ndctl list --namespace=namespace1.0
>>> {
>>> "dev":"namespace1.0",
>>> "mode":"memory",
>>> "size":32763805696,
>>> "uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
>>> "sector_size":-1,
>>> "blockdev":"pmem1",
>>> "numa_node":0
>>> }
>>>
>>> Fix this and clean up the output to only provide a sector_size in the
>>> non-default (i.e. != 512 bytes) case.
>>
>> That sounds confusing. Why not just always print out the sector size?
>
> I'm assuming the users that will go through the hassle of specifying a
> non-default sector_size are few and far between. This helps with the
> useful information density on a single screen for humans looking at
> json output.
I think that, to the novice, seeing one namespace print out sector size
and another not do the same, that would be confusing. Also, you'd have
to know what the default is.
Yes, but personally I think it is only an expert that would care about
non-512 byte sector sizes. There are no device physics or other
benefits that require different sector sizes. The only potential
benefit is amortizing the cost of BTT over larger sectors, but for
Linux ext4 and xfs should be fine without a BTT. The UEFI spec
unfortunately requires a default of 4K sector size, but that's only
for inter-OS compatibility. I'm just finding it hard to justify
treating sector_size as a first class attribute that the typical user
would care about.
9:44 p.m.
On 03/09/2018 04:23 PM, Dan Williams wrote:
On Fri, Mar 9, 2018 at 11:58 AM, Jeff Moyer <jmoyer(a)redhat.com>
wrote:
> Dan Williams <dan.j.williams(a)intel.com> writes:
>
>> On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer(a)redhat.com> wrote:
>>> Dan Williams <dan.j.williams(a)intel.com> writes:
>>>
>>>> Commit a7320456f1bc "ndctl: add sector_size to 'ndctl list'
output"
>>>> mishandles the case where a namespace does not specify a sector_size,
>>>> but otherwise supports a sector_size selection:
>>>>
>>>> # ndctl list --namespace=namespace1.0
>>>> {
>>>> "dev":"namespace1.0",
>>>> "mode":"memory",
>>>> "size":32763805696,
>>>> "uuid":"7c985ba5-6d33-48bd-8fde-6c25a520abe0",
>>>> "sector_size":-1,
>>>> "blockdev":"pmem1",
>>>> "numa_node":0
>>>> }
>>>>
>>>> Fix this and clean up the output to only provide a sector_size in the
>>>> non-default (i.e. != 512 bytes) case.
>>>
>>> That sounds confusing. Why not just always print out the sector size?
>>
>> I'm assuming the users that will go through the hassle of specifying a
>> non-default sector_size are few and far between. This helps with the
>> useful information density on a single screen for humans looking at
>> json output.
>
> I think that, to the novice, seeing one namespace print out sector size
> and another not do the same, that would be confusing. Also, you'd have
> to know what the default is.
As a former ndctl user, I'm with Jeff.
Yes, but personally I think it is only an expert that would care
about
non-512 byte sector sizes. There are no device physics or other
benefits that require different sector sizes. The only potential
benefit is amortizing the cost of BTT over larger sectors, but for
Linux ext4 and xfs should be fine without a BTT. The UEFI spec
unfortunately requires a default of 4K sector size, but that's only
for inter-OS compatibility. I'm just finding it hard to justify
treating sector_size as a first class attribute that the typical user
would care about.
Since the value can be set, even to the default value, and the kernel default doesn't
match the UEFI default, it seems worth including in all cases.
-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm(a)lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
10:06 p.m.
On Fri, Mar 9, 2018 at 1:44 PM, Linda Knippers <ljklists(a)gmail.com> wrote:
[..]
>> I think that, to the novice, seeing one namespace print out
sector size
>> and another not do the same, that would be confusing. Also, you'd have
>> to know what the default is.
As a former ndctl user, I'm with Jeff.
The user is always right, v2 on the way.
1620
days inactive
1620
days old
6 comments
3 participants
participants (3)
-
Dan Williams -
Jeff Moyer -
Linda Knippers