[ndctl PATCH 0/2] fix the nfit_test fallback for 'ndctl bat'

[ndctl PATCH 0/2] fix the nfit_test fallback for 'ndctl bat'

[Dan Williams]

Per test results from Ross [1] we don't want 'ndctl bat' to require
nfit_test.  Patch2 rearranges the fallback to only trigger when an
ACPI.NFIT provider is found.

This uncovered an interesting failure mode whereby
ndctl_bus_get_by_provider() failed to locate an expected bus.  The fix
for that lead to the creation of a new ndctl_invalidate() api, see
patch1 for the details.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-March/005017.html

---

Dan Williams (2):
      ndctl: provide a method to invalidate the bus list
      ndctl: don't skip bat tests if nfit_test not available


 lib/libndctl.c          |   22 ++++++++++++++---
 lib/libndctl.sym        |    1 +
 lib/ndctl/libndctl.h.in |    1 +
 test/blk_namespaces.c   |   62 +++++++++++++++++++++++------------------------
 test/pmem_namespaces.c  |   55 ++++++++++++++++++++----------------------
 5 files changed, 76 insertions(+), 65 deletions(-)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 1/2] ndctl: provide a method to invalidate the bus list

[Dan Williams]

Consider the following scenario:

    ndctl_new()
    ndctl_bus_foreach()
    <install a new nvdimm-bus provider>
    ndctl_bus_foreach()

...the second ndctl_bus_foreach() will not see any new buses because the
bus results are cached.  When the holder of the context knows that it
has invalidated the bus list it needs to call ndctl_invalidate() so that
subsequent ndctl_bus_foreach() scans are guaranteed to see new buses.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 lib/libndctl.c          |   22 ++++++++++++++++++----
 lib/libndctl.sym        |    1 +
 lib/ndctl/libndctl.h.in |    1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/lib/libndctl.c b/lib/libndctl.c
index 6728508024b0..b628243df651 100644
--- a/lib/libndctl.c
+++ b/lib/libndctl.c
@@ -584,7 +584,7 @@ static void free_region(struct ndctl_region *region)
 	free(region);
 }
 
-static void free_bus(struct ndctl_bus *bus)
+static void free_bus(struct ndctl_bus *bus, struct list_head *head)
 {
 	struct ndctl_dimm *dimm, *_d;
 	struct ndctl_region *region, *_r;
@@ -598,7 +598,8 @@ static void free_bus(struct ndctl_bus *bus)
 	}
 	list_for_each_safe(&bus->regions, region, _r, list)
 		free_region(region);
-	list_del_from(&bus->ctx->busses, &bus->list);
+	if (head)
+		list_del_from(head, &bus->list);
 	free(bus->provider);
 	free(bus->bus_path);
 	free(bus->bus_buf);
@@ -611,7 +612,7 @@ static void free_context(struct ndctl_ctx *ctx)
 	struct ndctl_bus *bus, *_b;
 
 	list_for_each_safe(&ctx->busses, bus, _b, list)
-		free_bus(bus);
+		free_bus(bus, &ctx->busses);
 	free(ctx);
 }
 
@@ -869,9 +870,9 @@ static void parse_nfit_mem_flags(struct ndctl_dimm *dimm, char *flags)
 static int add_bus(void *parent, int id, const char *ctl_base)
 {
 	int rc = -ENOMEM;
-	struct ndctl_bus *bus;
 	char buf[SYSFS_ATTR_SIZE];
 	struct ndctl_ctx *ctx = parent;
+	struct ndctl_bus *bus, *bus_dup;
 	char *path = calloc(1, strlen(ctl_base) + 100);
 
 	if (!path)
@@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char *ctl_base)
 		goto err_read;
 	bus->buf_len = strlen(bus->bus_path) + 50;
 
+	ndctl_bus_foreach(ctx, bus_dup)
+		if (strcmp(ndctl_bus_get_provider(bus_dup),
+					ndctl_bus_get_provider(bus)) == 0) {
+			free_bus(bus, NULL);
+			free(path);
+			return 1;
+		}
+
 	list_add(&ctx->busses, &bus->list);
 
 	rc = 0;
@@ -954,6 +963,11 @@ static void busses_init(struct ndctl_ctx *ctx)
 	device_parse(ctx, NULL, "/sys/class/nd", "ndctl", ctx, add_bus);
 }
 
