Misc. cleanups from #12928 pt. 2

[nabijaczleweli]

Motivation and Context

What I Noticed In #12928 (comment), Part Two. I just need to stop staring at the fucking thing before I go insane. As far as I can tell there's no leak. As far as I can tell, also

+53784 -53784  NO TPOOL
+53784 -53784  YES TPOOL, NO DISPATCH
+60960 -42944  YES TPOOL, YES DISPATCH
+53784 -53784  YES TPOOL, FAKE DISPATCH
+60960 -53912  YES TPOOL, YES DISPATCH, BUMP ALLOC
+60960 -42944  YES TPOOL, YES DISPATCH, NO BUMP ALLOC

somehow there is one. Barring something fundamental, I don't know what I could possibly be missing and why allocating in the other thread would lose this. See the VM link in that thread if you want to curse yourself with this.

Working diff on top of this, in case I want to come back to it.

diff --git a/lib/libnvpair/nvpair_alloc_system.c b/lib/libnvpair/nvpair_alloc_system.c
index 9771f58f6..5806a6283 100644
--- a/lib/libnvpair/nvpair_alloc_system.c
+++ b/lib/libnvpair/nvpair_alloc_system.c
@@ -29,17 +29,21 @@
 #include <rpc/types.h>
 #include <sys/kmem.h>
 #include <sys/nvpair.h>
+#include <stdio.h>
 
 static void *
 nv_alloc_sys(nv_alloc_t *nva, size_t size)
 {
-	return (kmem_alloc(size, (int)(uintptr_t)nva->nva_arg));
+	void * ret = (kmem_alloc(size, (int)(uintptr_t)nva->nva_arg));
+  printf("alloc: %p (%zu)\n", ret, size);
+  return ret;
 }
 
 static void
 nv_free_sys(nv_alloc_t *nva, void *buf, size_t size)
 {
 	(void) nva;
+  printf("free: %p (%zu)\n", buf, size);
 	kmem_free(buf, size);
 }
 
diff --git a/lib/libtpool/thread_pool.c b/lib/libtpool/thread_pool.c
index 58b706dde..b3ff14d31 100644
--- a/lib/libtpool/thread_pool.c
+++ b/lib/libtpool/thread_pool.c
@@ -399,7 +399,9 @@ tpool_create(uint_t min_threads, uint_t max_threads, uint_t linger,
 
 	return (tpool);
 }
-
+static void do_nothing(void *_) {
+	printf("do_nothing(%p)\n", _);
+}
 /*
  * Dispatch a work request to the thread pool.
  * If there are idle workers, awaken one.
@@ -419,6 +421,7 @@ tpool_dispatch(tpool_t *tpool, void (*func)(void *), void *arg)
 	job->tpj_next = NULL;
 	job->tpj_func = func;
 	job->tpj_arg = arg;
+	// job->tpj_func = do_nothing;
 
 	pthread_mutex_lock(&tpool->tp_mutex);
 
@@ -438,6 +441,7 @@ tpool_dispatch(tpool_t *tpool, void (*func)(void *), void *arg)
 	}
 
 	pthread_mutex_unlock(&tpool->tp_mutex);
+	// func(arg);
 	return (0);
 }
 
diff --git a/lib/libzfs/libzfs_mount.c b/lib/libzfs/libzfs_mount.c
index 8bde57051..e11224d7e 100644
--- a/lib/libzfs/libzfs_mount.c
+++ b/lib/libzfs/libzfs_mount.c
@@ -1267,6 +1267,7 @@ zfs_dispatch_mount(libzfs_handle_t *hdl, zfs_handle_t **handles,
 	mnt_param->mnt_data = data;
 
 	(void) tpool_dispatch(tp, zfs_mount_task, (void*)mnt_param);
+	// zfs_mount_task(mnt_param);
 }
 
 /*
@@ -1334,6 +1335,9 @@ zfs_mount_task(void *arg)
 	verify(zfs_prop_get(handles[idx], ZFS_PROP_MOUNTPOINT, mountpoint,
 	    sizeof (mountpoint), NULL, NULL, 0, B_FALSE) == 0);
 
+// printf("skipping: %p (%d): %s\n", handles[idx], idx, mountpoint);
+// goto out;
+printf("not-skipping: %p (%d): %s\n", handles[idx], idx, mountpoint);
 	if (mp->mnt_func(handles[idx], mp->mnt_data) != 0)
 		goto out;
 
@@ -1410,6 +1414,7 @@ zfs_foreach_mountpoint(libzfs_handle_t *hdl, zfs_handle_t **handles,
 	 * Issue the callback function for each dataset using a parallel
 	 * algorithm that uses a thread pool to manage threads.
 	 */
