| Message ID | 152062242529.34816.8248756922497912006.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dan Williams <dan.j.williams@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@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@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@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer@redhat.com> wrote: > Dan Williams <dan.j.williams@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.
Dan Williams <dan.j.williams@intel.com> writes: > On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer@redhat.com> wrote: >> Dan Williams <dan.j.williams@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
On Fri, Mar 9, 2018 at 11:58 AM, Jeff Moyer <jmoyer@redhat.com> wrote: > Dan Williams <dan.j.williams@intel.com> writes: > >> On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer@redhat.com> wrote: >>> Dan Williams <dan.j.williams@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.
On 03/09/2018 04:23 PM, Dan Williams wrote: > On Fri, Mar 9, 2018 at 11:58 AM, Jeff Moyer <jmoyer@redhat.com> wrote: >> Dan Williams <dan.j.williams@intel.com> writes: >> >>> On Fri, Mar 9, 2018 at 11:39 AM, Jeff Moyer <jmoyer@redhat.com> wrote: >>>> Dan Williams <dan.j.williams@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@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm >
On Fri, Mar 9, 2018 at 1:44 PM, Linda Knippers <ljklists@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.
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);
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@linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- util/json.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)