+NDCTL_EXPORT void ndctl_invalidate(struct ndctl_ctx *ctx)
+{
+	ctx->busses_init = 0;
+}
+
 /**
  * ndctl_bus_get_first - retrieve first "nd bus" in the system
  * @ctx: context established by ndctl_new
diff --git a/lib/libndctl.sym b/lib/libndctl.sym
index 25d7b982ecb1..9df2b5a6e89d 100644
--- a/lib/libndctl.sym
+++ b/lib/libndctl.sym
@@ -11,6 +11,7 @@ global:
 	ndctl_unref;
 	ndctl_set_log_priority;
 	ndctl_new;
+	ndctl_invalidate;
 local:
         *;
 };
diff --git a/lib/ndctl/libndctl.h.in b/lib/ndctl/libndctl.h.in
index b16f26dae535..831f7822e91f 100644
--- a/lib/ndctl/libndctl.h.in
+++ b/lib/ndctl/libndctl.h.in
@@ -77,6 +77,7 @@ struct ndctl_ctx;
 struct ndctl_ctx *ndctl_ref(struct ndctl_ctx *ctx);
 struct ndctl_ctx *ndctl_unref(struct ndctl_ctx *ctx);
 int ndctl_new(struct ndctl_ctx **ctx);
+void ndctl_invalidate(struct ndctl_ctx *ctx);
 void ndctl_set_log_fn(struct ndctl_ctx *ctx,
                   void (*log_fn)(struct ndctl_ctx *ctx,
                                  int priority, const char *file, int line, const char *fn,

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 2/2] ndctl: don't skip bat tests if nfit_test not available

[Dan Williams]

The 'blk-ns' and 'pmem-ns' tests are meant to fallback to nfit_test, but
not require nfit_test.

Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 test/blk_namespaces.c  |   62 +++++++++++++++++++++++-------------------------
 test/pmem_namespaces.c |   55 ++++++++++++++++++++-----------------------
 2 files changed, 56 insertions(+), 61 deletions(-)

diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
index b4d677b4d0d6..0ef5b65fd511 100644
--- a/test/blk_namespaces.c
+++ b/test/blk_namespaces.c
@@ -213,9 +213,9 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test)
 	char bdev[50];
 	struct ndctl_ctx *ctx;
 	struct ndctl_bus *bus;
-	struct kmod_module *mod;
 	struct ndctl_dimm *dimm;
-	struct kmod_ctx *kmod_ctx;
+	struct kmod_module *mod = NULL;
+	struct kmod_ctx *kmod_ctx = NULL;
 	struct ndctl_namespace *ndns[2];
 	struct ndctl_region *region, *blk_region = NULL;
 
@@ -228,38 +228,34 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test)
 
 	ndctl_set_log_priority(ctx, log_level);
 
-	kmod_ctx = kmod_new(NULL, NULL);
-	if (!kmod_ctx)
-		goto err_kmod;
-	kmod_set_log_priority(kmod_ctx, log_level);
-
-	rc = kmod_module_new_from_name(kmod_ctx, "nfit_test", &mod);
-	if (rc < 0)
-		goto err_module;
-
-	rc = kmod_module_probe_insert_module(mod, KMOD_PROBE_APPLY_BLACKLIST,
-			NULL, NULL, NULL, NULL);
-	if (rc < 0) {
-		rc = 77;
-		ndctl_test_skip(test);
-		fprintf(stderr, "nfit_test unavailable skipping tests\n");
-		goto err_module;
-	}
-
 	bus = ndctl_bus_get_by_provider(ctx, "ACPI.NFIT");
-	if (!bus)
-		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
-
 	if (!bus) {
-		fprintf(stderr, "%s: failed to find NFIT-provider\n", comm);
-		ndctl_test_skip(test);
-		rc = 77;
-		goto err_cleanup;
-	} else {
-		fprintf(stderr, "%s: found provider: %s\n", comm,
-				ndctl_bus_get_provider(bus));
+		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
+		kmod_ctx = kmod_new(NULL, NULL);
+		if (!kmod_ctx)
+			goto err_kmod;
+		kmod_set_log_priority(kmod_ctx, log_level);
+
+		rc = kmod_module_new_from_name(kmod_ctx, "nfit_test", &mod);
+		if (rc < 0)
+			goto err_module;
+
+		rc = kmod_module_probe_insert_module(mod,
+				KMOD_PROBE_APPLY_BLACKLIST,
+				NULL, NULL, NULL, NULL);
+		ndctl_invalidate(ctx);
+		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
+		if (rc < 0 || !bus) {
+			rc = 77;
+			ndctl_test_skip(test);
+			fprintf(stderr, "nfit_test unavailable skipping tests\n");
+			goto err_module;
+		}
 	}
 
+	fprintf(stderr, "%s: found provider: %s\n", comm,
+			ndctl_bus_get_provider(bus));
+
 	/* get the system to a clean state */
         ndctl_region_foreach(bus, region)
                 ndctl_region_disable_invalidate(region);
@@ -345,10 +341,12 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test)
 	if (bus)
 		ndctl_region_foreach(bus, region)
 			ndctl_region_disable_invalidate(region);
-	kmod_module_remove_module(mod, 0);
+	if (mod)
+		kmod_module_remove_module(mod, 0);
 
  err_module:
-	kmod_unref(kmod_ctx);
+	if (kmod_ctx)
+		kmod_unref(kmod_ctx);
  err_kmod:
 	ndctl_unref(ctx);
 	return rc;
