| Message ID | 20181119080056.13386-2-oohall@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] libndctl: Use the supported_alignment attribute | expand |
On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote: > > When creating an fsdax or devdax namespace we need to verify that the > seed namespaces exist. This patch reworks the validation so that it's > done earlier to simplify the subsequent patches in the series. > > No functional changes. It does appear to have a functional change. do_setup_pfn() supports the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY mode. This is what one gets by default with "legacy" pmem defined as E820-type-12 memory. In that case the kernel assumes that the resulting memmap is always small enough to be allocated from DRAM and there is no need to use a dynamic pfn device. So, if I'm not mistaken, the deletion of do_setup_pfn() loses that special case handling.
On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote: > > > > When creating an fsdax or devdax namespace we need to verify that the > > seed namespaces exist. This patch reworks the validation so that it's > > done earlier to simplify the subsequent patches in the series. > > > > No functional changes. > > It does appear to have a functional change. do_setup_pfn() supports > the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY > mode. Hmm, ok. Up until now I had assumed that as far as ndctl was concerned NS_MODE_MEMORY was synonymous with a pfn namespace. > This is what one gets by default with "legacy" pmem defined as > E820-type-12 memory. In that case the kernel assumes that the > resulting memmap is always small enough to be allocated from DRAM and > there is no need to use a dynamic pfn device. So, if I'm not > mistaken, the deletion of do_setup_pfn() loses that special case > handling. From what I see, the main difference is that ndctl would fail validation when fsdax mode is specified and there's no pfn namespace support in the kernel. I agree that's not great, but I'm not sure what we should be doing here. The current behaviour will silently ignore -a if "-m fsdax -M mem" is specified in the reconfigure case, which doesn't seem great either.
Sorry for letting this conversation go cold, lets try to revive it as Vishal is looking to cut an ndctl v64 in the coming days and it would be good to include this support. On Tue, Nov 27, 2018 at 5:27 AM Oliver <oohall@gmail.com> wrote: > > On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote: > > > > > > When creating an fsdax or devdax namespace we need to verify that the > > > seed namespaces exist. This patch reworks the validation so that it's > > > done earlier to simplify the subsequent patches in the series. > > > > > > No functional changes. > > > > It does appear to have a functional change. do_setup_pfn() supports > > the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY > > mode. > > Hmm, ok. Up until now I had assumed that as far as ndctl was concerned > NS_MODE_MEMORY was synonymous with a pfn namespace. > > > This is what one gets by default with "legacy" pmem defined as > > E820-type-12 memory. In that case the kernel assumes that the > > resulting memmap is always small enough to be allocated from DRAM and > > there is no need to use a dynamic pfn device. So, if I'm not > > mistaken, the deletion of do_setup_pfn() loses that special case > > handling. > > From what I see, the main difference is that ndctl would fail > validation when fsdax mode is specified and there's no pfn namespace > support in the kernel. I agree that's not great, but I'm not sure what > we should be doing here. The current behaviour will silently ignore -a > if "-m fsdax -M mem" is specified in the reconfigure case, which > doesn't seem great either. Right, both issues need to be addressed. As far as I can see do_setup_pfn() just needs to be extended to detect attempts to set a non-default alignment, and then when setting the alignment require it to be one of the supported values. I expect we should also extend the region listing output to include the supported alignments. Does this clarify your concern enough to attempt a respin of the series? If not just holler and Vishal and I will arm-wrestle to decide who picks this up.
On Tue, Jan 8, 2019 at 1:59 PM Dan Williams <dan.j.williams@intel.com> wrote: > > Sorry for letting this conversation go cold, lets try to revive it as > Vishal is looking to cut an ndctl v64 in the coming days and it would > be good to include this support. > > On Tue, Nov 27, 2018 at 5:27 AM Oliver <oohall@gmail.com> wrote: > > > > On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote: > > > > > > > > When creating an fsdax or devdax namespace we need to verify that the > > > > seed namespaces exist. This patch reworks the validation so that it's > > > > done earlier to simplify the subsequent patches in the series. > > > > > > > > No functional changes. > > > > > > It does appear to have a functional change. do_setup_pfn() supports > > > the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY > > > mode. > > > > Hmm, ok. Up until now I had assumed that as far as ndctl was concerned > > NS_MODE_MEMORY was synonymous with a pfn namespace. > > > > > This is what one gets by default with "legacy" pmem defined as > > > E820-type-12 memory. In that case the kernel assumes that the > > > resulting memmap is always small enough to be allocated from DRAM and > > > there is no need to use a dynamic pfn device. So, if I'm not > > > mistaken, the deletion of do_setup_pfn() loses that special case > > > handling. > > > > From what I see, the main difference is that ndctl would fail > > validation when fsdax mode is specified and there's no pfn namespace > > support in the kernel. I agree that's not great, but I'm not sure what > > we should be doing here. The current behaviour will silently ignore -a > > if "-m fsdax -M mem" is specified in the reconfigure case, which > > doesn't seem great either. > > Right, both issues need to be addressed. As far as I can see > do_setup_pfn() just needs to be extended to detect attempts to set a > non-default alignment, and then when setting the alignment require it > to be one of the supported values. Seems reasonable. I'll dust off the series and post a respin tomorrow. > I expect we should also extend the region listing output to include > the supported alignments. Probably a good idea. I think even have a patch to do that somewhere... > Does this clarify your concern enough to attempt a respin of the > series? If not just holler and Vishal and I will arm-wrestle to decide > who picks this up.
diff --git a/ndctl/namespace.c b/ndctl/namespace.c index b6f12306fe76..dc9a56609881 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -465,6 +465,8 @@ static int validate_namespace_options(struct ndctl_region *region, { const char *region_name = ndctl_region_get_devname(region); unsigned long long size_align = SZ_4K, units = 1, resource; + struct ndctl_pfn *pfn = NULL; + struct ndctl_dax *dax = NULL; unsigned int ways; int rc = 0; @@ -521,14 +523,28 @@ static int validate_namespace_options(struct ndctl_region *region, } else if (ndns) p->mode = ndctl_namespace_get_mode(ndns); - if (param.align) { - struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region); - struct ndctl_dax *dax = ndctl_region_get_dax_seed(region); + if (p->mode == NDCTL_NS_MODE_MEMORY) { + pfn = ndctl_region_get_pfn_seed(region); + if (!pfn && param.mode_default) { + debug("%s fsdax mode not available\n", region_name); + p->mode = NDCTL_NS_MODE_RAW; + } else if (!pfn) { + error("Kernel does not support fsdax mode\n"); + return -ENODEV; + } + } else if (p->mode == NDCTL_NS_MODE_DAX) { + dax = ndctl_region_get_dax_seed(region); + if (!dax) { + error("Kernel does not support devdax mode\n"); + return -ENODEV; + } + } + if (param.align) { p->align = parse_size64(param.align); if (p->mode == NDCTL_NS_MODE_MEMORY && p->align != SZ_2M - && (!pfn || !ndctl_pfn_has_align(pfn))) { + && !ndctl_pfn_has_align(pfn)) { /* * Initial pfn device support in the kernel * supported a 2M default alignment when @@ -538,7 +554,7 @@ static int validate_namespace_options(struct ndctl_region *region, region_name); return -EAGAIN; } else if (p->mode == NDCTL_NS_MODE_DAX - && (!dax || !ndctl_dax_has_align(dax))) { + && !ndctl_dax_has_align(dax)) { /* * Unlike the pfn case, we require the kernel to * have 'align' support for device-dax. @@ -705,31 +721,6 @@ static int validate_namespace_options(struct ndctl_region *region, || p->mode == NDCTL_NS_MODE_DAX) p->loc = NDCTL_PFN_LOC_PMEM; - /* check if we need, and whether the kernel supports, pfn devices */ - if (do_setup_pfn(ndns, p)) { - struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region); - - if (!pfn && param.mode_default) { - debug("%s fsdax mode not available\n", region_name); - p->mode = NDCTL_NS_MODE_RAW; - } else if (!pfn) { - error("operation failed, %s fsdax mode not available\n", - region_name); - return -EINVAL; - } - } - - /* check if we need, and whether the kernel supports, dax devices */ - if (p->mode == NDCTL_NS_MODE_DAX) { - struct ndctl_dax *dax = ndctl_region_get_dax_seed(region); - - if (!dax) { - error("operation failed, %s devdax mode not available\n", - region_name); - return -EINVAL; - } - } - p->autolabel = param.autolabel; return 0;
When creating an fsdax or devdax namespace we need to verify that the seed namespaces exist. This patch reworks the validation so that it's done earlier to simplify the subsequent patches in the series. No functional changes. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- ndctl/namespace.c | 51 +++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 30 deletions(-)