+	// tpool_t *tp = NULL;
 	tpool_t *tp = tpool_create(1, mount_tp_nthr, 0, NULL);
 
 	/*
diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c
index 88c7ee74b..5aeac1ebb 100644
--- a/lib/libzfs/libzfs_pool.c
+++ b/lib/libzfs/libzfs_pool.c
@@ -84,36 +84,36 @@ zpool_get_all_props(zpool_handle_t *zhp)
 
 	while (zfs_ioctl(hdl, ZFS_IOC_POOL_GET_PROPS, &zc) != 0) {
 		if (errno == ENOMEM) {
-			if (zcmd_expand_dst_nvlist(hdl, &zc) != 0) {
-				zcmd_free_nvlists(&zc);
-				return (-1);
-			}
-		} else {
-			zcmd_free_nvlists(&zc);
-			return (-1);
-		}
+			if (zcmd_expand_dst_nvlist(hdl, &zc) != 0)
+				goto err;
+		} else
+			goto err;
 	}
 
-	if (zcmd_read_dst_nvlist(hdl, &zc, &zhp->zpool_props) != 0) {
-		zcmd_free_nvlists(&zc);
-		return (-1);
-	}
+printf("reE: before: %p\n", zhp->zpool_props);
+	if (zcmd_read_dst_nvlist(hdl, &zc, &zhp->zpool_props) != 0)
+		goto err;
+printf("reE: after : %p\n", zhp->zpool_props);
 
+printf("ok : free: %p\n", &zc.zc_nvlist_dst);
 	zcmd_free_nvlists(&zc);
-
 	return (0);
+err:
+printf("err: free: %p\n", &zc.zc_nvlist_dst);
+	zcmd_free_nvlists(&zc);
+	return (-1);
 }
 
 int
 zpool_props_refresh(zpool_handle_t *zhp)
 {
-	nvlist_t *old_props;
-
-	old_props = zhp->zpool_props;
+	nvlist_t *old_props = zhp->zpool_props;
 
+printf("in : refr: %p\n", old_props);
 	if (zpool_get_all_props(zhp) != 0)
 		return (-1);
 
+printf("out: refr: %p\n", old_props);
 	nvlist_free(old_props);
 	return (0);
 }
@@ -151,9 +151,9 @@ zpool_get_prop_int(zpool_handle_t *zhp, zpool_prop_t prop, zprop_source_t *src)
 	uint64_t value;
 	zprop_source_t source;
 
-	if (zhp->zpool_props == NULL && zpool_get_all_props(zhp)) {
+	if (zhp->zpool_props == NULL && zpool_props_refresh(zhp)) {
 		/*
-		 * zpool_get_all_props() has most likely failed because
+		 * zpool_props_refresh() has most likely failed because
 		 * the pool is faulted, but if all we need is the top level
 		 * vdev's guid then get it from the zhp config nvlist.
 		 */
diff --git a/module/nvpair/nvpair.c b/module/nvpair/nvpair.c
index 8230cca20..365a1d984 100644
--- a/module/nvpair/nvpair.c
+++ b/module/nvpair/nvpair.c
@@ -598,14 +598,17 @@ nvlist_xalloc(nvlist_t **nvlp, uint_t nvflag, nv_alloc_t *nva)
 	if (nvlp == NULL || nva == NULL)
 		return (EINVAL);
 
+// printf("xalloc: pre: %p: %p\n", nvlp, *nvlp);
 	if ((priv = nv_priv_alloc(nva)) == NULL)
 		return (ENOMEM);
+// printf("xalloc: pri: %p: %p; priv=%p\n", nvlp, *nvlp, priv);
 
 	if ((*nvlp = nv_mem_zalloc(priv,
 	    NV_ALIGN(sizeof (nvlist_t)))) == NULL) {
 		nv_mem_free(priv, priv, sizeof (nvpriv_t));
 		return (ENOMEM);
 	}
+printf("xalloc: %p, %p\n", *nvlp, priv);
 
 	nvlist_init(*nvlp, nvflag, priv);
 

Description

See individual commits.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes. – none apply
• I have run the ZFS Test Suite with this change applied. – CI take my hand
• All commit messages are properly formatted and contain Signed-off-by.

[nabijaczleweli]

Week bump. This is all-round just good, clean, uncontroversial christian fun :)

[behlendorf]

The cleanup here looks nice and is straight forward, but just to be absolutely clear, only commit 54bb000 is relevant to resolve the originally reported leak. And it does fix it? Right?

[nabijaczleweli]

Love your optimism, but, no. It's irrelevant and doesn't.