diff --git a/test/pmem_namespaces.c b/test/pmem_namespaces.c
index 198f8ef0cb37..387c5e011f66 100644
--- a/test/pmem_namespaces.c
+++ b/test/pmem_namespaces.c
@@ -173,10 +173,10 @@ static const char *comm = "test-pmem-namespaces";
 int test_pmem_namespaces(int log_level, struct ndctl_test *test)
 {
 	struct ndctl_region *region, *pmem_region = NULL;
+	struct kmod_ctx *kmod_ctx = NULL;
+	struct kmod_module *mod = NULL;
 	struct ndctl_namespace *ndns;
-	struct kmod_ctx *kmod_ctx;
 	struct ndctl_dimm *dimm;
-	struct kmod_module *mod;
 	struct ndctl_ctx *ctx;
 	struct ndctl_bus *bus;
 	char bdev[50];
@@ -191,36 +191,33 @@ int test_pmem_namespaces(int log_level, struct ndctl_test *test)
 
 	ndctl_set_log_priority(ctx, log_level);
 
-	kmod_ctx = kmod_new(NULL, NULL);
-	if (!kmod_ctx)
-		goto err_kmod;
-	kmod_set_log_priority(kmod_ctx, log_level);
-
-	rc = kmod_module_new_from_name(kmod_ctx, "nfit_test", &mod);
-	if (rc < 0)
-		goto err_module;
-
-	rc = kmod_module_probe_insert_module(mod, KMOD_PROBE_APPLY_BLACKLIST,
-			NULL, NULL, NULL, NULL);
-	if (rc < 0) {
-		rc = 77;
-		ndctl_test_skip(test);
-		fprintf(stderr, "nfit_test unavailable skipping tests\n");
-		goto err_module;
-	}
-
 	bus = ndctl_bus_get_by_provider(ctx, "ACPI.NFIT");
-	if (!bus)
+	if (!bus) {
+		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
+		kmod_ctx = kmod_new(NULL, NULL);
+		if (!kmod_ctx)
+			goto err_kmod;
+		kmod_set_log_priority(kmod_ctx, log_level);
+
+		rc = kmod_module_new_from_name(kmod_ctx, "nfit_test", &mod);
+		if (rc < 0)
+			goto err_module;
+
+		rc = kmod_module_probe_insert_module(mod,
+				KMOD_PROBE_APPLY_BLACKLIST,
+				NULL, NULL, NULL, NULL);
+		ndctl_invalidate(ctx);
 		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
+		if (rc < 0 || !bus) {
+			rc = 77;
+			ndctl_test_skip(test);
+			fprintf(stderr, "nfit_test unavailable skipping tests\n");
+			goto err_module;
+		}
+	}
 
-	if (!bus) {
-		fprintf(stderr, "%s: failed to find NFIT-provider\n", comm);
-		ndctl_test_skip(test);
-		rc = 77;
-		goto err;
-	} else
-		fprintf(stderr, "%s: found provider: %s\n", comm,
-				ndctl_bus_get_provider(bus));
+	fprintf(stderr, "%s: found provider: %s\n", comm,
+			ndctl_bus_get_provider(bus));
 
 	/* get the system to a clean state */
         ndctl_region_foreach(bus, region)

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/2] ndctl: provide a method to invalidate the bus list

[Ross Zwisler]

On Wed, Mar 23, 2016 at 06:16:53PM -0700, Dan Williams wrote:
> Consider the following scenario:
> 
>     ndctl_new()
>     ndctl_bus_foreach()
>     <install a new nvdimm-bus provider>
>     ndctl_bus_foreach()
> 
> ...the second ndctl_bus_foreach() will not see any new buses because the
> bus results are cached.  When the holder of the context knows that it
> has invalidated the bus list it needs to call ndctl_invalidate() so that
> subsequent ndctl_bus_foreach() scans are guaranteed to see new buses.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  lib/libndctl.c          |   22 ++++++++++++++++++----
>  lib/libndctl.sym        |    1 +
>  lib/ndctl/libndctl.h.in |    1 +

I can't get this patch to apply because I don't have lib/ndctl/libndctl.h.in
in my source tree.

I'm using your upstream repo:

https://github.com/pmem/ndctl.git

and AFAICT this file doesn't exist in any upstream branches.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/2] ndctl: provide a method to invalidate the bus list

[Dan Williams]

On Thu, Mar 24, 2016 at 9:04 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Wed, Mar 23, 2016 at 06:16:53PM -0700, Dan Williams wrote:
>> Consider the following scenario:
>>
>>     ndctl_new()
>>     ndctl_bus_foreach()
>>     <install a new nvdimm-bus provider>
>>     ndctl_bus_foreach()
>>
>> ...the second ndctl_bus_foreach() will not see any new buses because the
>> bus results are cached.  When the holder of the context knows that it
>> has invalidated the bus list it needs to call ndctl_invalidate() so that
>> subsequent ndctl_bus_foreach() scans are guaranteed to see new buses.
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  lib/libndctl.c          |   22 ++++++++++++++++++----
>>  lib/libndctl.sym        |    1 +
>>  lib/ndctl/libndctl.h.in |    1 +
>
> I can't get this patch to apply because I don't have lib/ndctl/libndctl.h.in
> in my source tree.
>
> I'm using your upstream repo:
>
> https://github.com/pmem/ndctl.git
>
> and AFAICT this file doesn't exist in any upstream branches.

https://patchwork.kernel.org/patch/8584381/

I'll push this out to the -pending branch.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm