From 4d1b70175cf9bdf927351b1188215604d455f669 Mon Sep 17 00:00:00 2001 From: Laura Hild Date: Mon, 11 Sep 2023 17:58:19 -0400 Subject: [PATCH 01/18] Remove implication that child `disk`s aren't vdevs in zpoolconcepts(7) Reviewed-by: Brian Behlendorf Signed-off-by: Laura Hild Closes #15247 --- man/man7/zpoolconcepts.7 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/man/man7/zpoolconcepts.7 b/man/man7/zpoolconcepts.7 index db3fd492623..98f3ee7cd66 100644 --- a/man/man7/zpoolconcepts.7 +++ b/man/man7/zpoolconcepts.7 @@ -203,11 +203,9 @@ For more information, see the section. .El .Pp -Virtual devices cannot be nested, so a mirror or raidz virtual device can only -contain files or disks. -Mirrors of mirrors -.Pq or other combinations -are not allowed. +Virtual devices cannot be nested arbitrarily. +A mirror, raidz or draid virtual device can only be created with files or disks. +Mirrors of mirrors or other such combinations are not allowed. .Pp A pool can have any number of virtual devices at the top of the configuration .Po known as From 8af8d2abb1ee928beea16e12390dd7b8cf76244f Mon Sep 17 00:00:00 2001 From: Tony Hutter Date: Tue, 12 Sep 2023 12:51:11 -0700 Subject: [PATCH 02/18] Linux 6.5 compat: META (#15265) Update the META file to reflect compatibility with the 6.5 kernel. Signed-off-by: Tony Hutter Reviewed-by: Brian Behlendorf --- META | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/META b/META index 3919b0df468..15d4b19fdc4 100644 --- a/META +++ b/META @@ -6,5 +6,5 @@ Release: 1 Release-Tags: relext License: CDDL Author: OpenZFS -Linux-Maximum: 6.4 +Linux-Maximum: 6.5 Linux-Minimum: 3.10 From 9192ab7777fb23458e0f83fbc911d0fcc2fbba9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=BD=D0=B0=D0=B1?= Date: Mon, 18 Sep 2023 18:08:41 +0200 Subject: [PATCH 03/18] check-zstd-symbols: also ignore __pfx_ symbols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b341b20d648bb7e9a3307c33163e7399f0913e66 Reviewed-by: Matthew Ahrens Reviewed-by: Brian Behlendorf Signed-off-by: Ahelenia Ziemiańska Closes #15282 Closes #15284 --- module/Makefile.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Makefile.in b/module/Makefile.in index 5b71e1abf79..9b34b3dfaec 100644 --- a/module/Makefile.in +++ b/module/Makefile.in @@ -168,4 +168,4 @@ gen-zstd-symbols: for obj in $(addprefix zstd/,$(ZSTD_UPSTREAM_OBJS)); do echo; echo "/* $${obj#zstd/}: */"; @OBJDUMP@ -t $$obj | awk '$$2 == "g" && !/ zfs_/ {print "#define\t" $$6 " zfs_" $$6}' | sort; done >> zstd/include/zstd_compat_wrapper.h check-zstd-symbols: - @OBJDUMP@ -t $(addprefix zstd/,$(ZSTD_UPSTREAM_OBJS)) | awk '/file format/ {print} $$2 == "g" && !/ zfs_/ {++ret; print} END {exit ret}' + @OBJDUMP@ -t $(addprefix zstd/,$(ZSTD_UPSTREAM_OBJS)) | awk '/file format/ {print} $$2 == "g" && (!/ zfs_/ && !/ __pfx_zfs_/) {++ret; print} END {exit ret}' From 529bec7d7ba7ed0799476d0e9678d070b1b73be4 Mon Sep 17 00:00:00 2001 From: Tony Hutter Date: Mon, 18 Sep 2023 16:25:58 -0700 Subject: [PATCH 04/18] zed: Allow autoreplace and fault LEDs for removed vdevs Allow zed to autoreplace vdevs marked as REMOVED. Also update statechange-led zedlet to toggle fault LEDs for REMOVED vdevs. Reviewed-by: Brian Behlendorf Signed-off-by: Tony Hutter Closes #15281 --- cmd/zed/agents/zfs_mod.c | 1 + cmd/zed/zed.d/statechange-led.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/zed/agents/zfs_mod.c b/cmd/zed/agents/zfs_mod.c index a8d084bb4bd..2f040ff7582 100644 --- a/cmd/zed/agents/zfs_mod.c +++ b/cmd/zed/agents/zfs_mod.c @@ -372,6 +372,7 @@ zfs_process_add(zpool_handle_t *zhp, nvlist_t *vdev, boolean_t labeled) /* Only autoreplace bad disks */ if ((vs->vs_state != VDEV_STATE_DEGRADED) && (vs->vs_state != VDEV_STATE_FAULTED) && + (vs->vs_state != VDEV_STATE_REMOVED) && (vs->vs_state != VDEV_STATE_CANT_OPEN)) { zed_log_msg(LOG_INFO, " not autoreplacing since disk isn't in " "a bad state (currently %llu)", vs->vs_state); diff --git a/cmd/zed/zed.d/statechange-led.sh b/cmd/zed/zed.d/statechange-led.sh index 46bfc1b866f..40cb61f1730 100755 --- a/cmd/zed/zed.d/statechange-led.sh +++ b/cmd/zed/zed.d/statechange-led.sh @@ -121,7 +121,7 @@ state_to_val() { state="$1" case "$state" in - FAULTED|DEGRADED|UNAVAIL) + FAULTED|DEGRADED|UNAVAIL|REMOVED) echo 1 ;; ONLINE) From ee720ad7bca88997dd92370c295723a0f1aca3b2 Mon Sep 17 00:00:00 2001 From: Mateusz Guzik Date: Tue, 19 Sep 2023 01:53:33 +0200 Subject: [PATCH 05/18] Retire z_nr_znodes Added in ab26409db753 ("Linux 3.1 compat, super_block->s_shrink"), with the only consumer which needed the count getting retired in 066e82522101 ("Linux compat: Minimum kernel version 3.10"). The counter gets in the way of not maintaining the list to begin with. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Mateusz Guzik Closes #15274 --- include/os/freebsd/zfs/sys/zfs_vfsops_os.h | 1 - include/os/linux/zfs/sys/zfs_vfsops_os.h | 1 - module/os/freebsd/zfs/zfs_vfsops.c | 8 +++----- module/os/freebsd/zfs/zfs_znode.c | 2 -- module/os/linux/zfs/zfs_ctldir.c | 1 - module/os/linux/zfs/zfs_vfsops.c | 7 +++---- module/os/linux/zfs/zfs_znode.c | 2 -- 7 files changed, 6 insertions(+), 16 deletions(-) diff --git a/include/os/freebsd/zfs/sys/zfs_vfsops_os.h b/include/os/freebsd/zfs/sys/zfs_vfsops_os.h index f765d38dbac..24bb03575f3 100644 --- a/include/os/freebsd/zfs/sys/zfs_vfsops_os.h +++ b/include/os/freebsd/zfs/sys/zfs_vfsops_os.h @@ -93,7 +93,6 @@ struct zfsvfs { zfs_teardown_lock_t z_teardown_lock; zfs_teardown_inactive_lock_t z_teardown_inactive_lock; list_t z_all_znodes; /* all vnodes in the fs */ - uint64_t z_nr_znodes; /* number of znodes in the fs */ kmutex_t z_znodes_lock; /* lock for z_all_znodes */ struct zfsctl_root *z_ctldir; /* .zfs directory pointer */ boolean_t z_show_ctldir; /* expose .zfs in the root dir */ diff --git a/include/os/linux/zfs/sys/zfs_vfsops_os.h b/include/os/linux/zfs/sys/zfs_vfsops_os.h index e320b8de422..b4d5db21f5e 100644 --- a/include/os/linux/zfs/sys/zfs_vfsops_os.h +++ b/include/os/linux/zfs/sys/zfs_vfsops_os.h @@ -105,7 +105,6 @@ struct zfsvfs { rrmlock_t z_teardown_lock; krwlock_t z_teardown_inactive_lock; list_t z_all_znodes; /* all znodes in the fs */ - uint64_t z_nr_znodes; /* number of znodes in the fs */ unsigned long z_rollback_time; /* last online rollback time */ unsigned long z_snap_defer_time; /* last snapshot unmount deferral */ kmutex_t z_znodes_lock; /* lock for z_all_znodes */ diff --git a/module/os/freebsd/zfs/zfs_vfsops.c b/module/os/freebsd/zfs/zfs_vfsops.c index 33759fa2616..e8b9ada1316 100644 --- a/module/os/freebsd/zfs/zfs_vfsops.c +++ b/module/os/freebsd/zfs/zfs_vfsops.c @@ -1154,7 +1154,6 @@ zfsvfs_free(zfsvfs_t *zfsvfs) mutex_destroy(&zfsvfs->z_znodes_lock); mutex_destroy(&zfsvfs->z_lock); - ASSERT3U(zfsvfs->z_nr_znodes, ==, 0); list_destroy(&zfsvfs->z_all_znodes); ZFS_TEARDOWN_DESTROY(zfsvfs); ZFS_TEARDOWN_INACTIVE_DESTROY(zfsvfs); @@ -1558,12 +1557,11 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolean_t unmounting) * may add the parents of dir-based xattrs to the taskq * so we want to wait for these. * - * We can safely read z_nr_znodes without locking because the - * VFS has already blocked operations which add to the - * z_all_znodes list and thus increment z_nr_znodes. + * We can safely check z_all_znodes for being empty because the + * VFS has already blocked operations which add to it. */ int round = 0; - while (zfsvfs->z_nr_znodes > 0) { + while (!list_is_empty(&zfsvfs->z_all_znodes)) { taskq_wait_outstanding(dsl_pool_zrele_taskq( dmu_objset_pool(zfsvfs->z_os)), 0); if (++round > 1 && !unmounting) diff --git a/module/os/freebsd/zfs/zfs_znode.c b/module/os/freebsd/zfs/zfs_znode.c index c4f2b722ef4..0d4c94555c6 100644 --- a/module/os/freebsd/zfs/zfs_znode.c +++ b/module/os/freebsd/zfs/zfs_znode.c @@ -537,7 +537,6 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz, mutex_enter(&zfsvfs->z_znodes_lock); list_insert_tail(&zfsvfs->z_all_znodes, zp); - zfsvfs->z_nr_znodes++; zp->z_zfsvfs = zfsvfs; mutex_exit(&zfsvfs->z_znodes_lock); @@ -1286,7 +1285,6 @@ zfs_znode_free(znode_t *zp) mutex_enter(&zfsvfs->z_znodes_lock); POINTER_INVALIDATE(&zp->z_zfsvfs); list_remove(&zfsvfs->z_all_znodes, zp); - zfsvfs->z_nr_znodes--; mutex_exit(&zfsvfs->z_znodes_lock); #if __FreeBSD_version >= 1300139 diff --git a/module/os/linux/zfs/zfs_ctldir.c b/module/os/linux/zfs/zfs_ctldir.c index c45a3eb5a4e..02cb379ea84 100644 --- a/module/os/linux/zfs/zfs_ctldir.c +++ b/module/os/linux/zfs/zfs_ctldir.c @@ -537,7 +537,6 @@ zfsctl_inode_alloc(zfsvfs_t *zfsvfs, uint64_t id, mutex_enter(&zfsvfs->z_znodes_lock); list_insert_tail(&zfsvfs->z_all_znodes, zp); - zfsvfs->z_nr_znodes++; membar_producer(); mutex_exit(&zfsvfs->z_znodes_lock); diff --git a/module/os/linux/zfs/zfs_vfsops.c b/module/os/linux/zfs/zfs_vfsops.c index 464c12e1108..a1db5c57c18 100644 --- a/module/os/linux/zfs/zfs_vfsops.c +++ b/module/os/linux/zfs/zfs_vfsops.c @@ -1330,12 +1330,11 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolean_t unmounting) * may add the parents of dir-based xattrs to the taskq * so we want to wait for these. * - * We can safely read z_nr_znodes without locking because the - * VFS has already blocked operations which add to the - * z_all_znodes list and thus increment z_nr_znodes. + * We can safely check z_all_znodes for being empty because the + * VFS has already blocked operations which add to it. */ int round = 0; - while (zfsvfs->z_nr_znodes > 0) { + while (!list_is_empty(&zfsvfs->z_all_znodes)) { taskq_wait_outstanding(dsl_pool_zrele_taskq( dmu_objset_pool(zfsvfs->z_os)), 0); if (++round > 1 && !unmounting) diff --git a/module/os/linux/zfs/zfs_znode.c b/module/os/linux/zfs/zfs_znode.c index 335ae3460c5..52c8e51df65 100644 --- a/module/os/linux/zfs/zfs_znode.c +++ b/module/os/linux/zfs/zfs_znode.c @@ -390,7 +390,6 @@ zfs_inode_destroy(struct inode *ip) mutex_enter(&zfsvfs->z_znodes_lock); if (list_link_active(&zp->z_link_node)) { list_remove(&zfsvfs->z_all_znodes, zp); - zfsvfs->z_nr_znodes--; } mutex_exit(&zfsvfs->z_znodes_lock); @@ -641,7 +640,6 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz, mutex_enter(&zfsvfs->z_znodes_lock); list_insert_tail(&zfsvfs->z_all_znodes, zp); - zfsvfs->z_nr_znodes++; mutex_exit(&zfsvfs->z_znodes_lock); if (links > 0) From e923bcd16c33d6a83712b08c7854f0c3edb62172 Mon Sep 17 00:00:00 2001 From: George Amanakis Date: Tue, 19 Sep 2023 02:06:35 +0200 Subject: [PATCH 06/18] Update the MOS directory on spa_upgrade_errlog() spa_upgrade_errlog() does not update the MOS directory when the head_errlog feature is enabled. In this case if spa_errlog_sync() is not called, the MOS dir references the old errlog_last and errlog_sync objects. Thus when doing a scrub a panic will occur: Call Trace: dump_stack+0x6d/0x8b panic+0x101/0x2e3 spl_panic+0xcf/0x102 [spl] delete_errlog+0x124/0x130 [zfs] spa_errlog_sync+0x256/0x260 [zfs] spa_sync_iterate_to_convergence+0xe5/0x250 [zfs] spa_sync+0x2f7/0x670 [zfs] txg_sync_thread+0x22d/0x2d0 [zfs] thread_generic_wrapper+0x83/0xa0 [spl] kthread+0x104/0x140 ret_from_fork+0x1f/0x40 Fix this by updating the related MOS directory objects in spa_upgrade_errlog(). Reviewed-by: Mark Maybee Reviewed-by: Brian Behlendorf Signed-off-by: George Amanakis Closes #15279 Closes #15277 --- module/zfs/spa_errlog.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/module/zfs/spa_errlog.c b/module/zfs/spa_errlog.c index 2e5c22c1149..5dd08f597f3 100644 --- a/module/zfs/spa_errlog.c +++ b/module/zfs/spa_errlog.c @@ -930,12 +930,21 @@ spa_upgrade_errlog(spa_t *spa, dmu_tx_t *tx) if (spa->spa_errlog_last != 0) { sync_upgrade_errlog(spa, spa->spa_errlog_last, &newobj, tx); spa->spa_errlog_last = newobj; + + (void) zap_update(spa->spa_meta_objset, + DMU_POOL_DIRECTORY_OBJECT, DMU_POOL_ERRLOG_LAST, + sizeof (uint64_t), 1, &spa->spa_errlog_last, tx); } if (spa->spa_errlog_scrub != 0) { sync_upgrade_errlog(spa, spa->spa_errlog_scrub, &newobj, tx); spa->spa_errlog_scrub = newobj; + + (void) zap_update(spa->spa_meta_objset, + DMU_POOL_DIRECTORY_OBJECT, DMU_POOL_ERRLOG_SCRUB, + sizeof (uint64_t), 1, &spa->spa_errlog_scrub, tx); } + mutex_exit(&spa->spa_errlog_lock); } From 6cb933c56ea6e865adfa37c394a77e3866f41dfb Mon Sep 17 00:00:00 2001 From: Rob N Date: Wed, 20 Sep 2023 01:48:02 +1000 Subject: [PATCH 07/18] tests: install missing PAM tests 'pam_change_unmounted' and 'pam_recursive' both exist and are referenced by the test run config, but weren't being installed and so are excluded. This gets them installed so they will run as expected. Reviewed-by: Brian Behlendorf Reviewed-by: Kay Pedersen Signed-off-by: Rob Norris Closes #15291 --- tests/zfs-tests/tests/Makefile.am | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index 66aff5026f8..1a58e6f774e 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1588,7 +1588,9 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/online_offline/setup.ksh \ functional/pam/cleanup.ksh \ functional/pam/pam_basic.ksh \ + functional/pam/pam_change_unmounted.ksh \ functional/pam/pam_nounmount.ksh \ + functional/pam/pam_recursive.ksh \ functional/pam/pam_short_password.ksh \ functional/pam/setup.ksh \ functional/pool_checkpoint/checkpoint_after_rewind.ksh \ From 741c215bab32a0a823572769c3560729c78390ea Mon Sep 17 00:00:00 2001 From: Paul Dagnelie Date: Tue, 19 Sep 2023 08:58:14 -0700 Subject: [PATCH 08/18] Fix l2arc_apply_transforms ztest crash In #13375 we modified the allocation size of the buffer that we use to apply l2arc transforms to be the size of the arc hdr we're using, rather than the allocation size that will be in place on the disk, because sometimes the hdr size is larger. Unfortunately, sometimes the allocation size is larger, which means that we overflow the buffer in that case. This change modifies the allocation to be the max of the two values Reviewed-by: Mark Maybee Reviewed-by: Brian Behlendorf Signed-off-by: Paul Dagnelie Closes #15177 Closes #15248 --- module/zfs/arc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/module/zfs/arc.c b/module/zfs/arc.c index 7023f448182..22dc0ed5e3b 100644 --- a/module/zfs/arc.c +++ b/module/zfs/arc.c @@ -9092,15 +9092,16 @@ l2arc_apply_transforms(spa_t *spa, arc_buf_hdr_t *hdr, uint64_t asize, * write things before deciding to fail compression in nearly * every case.) */ - cabd = abd_alloc_for_io(size, ismd); - tmp = abd_borrow_buf(cabd, size); + uint64_t bufsize = MAX(size, asize); + cabd = abd_alloc_for_io(bufsize, ismd); + tmp = abd_borrow_buf(cabd, bufsize); psize = zio_compress_data(compress, to_write, &tmp, size, hdr->b_complevel); if (psize >= asize) { psize = HDR_GET_PSIZE(hdr); - abd_return_buf_copy(cabd, tmp, size); + abd_return_buf_copy(cabd, tmp, bufsize); HDR_SET_COMPRESS(hdr, ZIO_COMPRESS_OFF); to_write = cabd; abd_copy(to_write, hdr->b_l1hdr.b_pabd, psize); @@ -9110,9 +9111,9 @@ l2arc_apply_transforms(spa_t *spa, arc_buf_hdr_t *hdr, uint64_t asize, } ASSERT3U(psize, <=, HDR_GET_PSIZE(hdr)); if (psize < asize) - memset((char *)tmp + psize, 0, asize - psize); + memset((char *)tmp + psize, 0, bufsize - psize); psize = HDR_GET_PSIZE(hdr); - abd_return_buf_copy(cabd, tmp, size); + abd_return_buf_copy(cabd, tmp, bufsize); to_write = cabd; } From 2076011e0c4c2d8ad6a59534a4784a6aa5f4f3df Mon Sep 17 00:00:00 2001 From: Paul Dagnelie Date: Tue, 19 Sep 2023 09:02:23 -0700 Subject: [PATCH 09/18] Fix incorrect expected error in ztest There is an occasional ztest failure that looks like ztest: attach (/var/tmp/zloop-run/ztest.13a 570425344, draid1-1-0 532152320, 1) returned 22, expected 95. This is because the value that we return is EINVAL, but expected_error is set incorrectly. Change the expected_error value to match both the comment and the actual error value. Reviewed-by: Brian Behlendorf Signed-off-by: Paul Dagnelie Closes #15295 --- cmd/ztest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/ztest.c b/cmd/ztest.c index 398c519cfc3..59c4be225f9 100644 --- a/cmd/ztest.c +++ b/cmd/ztest.c @@ -3767,7 +3767,7 @@ ztest_vdev_attach_detach(ztest_ds_t *zd, uint64_t id) else if (ashift > oldvd->vdev_top->vdev_ashift) expected_error = EDOM; else if (newvd_is_dspare && pvd != vdev_draid_spare_get_parent(newvd)) - expected_error = ENOTSUP; + expected_error = EINVAL; else expected_error = 0; From 7228ba1114024bb3564e272dded995cf1dd3efb4 Mon Sep 17 00:00:00 2001 From: Rob N Date: Wed, 20 Sep 2023 02:06:47 +1000 Subject: [PATCH 10/18] cmd: add 'help' subcommand to zpool and zfs 'program help subcommand' is a reasonably common pattern for multifunction command-line programs. This commit adds support for that style to the zpool and zfs commands. When run as 'zpool help []' or 'zfs help []', executes the 'man' program on the PATH with the most likely manpage name for the requested topic: "zpool-" or "zfs-" for subcommands, or "zpool" or "zfs" for the "concepts" and "props" topics. If no topic is supplied, uses the top "zpool" or "zfs" pages. Reviewed-by: Brian Behlendorf Reviewed-by: Kay Pedersen Signed-off-by: Rob Norris Closes #15288 --- cmd/zfs/zfs_main.c | 30 ++++++++++++++++++ cmd/zpool/zpool_main.c | 31 +++++++++++++++++++ .../cli_root/zfs_program/zfs_program_json.ksh | 4 ++- 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index ea73bb018a9..629e048872b 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -132,6 +132,8 @@ static int zfs_do_zone(int argc, char **argv); static int zfs_do_unzone(int argc, char **argv); #endif +static int zfs_do_help(int argc, char **argv); + /* * Enable a reasonable set of defaults for libumem debugging on DEBUG builds. */ @@ -606,6 +608,9 @@ usage(boolean_t requested) (void) fprintf(fp, gettext("\nFor the delegated permission list, run: %s\n"), "zfs allow|unallow"); + (void) fprintf(fp, + gettext("\nFor further help on a command or topic, " + "run: %s\n"), "zfs help []"); } /* @@ -8730,6 +8735,25 @@ zfs_do_version(int argc, char **argv) return (zfs_version_print() != 0); } +/* Display documentation */ +static int +zfs_do_help(int argc, char **argv) +{ + char page[MAXNAMELEN]; + if (argc < 3 || strcmp(argv[2], "zfs") == 0) + strcpy(page, "zfs"); + else if (strcmp(argv[2], "concepts") == 0 || + strcmp(argv[2], "props") == 0) + snprintf(page, sizeof (page), "zfs%s", argv[2]); + else + snprintf(page, sizeof (page), "zfs-%s", argv[2]); + + execlp("man", "man", page, NULL); + + fprintf(stderr, "couldn't run man program: %s", strerror(errno)); + return (-1); +} + int main(int argc, char **argv) { @@ -8785,6 +8809,12 @@ main(int argc, char **argv) if ((strcmp(cmdname, "-V") == 0) || (strcmp(cmdname, "--version") == 0)) return (zfs_do_version(argc, argv)); + /* + * Special case 'help' + */ + if (strcmp(cmdname, "help") == 0) + return (zfs_do_help(argc, argv)); + if ((g_zfs = libzfs_init()) == NULL) { (void) fprintf(stderr, "%s\n", libzfs_error_init(errno)); return (1); diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c index 6d0dae8d8b0..d64fdfa5ba4 100644 --- a/cmd/zpool/zpool_main.c +++ b/cmd/zpool/zpool_main.c @@ -126,6 +126,8 @@ static int zpool_do_version(int, char **); static int zpool_do_wait(int, char **); +static int zpool_do_help(int argc, char **argv); + static zpool_compat_status_t zpool_do_load_compat( const char *, boolean_t *); @@ -538,6 +540,10 @@ usage(boolean_t requested) (void) fprintf(fp, "%s", get_usage(command_table[i].usage)); } + + (void) fprintf(fp, + gettext("\nFor further help on a command or topic, " + "run: %s\n"), "zpool help []"); } else { (void) fprintf(fp, gettext("usage:\n")); (void) fprintf(fp, "%s", get_usage(current_command->usage)); @@ -11051,6 +11057,25 @@ zpool_do_version(int argc, char **argv) return (zfs_version_print() != 0); } +/* Display documentation */ +static int +zpool_do_help(int argc, char **argv) +{ + char page[MAXNAMELEN]; + if (argc < 3 || strcmp(argv[2], "zpool") == 0) + strcpy(page, "zpool"); + else if (strcmp(argv[2], "concepts") == 0 || + strcmp(argv[2], "props") == 0) + snprintf(page, sizeof (page), "zpool%s", argv[2]); + else + snprintf(page, sizeof (page), "zpool-%s", argv[2]); + + execlp("man", "man", page, NULL); + + fprintf(stderr, "couldn't run man program: %s", strerror(errno)); + return (-1); +} + /* * Do zpool_load_compat() and print error message on failure */ @@ -11118,6 +11143,12 @@ main(int argc, char **argv) if ((strcmp(cmdname, "-V") == 0) || (strcmp(cmdname, "--version") == 0)) return (zpool_do_version(argc, argv)); + /* + * Special case 'help' + */ + if (strcmp(cmdname, "help") == 0) + return (zpool_do_help(argc, argv)); + if ((g_zfs = libzfs_init()) == NULL) { (void) fprintf(stderr, "%s\n", libzfs_error_init(errno)); return (1); diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_program/zfs_program_json.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_program/zfs_program_json.ksh index b0265c5ee4a..2241b77bf80 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_program/zfs_program_json.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_program/zfs_program_json.ksh @@ -117,7 +117,9 @@ usage: For the property list, run: zfs set|get -For the delegated permission list, run: zfs allow|unallow") +For the delegated permission list, run: zfs allow|unallow + +For further help on a command or topic, run: zfs help []") cnt=0 for cmd in ${neg_cmds[@]}; do log_mustnot zfs program $cmd $TESTPOOL $TESTZCP $TESTDS 2>&1 From bbac1d2977ec245ec063313c97ffc2356be07c20 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:27:53 +0500 Subject: [PATCH 11/18] Update the behavior of mountpoint property There are some inconsistencies in the handling of mountpoint property. This commit updates the behavior and makes it consistent. If mountpoint property is set when dataset is unmounted, this would update the mountpoint property. The mountpoint could be valid or invalid in this case. Setting the mountpoint property would result in success in this case. Dataset would still be unmounted here. On the other hand, if dataset is mounted and mountpoint property is updated to something invalid where mount cannot be successful, for example, setting the mountpoint inside a readonly directory. This would unmount the dataset, set the mountpoint property to requested value and tries to mount the dataset. The mount operation returns error and this error is treated as overall failure of setting the property while the property is actually set. To make the behavior consistent in case dataset is mounted or unmounted, we should try to mount the dataset whenever mountpoint property is updated. This would result in mounting the datasets if canmount property is set to on, regardless if the dataset was previously unmounted. The failure in mount operation while setting the mountpoint property should not be treated as failure, since the property is actually set now to user requested value. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Reviewed-by: Ameer Hamza Signed-off-by: Umer Saleem Closes #15240 --- cmd/zfs/zfs_main.c | 7 ++++--- lib/libzfs/libzfs_changelist.c | 8 ++++---- .../cli_root/zfs_mount/zfs_mount_006_pos.ksh | 2 +- .../cli_root/zfs_mount/zfs_mount_008_pos.ksh | 4 ++-- .../cli_root/zfs_mount/zfs_mount_012_pos.ksh | 15 +++++++-------- .../cli_root/zfs_set/mountpoint_002_pos.ksh | 12 +++++++++--- .../cli_root/zfs_set/zfs_set_003_neg.ksh | 10 +++++++--- 7 files changed, 34 insertions(+), 24 deletions(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 629e048872b..45cd2998d6e 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -4207,8 +4207,9 @@ static int set_callback(zfs_handle_t *zhp, void *data) { nvlist_t *props = data; + int ret = zfs_prop_set_list(zhp, props); - if (zfs_prop_set_list(zhp, props) != 0) { + if (ret != 0 || libzfs_errno(g_zfs) != EZFS_SUCCESS) { switch (libzfs_errno(g_zfs)) { case EZFS_MOUNTFAILED: (void) fprintf(stderr, gettext("property may be set " @@ -4217,11 +4218,11 @@ set_callback(zfs_handle_t *zhp, void *data) case EZFS_SHARENFSFAILED: (void) fprintf(stderr, gettext("property may be set " "but unable to reshare filesystem\n")); + ret = 1; break; } - return (1); } - return (0); + return (ret); } static int diff --git a/lib/libzfs/libzfs_changelist.c b/lib/libzfs/libzfs_changelist.c index dd14c570ec0..4b0f6696434 100644 --- a/lib/libzfs/libzfs_changelist.c +++ b/lib/libzfs/libzfs_changelist.c @@ -244,13 +244,13 @@ changelist_postfix(prop_changelist_t *clp) zfs_is_mounted(cn->cn_handle, NULL); if (!mounted && !needs_key && (cn->cn_mounted || - ((sharenfs || sharesmb || clp->cl_waslegacy) && + (((clp->cl_prop == ZFS_PROP_MOUNTPOINT && + clp->cl_prop == clp->cl_realprop) || + sharenfs || sharesmb || clp->cl_waslegacy) && (zfs_prop_get_int(cn->cn_handle, ZFS_PROP_CANMOUNT) == ZFS_CANMOUNT_ON)))) { - if (zfs_mount(cn->cn_handle, NULL, 0) != 0) - errors++; - else + if (zfs_mount(cn->cn_handle, NULL, 0) == 0) mounted = TRUE; } diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh index 2a2466f65c0..e9ab472795e 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh @@ -94,7 +94,7 @@ while (( depth < MAXDEPTH )); do done log_must zfs set mountpoint=$mtpt $TESTPOOL/$TESTFS -log_must zfs $mountcmd $TESTPOOL/$TESTFS +log_must ismounted $TESTPOOL/$TESTFS log_must zfs set overlay=off $TESTPOOL/$TESTFS if ! is_illumos; then diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh index 2c1029d551c..0437c61a2c4 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh @@ -71,7 +71,7 @@ log_must mkfile 1M $testfile $testfile1 log_must zfs unmount $fs1 log_must zfs set mountpoint=$mntpnt $fs1 -log_must zfs mount $fs1 +log_must ismounted $fs1 log_must zfs unmount $fs1 log_must zfs mount -O $fs1 @@ -85,7 +85,7 @@ log_must ls $mntpnt/$TESTFILE1 $mntpnt/$TESTFILE2 # Verify $TESTFILE2 was created in $fs1, rather than $fs log_must zfs unmount $fs1 log_must zfs set mountpoint=$mntpnt1 $fs1 -log_must zfs mount $fs1 +log_must ismounted $fs1 log_must ls $testfile1 $mntpnt1/$TESTFILE2 # Verify $TESTFILE2 was not created in $fs, and $fs is accessible again. diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh index 5ff094d2c47..66958f2f088 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh @@ -25,13 +25,12 @@ # STRATEGY: # 1. Unmount the dataset # 2. Create a new empty directory -# 3. Set the dataset's mountpoint -# 4. Attempt to mount the dataset -# 5. Verify the mount succeeds -# 6. Unmount the dataset -# 7. Create a file in the directory created in step 2 -# 8. Attempt to mount the dataset -# 9. Verify the mount succeeds +# 3. Set the dataset's mountpoint, this should mount the dataset +# 4. Verify the mount succeeds +# 5. Unmount the dataset +# 6. Create a file in the directory created in step 2 +# 7. Attempt to mount the dataset +# 8. Verify the mount succeeds # verify_runnable "both" @@ -43,7 +42,7 @@ fs=$TESTPOOL/$TESTFS log_must zfs umount $fs log_must mkdir -p $TESTDIR log_must zfs set mountpoint=$TESTDIR $fs -log_must zfs mount $fs +log_must ismounted $fs log_must zfs umount $fs log_must touch $TESTDIR/testfile.$$ log_must zfs mount $fs diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh index a5785226e02..c227a6fb8aa 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh @@ -35,7 +35,9 @@ # # DESCRIPTION: # If ZFS is currently managing the file system but it is currently unmounted, -# and the mountpoint property is changed, the file system remains unmounted. +# and the mountpoint property is changed, the file system should be mounted +# if it is a valid mountpoint and canmount allows to mount, otherwise it +# should not be mounted. # # STRATEGY: # 1. Setup a pool and create fs, ctr within it. @@ -62,7 +64,7 @@ function cleanup } log_assert "Setting a valid mountpoint for an unmounted file system, \ - it remains unmounted." + it gets mounted." log_onexit cleanup old_fs_mpt=$(get_prop mountpoint $TESTPOOL/$TESTFS) @@ -83,7 +85,11 @@ while (( i < ${#dataset[@]} )); do while (( j < ${#values[@]} )); do set_n_check_prop "${values[j]}" "mountpoint" \ "${dataset[i]}" - log_mustnot ismounted ${dataset[i]} + if [ "${dataset[i]}" = "$TESTPOOL/$TESTFS" ]; then + log_must ismounted ${dataset[i]} + else + log_mustnot ismounted ${dataset[i]} + fi (( j += 1 )) done cleanup diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh index 3afb0eb7010..5901ba7dc46 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh @@ -33,7 +33,9 @@ # # DESCRIPTION: -# 'zfs set mountpoint/sharenfs' should fail when the mountpoint is invalid +# 'zfs set mountpoint/sharenfs' should set the property when mountpoint +# is invalid. Setting the property should be successful, but dataset +# should not be mounted, as mountpoint is invalid. # # STRATEGY: # 1. Create invalid scenarios @@ -62,10 +64,12 @@ longpath=$(gen_dataset_name 1030 "abcdefg") log_must zfs create -o mountpoint=legacy $TESTPOOL/foo # Do the negative testing about "property may be set but unable to remount filesystem" -log_mustnot eval "zfs set mountpoint=$badpath $TESTPOOL/foo >/dev/null 2>&1" +set_n_check_prop "$badpath" "mountpoint" "$TESTPOOL/foo" +log_mustnot ismounted $TESTPOOL/foo # Do the negative testing about "property may be set but unable to reshare filesystem" -log_mustnot eval "zfs set sharenfs=on $TESTPOOL/foo >/dev/null 2>&1" +set_n_check_prop "on" "sharenfs" "$TESTPOOL/foo" +log_mustnot ismounted $TESTPOOL/foo # Do the negative testing about "sharenfs property can not be set to null" log_mustnot eval "zfs set sharenfs= $TESTPOOL/foo >/dev/null 2>&1" From c63aabaf1c548248c8a5081f6014a472dc7c4570 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:33:58 +0500 Subject: [PATCH 12/18] Improve the handling of sharesmb,sharenfs properties For sharesmb and sharenfs properties, the status of setting the property is tied with whether we succeed to share the dataset or not. In case sharing the dataset is not successful, this is treated as overall failure of setting the property. In this case, if we check the property after the failure, it is set to on. This commit updates this behavior and the status of setting the share properties is not returned as failure, when we fail to share the dataset. For sharenfs property, if access list is provided, the syntax errors in access list/host adresses are not validated until after setting the property during postfix phase while trying to share the dataset. This is not correct, since the property has already been set when we reach there. Syntax errors in access list/host addresses are validated while validating the property list, before setting the property and failure is returned to user in this case when there are errors in access list. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Reviewed-by: Ameer Hamza Signed-off-by: Umer Saleem Closes #15240 --- cmd/zfs/zfs_main.c | 1 - lib/libshare/os/freebsd/nfs.c | 3 ++- lib/libshare/os/linux/nfs.c | 47 +++++++++++++++++++++++++++++++--- lib/libzfs/libzfs_changelist.c | 11 ++++---- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 45cd2998d6e..76c82fd5321 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -4218,7 +4218,6 @@ set_callback(zfs_handle_t *zhp, void *data) case EZFS_SHARENFSFAILED: (void) fprintf(stderr, gettext("property may be set " "but unable to reshare filesystem\n")); - ret = 1; break; } } diff --git a/lib/libshare/os/freebsd/nfs.c b/lib/libshare/os/freebsd/nfs.c index 521631c51f0..d9fc6610636 100644 --- a/lib/libshare/os/freebsd/nfs.c +++ b/lib/libshare/os/freebsd/nfs.c @@ -161,7 +161,8 @@ nfs_is_shared(sa_share_impl_t impl_share) static int nfs_validate_shareopts(const char *shareopts) { - (void) shareopts; + if (strlen(shareopts) == 0) + return (SA_SYNTAX_ERR); return (SA_OK); } diff --git a/lib/libshare/os/linux/nfs.c b/lib/libshare/os/linux/nfs.c index c27e5564c1e..004946b0cfe 100644 --- a/lib/libshare/os/linux/nfs.c +++ b/lib/libshare/os/linux/nfs.c @@ -319,12 +319,49 @@ get_linux_shareopts_cb(const char *key, const char *value, void *cookie) "wdelay" }; char **plinux_opts = (char **)cookie; + char *host, *val_dup, *literal, *next; - /* host-specific options, these are taken care of elsewhere */ - if (strcmp(key, "ro") == 0 || strcmp(key, "rw") == 0 || - strcmp(key, "sec") == 0) + if (strcmp(key, "sec") == 0) return (SA_OK); + if (strcmp(key, "ro") == 0 || strcmp(key, "rw") == 0) { + if (value == NULL || strlen(value) == 0) + return (SA_OK); + val_dup = strdup(value); + host = val_dup; + if (host == NULL) + return (SA_NO_MEMORY); + do { + if (*host == '[') { + host++; + literal = strchr(host, ']'); + if (literal == NULL) { + free(val_dup); + return (SA_SYNTAX_ERR); + } + if (literal[1] == '\0') + next = NULL; + else if (literal[1] == '/') { + next = strchr(literal + 2, ':'); + if (next != NULL) + ++next; + } else if (literal[1] == ':') + next = literal + 2; + else { + free(val_dup); + return (SA_SYNTAX_ERR); + } + } else { + next = strchr(host, ':'); + if (next != NULL) + ++next; + } + host = next; + } while (host != NULL); + free(val_dup); + return (SA_OK); + } + if (strcmp(key, "anon") == 0) key = "anonuid"; @@ -472,6 +509,10 @@ static int nfs_validate_shareopts(const char *shareopts) { char *linux_opts = NULL; + + if (strlen(shareopts) == 0) + return (SA_SYNTAX_ERR); + int error = get_linux_shareopts(shareopts, &linux_opts); if (error != SA_OK) return (error); diff --git a/lib/libzfs/libzfs_changelist.c b/lib/libzfs/libzfs_changelist.c index 4b0f6696434..efe1c0c0603 100644 --- a/lib/libzfs/libzfs_changelist.c +++ b/lib/libzfs/libzfs_changelist.c @@ -174,7 +174,6 @@ changelist_postfix(prop_changelist_t *clp) prop_changenode_t *cn; uu_avl_walk_t *walk; char shareopts[ZFS_MAXPROPLEN]; - int errors = 0; boolean_t commit_smb_shares = B_FALSE; boolean_t commit_nfs_shares = B_FALSE; @@ -262,19 +261,19 @@ changelist_postfix(prop_changelist_t *clp) const enum sa_protocol nfs[] = {SA_PROTOCOL_NFS, SA_NO_PROTOCOL}; if (sharenfs && mounted) { - errors += zfs_share(cn->cn_handle, nfs); + zfs_share(cn->cn_handle, nfs); commit_nfs_shares = B_TRUE; } else if (cn->cn_shared || clp->cl_waslegacy) { - errors += zfs_unshare(cn->cn_handle, NULL, nfs); + zfs_unshare(cn->cn_handle, NULL, nfs); commit_nfs_shares = B_TRUE; } const enum sa_protocol smb[] = {SA_PROTOCOL_SMB, SA_NO_PROTOCOL}; if (sharesmb && mounted) { - errors += zfs_share(cn->cn_handle, smb); + zfs_share(cn->cn_handle, smb); commit_smb_shares = B_TRUE; } else if (cn->cn_shared || clp->cl_waslegacy) { - errors += zfs_unshare(cn->cn_handle, NULL, smb); + zfs_unshare(cn->cn_handle, NULL, smb); commit_smb_shares = B_TRUE; } } @@ -288,7 +287,7 @@ changelist_postfix(prop_changelist_t *clp) zfs_commit_shares(proto); uu_avl_walk_end(walk); - return (errors ? -1 : 0); + return (0); } /* From 01d9283af35498202acee9e6cd17dba1cb1fe87b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dag-Erling=20Sm=C3=B8rgrav?= Date: Wed, 30 Aug 2023 17:13:06 +0200 Subject: [PATCH 13/18] Clean up existing VERIFY*() macros. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chiefly: - Remove unnecessary parentheses around variable names. - Remove spaces between the type and variable in casts. - Make the panic message for VERIFY0() reflect how the macro is used. - Use %p to format pointers, except in Linux kernel code. Reviewed-by: Brian Behlendorf Reviewed-by: Kay Pedersen Reviewed-by: Alexander Motin Signed-off-by: Dag-Erling Smørgrav Closes #15225 --- include/os/freebsd/spl/sys/debug.h | 27 +++++++++++++-------------- include/os/linux/spl/sys/debug.h | 25 ++++++++++++------------- lib/libspl/include/assert.h | 4 ++-- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/include/os/freebsd/spl/sys/debug.h b/include/os/freebsd/spl/sys/debug.h index 3e67cf0e9a7..b29d0daecc4 100644 --- a/include/os/freebsd/spl/sys/debug.h +++ b/include/os/freebsd/spl/sys/debug.h @@ -89,8 +89,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%d " #OP " %d)\n", \ - (boolean_t)(_verify3_left), \ - (boolean_t)(_verify3_right)); \ + (boolean_t)_verify3_left, \ + (boolean_t)_verify3_right); \ } while (0) #define VERIFY3S(LEFT, OP, RIGHT) do { \ @@ -100,8 +100,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%lld " #OP " %lld)\n", \ - (long long) (_verify3_left), \ - (long long) (_verify3_right)); \ + (long long)_verify3_left, \ + (long long)_verify3_right); \ } while (0) #define VERIFY3U(LEFT, OP, RIGHT) do { \ @@ -111,8 +111,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%llu " #OP " %llu)\n", \ - (unsigned long long) (_verify3_left), \ - (unsigned long long) (_verify3_right)); \ + (unsigned long long)_verify3_left, \ + (unsigned long long)_verify3_right); \ } while (0) #define VERIFY3P(LEFT, OP, RIGHT) do { \ @@ -121,19 +121,18 @@ spl_assert(const char *buf, const char *file, const char *func, int line) if (unlikely(!(_verify3_left OP _verify3_right))) \ spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ - "failed (%px " #OP " %px)\n", \ - (void *) (_verify3_left), \ - (void *) (_verify3_right)); \ + "failed (%p " #OP " %p)\n", \ + (void *)_verify3_left, \ + (void *)_verify3_right); \ } while (0) #define VERIFY0(RIGHT) do { \ - const int64_t _verify3_left = (int64_t)(0); \ - const int64_t _verify3_right = (int64_t)(RIGHT); \ - if (unlikely(!(_verify3_left == _verify3_right))) \ + const int64_t _verify0_right = (int64_t)(RIGHT); \ + if (unlikely(!(0 == _verify0_right))) \ spl_panic(__FILE__, __FUNCTION__, __LINE__, \ - "VERIFY0(0 == " #RIGHT ") " \ + "VERIFY0(" #RIGHT ") " \ "failed (0 == %lld)\n", \ - (long long) (_verify3_right)); \ + (long long)_verify0_right); \ } while (0) /* diff --git a/include/os/linux/spl/sys/debug.h b/include/os/linux/spl/sys/debug.h index 007238574fe..9bcc2e1d192 100644 --- a/include/os/linux/spl/sys/debug.h +++ b/include/os/linux/spl/sys/debug.h @@ -93,8 +93,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%d " #OP " %d)\n", \ - (boolean_t)(_verify3_left), \ - (boolean_t)(_verify3_right)); \ + (boolean_t)_verify3_left, \ + (boolean_t)_verify3_right); \ } while (0) #define VERIFY3S(LEFT, OP, RIGHT) do { \ @@ -104,8 +104,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%lld " #OP " %lld)\n", \ - (long long)(_verify3_left), \ - (long long)(_verify3_right)); \ + (long long)_verify3_left, \ + (long long)_verify3_right); \ } while (0) #define VERIFY3U(LEFT, OP, RIGHT) do { \ @@ -115,8 +115,8 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%llu " #OP " %llu)\n", \ - (unsigned long long)(_verify3_left), \ - (unsigned long long)(_verify3_right)); \ + (unsigned long long)_verify3_left, \ + (unsigned long long)_verify3_right); \ } while (0) #define VERIFY3P(LEFT, OP, RIGHT) do { \ @@ -126,18 +126,17 @@ spl_assert(const char *buf, const char *file, const char *func, int line) spl_panic(__FILE__, __FUNCTION__, __LINE__, \ "VERIFY3(" #LEFT " " #OP " " #RIGHT ") " \ "failed (%px " #OP " %px)\n", \ - (void *) (_verify3_left), \ - (void *) (_verify3_right)); \ + (void *)_verify3_left, \ + (void *)_verify3_right); \ } while (0) #define VERIFY0(RIGHT) do { \ - const int64_t _verify3_left = (int64_t)(0); \ - const int64_t _verify3_right = (int64_t)(RIGHT); \ - if (unlikely(!(_verify3_left == _verify3_right))) \ + const int64_t _verify0_right = (int64_t)(RIGHT); \ + if (unlikely(!(0 == _verify0_right))) \ spl_panic(__FILE__, __FUNCTION__, __LINE__, \ - "VERIFY0(0 == " #RIGHT ") " \ + "VERIFY0(" #RIGHT ") " \ "failed (0 == %lld)\n", \ - (long long) (_verify3_right)); \ + (long long)_verify0_right); \ } while (0) #define VERIFY_IMPLY(A, B) \ diff --git a/lib/libspl/include/assert.h b/lib/libspl/include/assert.h index c5bf0f0cc8f..e92f28300d2 100644 --- a/lib/libspl/include/assert.h +++ b/lib/libspl/include/assert.h @@ -107,8 +107,8 @@ do { \ const uintptr_t __right = (uintptr_t)(RIGHT); \ if (!(__left OP __right)) \ libspl_assertf(__FILE__, __FUNCTION__, __LINE__, \ - "%s %s %s (0x%llx %s 0x%llx)", #LEFT, #OP, #RIGHT, \ - (u_longlong_t)__left, #OP, (u_longlong_t)__right); \ + "%s %s %s (%p %s %p)", #LEFT, #OP, #RIGHT, \ + (void *)__left, #OP, (void *)__right); \ } while (0) #define VERIFY0(LEFT) \ From 506fe78c4843cd1f17e2aa183adce8e9717069c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dag-Erling=20Sm=C3=B8rgrav?= Date: Wed, 30 Aug 2023 17:13:09 +0200 Subject: [PATCH 14/18] Add VERIFY0P() and ASSERT0P() macros. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These macros are similar to VERIFY0() and ASSERT0() but are intended for pointers, and therefore use uintptr_t instead of int64_t. Reviewed-by: Brian Behlendorf Reviewed-by: Kay Pedersen Reviewed-by: Alexander Motin Signed-off-by: Dag-Erling Smørgrav Closes #15225 --- include/os/freebsd/spl/sys/debug.h | 13 +++++++++++++ include/os/linux/spl/sys/debug.h | 13 +++++++++++++ lib/libspl/include/assert.h | 11 +++++++++++ 3 files changed, 37 insertions(+) diff --git a/include/os/freebsd/spl/sys/debug.h b/include/os/freebsd/spl/sys/debug.h index b29d0daecc4..785fcf62dd1 100644 --- a/include/os/freebsd/spl/sys/debug.h +++ b/include/os/freebsd/spl/sys/debug.h @@ -39,12 +39,14 @@ * ASSERT3U() - Assert unsigned X OP Y is true, if not panic. * ASSERT3P() - Assert pointer X OP Y is true, if not panic. * ASSERT0() - Assert value is zero, if not panic. + * ASSERT0P() - Assert pointer is null, if not panic. * VERIFY() - Verify X is true, if not panic. * VERIFY3B() - Verify boolean X OP Y is true, if not panic. * VERIFY3S() - Verify signed X OP Y is true, if not panic. * VERIFY3U() - Verify unsigned X OP Y is true, if not panic. * VERIFY3P() - Verify pointer X OP Y is true, if not panic. * VERIFY0() - Verify value is zero, if not panic. + * VERIFY0P() - Verify pointer is null, if not panic. */ #ifndef _SPL_DEBUG_H @@ -135,6 +137,15 @@ spl_assert(const char *buf, const char *file, const char *func, int line) (long long)_verify0_right); \ } while (0) +#define VERIFY0P(RIGHT) do { \ + const uintptr_t _verify0_right = (uintptr_t)(RIGHT); \ + if (unlikely(!(0 == _verify0_right))) \ + spl_panic(__FILE__, __FUNCTION__, __LINE__, \ + "VERIFY0P(" #RIGHT ") " \ + "failed (NULL == %p)\n", \ + (void *)_verify0_right); \ + } while (0) + /* * Debugging disabled (--disable-debug) */ @@ -150,6 +161,7 @@ spl_assert(const char *buf, const char *file, const char *func, int line) #define ASSERT3P(x, y, z) \ ((void) sizeof ((uintptr_t)(x)), (void) sizeof ((uintptr_t)(z))) #define ASSERT0(x) ((void) sizeof ((uintptr_t)(x))) +#define ASSERT0P(x) ((void) sizeof ((uintptr_t)(x))) #define IMPLY(A, B) \ ((void) sizeof ((uintptr_t)(A)), (void) sizeof ((uintptr_t)(B))) #define EQUIV(A, B) \ @@ -165,6 +177,7 @@ spl_assert(const char *buf, const char *file, const char *func, int line) #define ASSERT3U VERIFY3U #define ASSERT3P VERIFY3P #define ASSERT0 VERIFY0 +#define ASSERT0P VERIFY0P #define ASSERT VERIFY #define IMPLY(A, B) \ ((void)(likely((!(A)) || (B)) || \ diff --git a/include/os/linux/spl/sys/debug.h b/include/os/linux/spl/sys/debug.h index 9bcc2e1d192..288193ad21c 100644 --- a/include/os/linux/spl/sys/debug.h +++ b/include/os/linux/spl/sys/debug.h @@ -34,12 +34,14 @@ * ASSERT3U() - Assert unsigned X OP Y is true, if not panic. * ASSERT3P() - Assert pointer X OP Y is true, if not panic. * ASSERT0() - Assert value is zero, if not panic. + * ASSERT0P() - Assert pointer is null, if not panic. * VERIFY() - Verify X is true, if not panic. * VERIFY3B() - Verify boolean X OP Y is true, if not panic. * VERIFY3S() - Verify signed X OP Y is true, if not panic. * VERIFY3U() - Verify unsigned X OP Y is true, if not panic. * VERIFY3P() - Verify pointer X OP Y is true, if not panic. * VERIFY0() - Verify value is zero, if not panic. + * VERIFY0P() - Verify pointer is null, if not panic. */ #ifndef _SPL_DEBUG_H @@ -139,6 +141,15 @@ spl_assert(const char *buf, const char *file, const char *func, int line) (long long)_verify0_right); \ } while (0) +#define VERIFY0P(RIGHT) do { \ + const uintptr_t _verify0_right = (uintptr_t)(RIGHT); \ + if (unlikely(!(0 == _verify0_right))) \ + spl_panic(__FILE__, __FUNCTION__, __LINE__, \ + "VERIFY0P(" #RIGHT ") " \ + "failed (NULL == %px)\n", \ + (void *)_verify0_right); \ + } while (0) + #define VERIFY_IMPLY(A, B) \ ((void)(likely((!(A)) || (B)) || \ spl_assert("(" #A ") implies (" #B ")", \ @@ -164,6 +175,7 @@ spl_assert(const char *buf, const char *file, const char *func, int line) #define ASSERT3P(x, y, z) \ ((void) sizeof ((uintptr_t)(x)), (void) sizeof ((uintptr_t)(z))) #define ASSERT0(x) ((void) sizeof ((uintptr_t)(x))) +#define ASSERT0P(x) ((void) sizeof ((uintptr_t)(x))) #define IMPLY(A, B) \ ((void) sizeof ((uintptr_t)(A)), (void) sizeof ((uintptr_t)(B))) #define EQUIV(A, B) \ @@ -179,6 +191,7 @@ spl_assert(const char *buf, const char *file, const char *func, int line) #define ASSERT3U VERIFY3U #define ASSERT3P VERIFY3P #define ASSERT0 VERIFY0 +#define ASSERT0P VERIFY0P #define ASSERT VERIFY #define IMPLY VERIFY_IMPLY #define EQUIV VERIFY_EQUIV diff --git a/lib/libspl/include/assert.h b/lib/libspl/include/assert.h index e92f28300d2..d8c5e203f42 100644 --- a/lib/libspl/include/assert.h +++ b/lib/libspl/include/assert.h @@ -120,6 +120,15 @@ do { \ (u_longlong_t)__left); \ } while (0) +#define VERIFY0P(LEFT) \ +do { \ + const uintptr_t __left = (uintptr_t)(LEFT); \ + if (!(__left == 0)) \ + libspl_assertf(__FILE__, __FUNCTION__, __LINE__, \ + "%s == 0 (%p == 0)", #LEFT, \ + (void *)__left); \ +} while (0) + #ifdef assert #undef assert #endif @@ -134,6 +143,7 @@ do { \ #define ASSERT3P(x, y, z) \ ((void) sizeof ((uintptr_t)(x)), (void) sizeof ((uintptr_t)(z))) #define ASSERT0(x) ((void) sizeof ((uintptr_t)(x))) +#define ASSERT0P(x) ((void) sizeof ((uintptr_t)(x))) #define ASSERT(x) ((void) sizeof ((uintptr_t)(x))) #define assert(x) ((void) sizeof ((uintptr_t)(x))) #define IMPLY(A, B) \ @@ -146,6 +156,7 @@ do { \ #define ASSERT3U VERIFY3U #define ASSERT3P VERIFY3P #define ASSERT0 VERIFY0 +#define ASSERT0P VERIFY0P #define ASSERT VERIFY #define assert VERIFY #define IMPLY(A, B) \ From 5f1479d92f66f87b88750c0fdbaba0238b751d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dag-Erling=20Sm=C3=B8rgrav?= Date: Wed, 30 Aug 2023 17:13:10 +0200 Subject: [PATCH 15/18] Use ASSERT0P() to check that a pointer is NULL. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Brian Behlendorf Reviewed-by: Kay Pedersen Reviewed-by: Alexander Motin Signed-off-by: Dag-Erling Smørgrav Closes #15225 --- module/zfs/dbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c index f2831a0e8ab..c0c2692c113 100644 --- a/module/zfs/dbuf.c +++ b/module/zfs/dbuf.c @@ -2701,7 +2701,7 @@ dmu_buf_will_clone(dmu_buf_t *db_fake, dmu_tx_t *tx) */ mutex_enter(&db->db_mtx); VERIFY(!dbuf_undirty(db, tx)); - ASSERT3P(dbuf_find_dirty_eq(db, tx->tx_txg), ==, NULL); + ASSERT0P(dbuf_find_dirty_eq(db, tx->tx_txg)); if (db->db_buf != NULL) { arc_buf_destroy(db->db_buf, db); db->db_buf = NULL; From 90149552b1ee50c2915305f869cbf09da271e186 Mon Sep 17 00:00:00 2001 From: Alexander Motin Date: Wed, 20 Sep 2023 14:17:11 -0400 Subject: [PATCH 16/18] ZIL: Fix potential race on flush deferring. zil_lwb_set_zio_dependency() can not set write ZIO dependency on previous LWB's write ZIO if one is already in done handler and set state to LWB_STATE_WRITE_DONE. So theoretically done handler of next LWB's write ZIO may run before done handler of previous LWB write ZIO completes. In such case we can not defer flushes, since the flush issue process is not locked. This may fix some reported assertions of lwb_vdev_tree not being empty inside zil_free_lwb(). Reviewed-by: Brian Behlendorf Signed-off-by: Alexander Motin Sponsored by: iXsystems, Inc. Closes #15278 --- module/zfs/zil.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/module/zfs/zil.c b/module/zfs/zil.c index b30676b42d8..9e9c9c22549 100644 --- a/module/zfs/zil.c +++ b/module/zfs/zil.c @@ -1550,7 +1550,16 @@ zil_lwb_write_done(zio_t *zio) lwb->lwb_state = LWB_STATE_WRITE_DONE; lwb->lwb_child_zio = NULL; lwb->lwb_write_zio = NULL; + + /* + * If nlwb is not yet issued, zil_lwb_set_zio_dependency() is not + * called for it yet, and when it will be, it won't be able to make + * its write ZIO a parent this ZIO. In such case we can not defer + * our flushes or below may be a race between the done callbacks. + */ nlwb = list_next(&zilog->zl_lwb_list, lwb); + if (nlwb && nlwb->lwb_state != LWB_STATE_ISSUED) + nlwb = NULL; mutex_exit(&zilog->zl_lock); if (avl_numnodes(t) == 0) From 6d9bc3ec9f177a278110ab004aba3e40ad421a0c Mon Sep 17 00:00:00 2001 From: Paul Dagnelie Date: Wed, 20 Sep 2023 16:39:38 -0700 Subject: [PATCH 17/18] Fix occasional rsend test crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have occasional crashes in the rsend tests. Debugging revealed that this is because the send_worker thread is getting EINTR from splice(). This happens when a non-fatal signal is received during the syscall. We should retry the syscall, rather than exiting failure. Tweak the loop to only break if the splice is finished or we receive a non-EINTR error. Reviewed-by: Brian Behlendorf Reviewed-by: Ahelenia Ziemiańska Signed-off-by: Paul Dagnelie Closes #15273 --- lib/libzfs_core/libzfs_core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/libzfs_core/libzfs_core.c b/lib/libzfs_core/libzfs_core.c index c63a16de5ab..01d803e21db 100644 --- a/lib/libzfs_core/libzfs_core.c +++ b/lib/libzfs_core/libzfs_core.c @@ -650,10 +650,12 @@ send_worker(void *arg) unsigned int bufsiz = max_pipe_buffer(ctx->from); ssize_t rd; - while ((rd = splice(ctx->from, NULL, ctx->to, NULL, bufsiz, - SPLICE_F_MOVE | SPLICE_F_MORE)) > 0) - ; - + for (;;) { + rd = splice(ctx->from, NULL, ctx->to, NULL, bufsiz, + SPLICE_F_MOVE | SPLICE_F_MORE); + if ((rd == -1 && errno != EINTR) || rd == 0) + break; + } int err = (rd == -1) ? errno : 0; close(ctx->from); return ((void *)(uintptr_t)err); From 4647353c8b2b5ca506da45bb9b01e1f3ef521847 Mon Sep 17 00:00:00 2001 From: Rob N Date: Thu, 21 Sep 2023 09:56:45 +1000 Subject: [PATCH 18/18] status: report pool suspension state under failmode=continue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When failmode=continue is set and the pool suspends, both 'zpool status' and the 'zfs/pool/state' kstat ignore it and report the normal vdev tree state. There's no clear indicator that the pool is suspended. This is unlike suspend in failmode=wait, or suspend due to MMP check failure, which both report "SUSPENDED" explicitly. This commit changes it so SUSPENDED is reported for failmode=continue the same as for other modes. Rationale: The historical behaviour of failmode=continue is roughly, "press on as though all is well". To this end, the fact that the pool had suspended was not shown, to maintain the façade that all is well. Its unclear why hiding this information was considered appropriate. One possibility is that it was expected that a true pool fault would always be reported as DEGRADED or FAULTED, and that the pool could not suspend without these happening. That is not necessarily true, as vdev health and suspend state are only loosely connected, such that a pool in (apparent) good health can be suspended for good reasons, and of course a degraded pool does not lead to suspension. Even if that expectation were true, there's still a difference in urgency - a degraded pool may not need to be attended to for hours, while a suspended pool is most often unusable until an operator intervenes. An operator that has set failmode=continue has presumably done so because their workload is one that can continue to operate in a useful way when the pool suspends. In this case the operator still needs a clear indicator that there is a problem that needs attending to. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #15297 --- lib/libzfs/libzfs_pool.c | 3 ++- module/zfs/spa_misc.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c index 85564edfd86..4ebd112f452 100644 --- a/lib/libzfs/libzfs_pool.c +++ b/lib/libzfs/libzfs_pool.c @@ -29,7 +29,7 @@ * Copyright (c) 2017, Intel Corporation. * Copyright (c) 2018, loli10K * Copyright (c) 2021, Colm Buckley - * Copyright (c) 2021, Klara Inc. + * Copyright (c) 2021, 2023, Klara Inc. */ #include @@ -255,6 +255,7 @@ zpool_get_state_str(zpool_handle_t *zhp) if (zpool_get_state(zhp) == POOL_STATE_UNAVAIL) { str = gettext("FAULTED"); } else if (status == ZPOOL_STATUS_IO_FAILURE_WAIT || + status == ZPOOL_STATUS_IO_FAILURE_CONTINUE || status == ZPOOL_STATUS_IO_FAILURE_MMP) { str = gettext("SUSPENDED"); } else { diff --git a/module/zfs/spa_misc.c b/module/zfs/spa_misc.c index 413476196b9..c7472f972cc 100644 --- a/module/zfs/spa_misc.c +++ b/module/zfs/spa_misc.c @@ -27,6 +27,7 @@ * Copyright (c) 2017 Datto Inc. * Copyright (c) 2017, Intel Corporation. * Copyright (c) 2019, loli10K . All rights reserved. + * Copyright (c) 2023, Klara Inc. */ #include @@ -2762,8 +2763,7 @@ spa_state_to_name(spa_t *spa) vdev_state_t state = rvd->vdev_state; vdev_aux_t aux = rvd->vdev_stat.vs_aux; - if (spa_suspended(spa) && - (spa_get_failmode(spa) != ZIO_FAILURE_MODE_CONTINUE)) + if (spa_suspended(spa)) return ("SUSPENDED"); switch (state) {