From 801d9b4f9685560e154ccb0b26fef5a2f0aa38d4 Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Thu, 30 Oct 2025 17:23:27 -0700 Subject: [PATCH] debug: move all of the debug bits out of the spl Pull all of the internal debug infrastructure up in to the zfs code to clean up the layering. Remove all the dodgy usage of SET_ERROR and DTRACE_PROBE from the spl. Luckily it was lightly used in the spl layer so we're not losing much. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #17861 --- include/os/freebsd/Makefile.am | 4 +-- .../os/freebsd/{spl => zfs}/sys/trace_zfs.h | 0 .../{spl/sys/sdt.h => zfs/sys/zfs_debug_os.h} | 9 +++--- include/os/linux/Makefile.am | 2 +- include/os/linux/spl/sys/sysmacros.h | 8 ----- include/os/linux/zfs/sys/zfs_debug_os.h | 29 +++++++++++++++++++ include/sys/zfs_context.h | 1 - include/sys/zfs_debug.h | 1 + lib/libspl/include/Makefile.am | 1 - lib/libspl/include/sys/trace_spl.h | 24 --------------- lib/libzpool/include/Makefile.am | 3 +- lib/libzpool/include/sys/trace_zfs.h | 25 +--------------- lib/libzpool/include/sys/zfs_debug_os.h | 29 +++++++++++++++++++ module/os/freebsd/spl/spl_uio.c | 2 +- module/os/linux/spl/spl-taskq.c | 14 --------- scripts/spdxcheck.pl | 1 - 16 files changed, 71 insertions(+), 82 deletions(-) rename include/os/freebsd/{spl => zfs}/sys/trace_zfs.h (100%) rename include/os/freebsd/{spl/sys/sdt.h => zfs/sys/zfs_debug_os.h} (92%) create mode 100644 include/os/linux/zfs/sys/zfs_debug_os.h delete mode 100644 lib/libspl/include/sys/trace_spl.h create mode 100644 lib/libzpool/include/sys/zfs_debug_os.h diff --git a/include/os/freebsd/Makefile.am b/include/os/freebsd/Makefile.am index d6b6923d0..47cf6756a 100644 --- a/include/os/freebsd/Makefile.am +++ b/include/os/freebsd/Makefile.am @@ -44,7 +44,6 @@ noinst_HEADERS = \ %D%/spl/sys/procfs_list.h \ %D%/spl/sys/random.h \ %D%/spl/sys/rwlock.h \ - %D%/spl/sys/sdt.h \ %D%/spl/sys/sid.h \ %D%/spl/sys/sig.h \ %D%/spl/sys/simd.h \ @@ -63,7 +62,6 @@ noinst_HEADERS = \ %D%/spl/sys/time.h \ %D%/spl/sys/timer.h \ %D%/spl/sys/trace.h \ - %D%/spl/sys/trace_zfs.h \ %D%/spl/sys/types.h \ %D%/spl/sys/types32.h \ %D%/spl/sys/uio.h \ @@ -82,10 +80,12 @@ noinst_HEADERS = \ %D%/zfs/sys/arc_os.h \ %D%/zfs/sys/freebsd_crypto.h \ %D%/zfs/sys/freebsd_event.h \ + %D%/zfs/sys/trace_zfs.h \ %D%/zfs/sys/vdev_os.h \ %D%/zfs/sys/zfs_bootenv_os.h \ %D%/zfs/sys/zfs_context_os.h \ %D%/zfs/sys/zfs_ctldir.h \ + %D%/zfs/sys/zfs_debug_os.h \ %D%/zfs/sys/zfs_dir.h \ %D%/zfs/sys/zfs_ioctl_compat.h \ %D%/zfs/sys/zfs_vfsops_os.h \ diff --git a/include/os/freebsd/spl/sys/trace_zfs.h b/include/os/freebsd/zfs/sys/trace_zfs.h similarity index 100% rename from include/os/freebsd/spl/sys/trace_zfs.h rename to include/os/freebsd/zfs/sys/trace_zfs.h diff --git a/include/os/freebsd/spl/sys/sdt.h b/include/os/freebsd/zfs/sys/zfs_debug_os.h similarity index 92% rename from include/os/freebsd/spl/sys/sdt.h rename to include/os/freebsd/zfs/sys/zfs_debug_os.h index ef1dad6c1..cc7540c4f 100644 --- a/include/os/freebsd/spl/sys/sdt.h +++ b/include/os/freebsd/zfs/sys/zfs_debug_os.h @@ -27,10 +27,11 @@ * $FreeBSD$ */ -#ifndef _OPENSOLARIS_SYS_SDT_H_ -#define _OPENSOLARIS_SYS_SDT_H_ +#ifndef _SYS_ZFS_DEBUG_OS_H +#define _SYS_ZFS_DEBUG_OS_H + +#include -#include_next #ifdef KDTRACE_HOOKS SDT_PROBE_DECLARE(sdt, , , set__error); @@ -44,4 +45,4 @@ SDT_PROBE_DECLARE(sdt, , , set__error); #define SET_ERROR(err) (err) #endif -#endif /* _OPENSOLARIS_SYS_SDT_H_ */ +#endif /* _SYS_ZFS_DEBUG_OS_H */ diff --git a/include/os/linux/Makefile.am b/include/os/linux/Makefile.am index e156ca183..9188a974c 100644 --- a/include/os/linux/Makefile.am +++ b/include/os/linux/Makefile.am @@ -41,6 +41,7 @@ kernel_sys_HEADERS = \ %D%/zfs/sys/zfs_bootenv_os.h \ %D%/zfs/sys/zfs_context_os.h \ %D%/zfs/sys/zfs_ctldir.h \ + %D%/zfs/sys/zfs_debug_os.h \ %D%/zfs/sys/zfs_dir.h \ %D%/zfs/sys/zfs_vfsops_os.h \ %D%/zfs/sys/zfs_vnops_os.h \ @@ -97,7 +98,6 @@ kernel_spl_sys_HEADERS = \ %D%/spl/sys/time.h \ %D%/spl/sys/timer.h \ %D%/spl/sys/trace.h \ - %D%/spl/sys/trace_spl.h \ %D%/spl/sys/trace_taskq.h \ %D%/spl/sys/tsd.h \ %D%/spl/sys/types.h \ diff --git a/include/os/linux/spl/sys/sysmacros.h b/include/os/linux/spl/sys/sysmacros.h index db48222b7..dc9e9e492 100644 --- a/include/os/linux/spl/sys/sysmacros.h +++ b/include/os/linux/spl/sys/sysmacros.h @@ -34,11 +34,6 @@ #include #include - -#ifndef _KERNEL -#define _KERNEL __KERNEL__ -#endif - #define FALSE 0 #define TRUE 1 @@ -202,9 +197,6 @@ makedev(unsigned int major, unsigned int minor) #define P2SAMEHIGHBIT_TYPED(x, y, type) \ (((type)(x) ^ (type)(y)) < ((type)(x) & (type)(y))) -#define SET_ERROR(err) \ - (__set_error(__FILE__, __func__, __LINE__, err), err) - #include #define qsort(base, num, size, cmp) \ sort(base, num, size, cmp, NULL) diff --git a/include/os/linux/zfs/sys/zfs_debug_os.h b/include/os/linux/zfs/sys/zfs_debug_os.h new file mode 100644 index 000000000..284180952 --- /dev/null +++ b/include/os/linux/zfs/sys/zfs_debug_os.h @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: CDDL-1.0 +/* + * CDDL HEADER START + * + * The contents of this file are subject to the terms of the + * Common Development and Distribution License (the "License"). + * You may not use this file except in compliance with the License. + * + * You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE + * or https://opensource.org/licenses/CDDL-1.0. + * See the License for the specific language governing permissions + * and limitations under the License. + * + * When distributing Covered Code, include this CDDL HEADER in each + * file and include the License file at usr/src/OPENSOLARIS.LICENSE. + * If applicable, add the following below this CDDL HEADER, with the + * fields enclosed by brackets "[]" replaced with your own identifying + * information: Portions Copyright [yyyy] [name of copyright owner] + * + * CDDL HEADER END + */ + +#ifndef _SYS_ZFS_DEBUG_OS_H +#define _SYS_ZFS_DEBUG_OS_H + +#define SET_ERROR(err) \ + (__set_error(__FILE__, __func__, __LINE__, err), err) + +#endif /* _SYS_ZFS_DEBUG_OS_H */ diff --git a/include/sys/zfs_context.h b/include/sys/zfs_context.h index 4be22835d..aeac81df9 100644 --- a/include/sys/zfs_context.h +++ b/include/sys/zfs_context.h @@ -109,7 +109,6 @@ extern "C" { #include #include #include -#include #include #include diff --git a/include/sys/zfs_debug.h b/include/sys/zfs_debug.h index 4d4cd4c39..0f021d151 100644 --- a/include/sys/zfs_debug.h +++ b/include/sys/zfs_debug.h @@ -40,6 +40,7 @@ extern "C" { #endif #include +#include extern int zfs_flags; extern int zfs_recover; diff --git a/lib/libspl/include/Makefile.am b/lib/libspl/include/Makefile.am index 202f15944..e68742409 100644 --- a/lib/libspl/include/Makefile.am +++ b/lib/libspl/include/Makefile.am @@ -69,7 +69,6 @@ libspl_sys_HEADERS = \ %D%/sys/time.h \ %D%/sys/timer.h \ %D%/sys/trace.h \ - %D%/sys/trace_spl.h \ %D%/sys/tsd.h \ %D%/sys/tunables.h \ %D%/sys/types.h \ diff --git a/lib/libspl/include/sys/trace_spl.h b/lib/libspl/include/sys/trace_spl.h deleted file mode 100644 index b80d288f7..000000000 --- a/lib/libspl/include/sys/trace_spl.h +++ /dev/null @@ -1,24 +0,0 @@ -/* Here to keep the libspl build happy */ - -#ifndef _LIBSPL_SPL_TRACE_H -#define _LIBSPL_SPL_TRACE_H - -/* - * The set-error SDT probe is extra static, in that we declare its fake - * function literally, rather than with the DTRACE_PROBE1() macro. This is - * necessary so that SET_ERROR() can evaluate to a value, which wouldn't - * be possible if it required multiple statements (to declare the function - * and then call it). - * - * SET_ERROR() uses the comma operator so that it can be used without much - * additional code. For example, "return (EINVAL);" becomes - * "return (SET_ERROR(EINVAL));". Note that the argument will be evaluated - * twice, so it should not have side effects (e.g. something like: - * "return (SET_ERROR(log_error(EINVAL, info)));" would log the error twice). - */ -#undef SET_ERROR -#define SET_ERROR(err) \ - (__set_error(__FILE__, __func__, __LINE__, err), err) - - -#endif diff --git a/lib/libzpool/include/Makefile.am b/lib/libzpool/include/Makefile.am index 54d10e623..420b6f646 100644 --- a/lib/libzpool/include/Makefile.am +++ b/lib/libzpool/include/Makefile.am @@ -3,4 +3,5 @@ libzpool_sys_HEADERS = \ %D%/sys/abd_os.h \ %D%/sys/abd_impl_os.h \ %D%/sys/trace_zfs.h \ - %D%/sys/zfs_context_os.h + %D%/sys/zfs_context_os.h \ + %D%/sys/zfs_debug_os.h diff --git a/lib/libzpool/include/sys/trace_zfs.h b/lib/libzpool/include/sys/trace_zfs.h index 87ed5ad3c..d9639d27b 100644 --- a/lib/libzpool/include/sys/trace_zfs.h +++ b/lib/libzpool/include/sys/trace_zfs.h @@ -1,24 +1 @@ -/* Here to keep the libspl build happy */ - -#ifndef _LIBSPL_ZFS_TRACE_H -#define _LIBSPL_ZFS_TRACE_H - -/* - * The set-error SDT probe is extra static, in that we declare its fake - * function literally, rather than with the DTRACE_PROBE1() macro. This is - * necessary so that SET_ERROR() can evaluate to a value, which wouldn't - * be possible if it required multiple statements (to declare the function - * and then call it). - * - * SET_ERROR() uses the comma operator so that it can be used without much - * additional code. For example, "return (EINVAL);" becomes - * "return (SET_ERROR(EINVAL));". Note that the argument will be evaluated - * twice, so it should not have side effects (e.g. something like: - * "return (SET_ERROR(log_error(EINVAL, info)));" would log the error twice). - */ -#undef SET_ERROR -#define SET_ERROR(err) \ - (__set_error(__FILE__, __func__, __LINE__, err), err) - - -#endif +/* keep me */ diff --git a/lib/libzpool/include/sys/zfs_debug_os.h b/lib/libzpool/include/sys/zfs_debug_os.h new file mode 100644 index 000000000..b59165a6c --- /dev/null +++ b/lib/libzpool/include/sys/zfs_debug_os.h @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: CDDL-1.0 +/* + * CDDL HEADER START + * + * The contents of this file are subject to the terms of the + * Common Development and Distribution License (the "License"). + * You may not use this file except in compliance with the License. + * + * You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE + * or https://opensource.org/licenses/CDDL-1.0. + * See the License for the specific language governing permissions + * and limitations under the License. + * + * When distributing Covered Code, include this CDDL HEADER in each + * file and include the License file at usr/src/OPENSOLARIS.LICENSE. + * If applicable, add the following below this CDDL HEADER, with the + * fields enclosed by brackets "[]" replaced with your own identifying + * information: Portions Copyright [yyyy] [name of copyright owner] + * + * CDDL HEADER END + */ + +#ifndef _SYS_ZFS_DEBUG_OS_H +#define _SYS_ZFS_DEBUG_OS_H + +#define SET_ERROR(err) \ + (__set_error(__FILE__, __func__, __LINE__, err), err) + +#endif diff --git a/module/os/freebsd/spl/spl_uio.c b/module/os/freebsd/spl/spl_uio.c index 54d4029c5..b92be3710 100644 --- a/module/os/freebsd/spl/spl_uio.c +++ b/module/os/freebsd/spl/spl_uio.c @@ -238,7 +238,7 @@ zfs_uio_iov_step(struct iovec v, zfs_uio_t *uio, int *numpages) zfs_uio_rw(uio), &uio->uio_dio.pages[uio->uio_dio.npages]); if (res != n) - return (SET_ERROR(EFAULT)); + return (EFAULT); ASSERT3U(len, ==, res * PAGE_SIZE); *numpages = res; diff --git a/module/os/linux/spl/spl-taskq.c b/module/os/linux/spl/spl-taskq.c index 092f090d9..00ff78926 100644 --- a/module/os/linux/spl/spl-taskq.c +++ b/module/os/linux/spl/spl-taskq.c @@ -32,7 +32,6 @@ #include #include #include -#include #include #include #include @@ -325,7 +324,6 @@ task_expire_impl(taskq_ent_t *t) } t->tqent_birth = jiffies; - DTRACE_PROBE1(taskq_ent__birth, taskq_ent_t *, t); /* * The priority list must be maintained in strict task id order @@ -713,9 +711,7 @@ taskq_dispatch(taskq_t *tq, task_func_t func, void *arg, uint_t flags) t->tqent_taskq = tq; t->tqent_timer.function = NULL; t->tqent_timer.expires = 0; - t->tqent_birth = jiffies; - DTRACE_PROBE1(taskq_ent__birth, taskq_ent_t *, t); ASSERT(!(t->tqent_flags & TQENT_FLAG_PREALLOC)); @@ -840,9 +836,7 @@ taskq_dispatch_ent(taskq_t *tq, task_func_t func, void *arg, uint_t flags, t->tqent_func = func; t->tqent_arg = arg; t->tqent_taskq = tq; - t->tqent_birth = jiffies; - DTRACE_PROBE1(taskq_ent__birth, taskq_ent_t *, t); spin_unlock(&t->tqent_lock); @@ -1054,11 +1048,6 @@ taskq_thread(void *args) * A TQENT_FLAG_PREALLOC task may be reused or freed * during the task function call. Store tqent_id and * tqent_flags here. - * - * Also use an on stack taskq_ent_t for tqt_task - * assignment in this case; we want to make sure - * to duplicate all fields, so the values are - * correct when it's accessed via DTRACE_PROBE*. */ tqt->tqt_id = t->tqent_id; tqt->tqt_flags = t->tqent_flags; @@ -1074,13 +1063,10 @@ taskq_thread(void *args) spin_unlock_irqrestore(&tq->tq_lock, flags); TQSTAT_INC(tq, threads_active); - DTRACE_PROBE1(taskq_ent__start, taskq_ent_t *, t); /* Perform the requested task */ t->tqent_func(t->tqent_arg); - DTRACE_PROBE1(taskq_ent__finish, taskq_ent_t *, t); - TQSTAT_DEC(tq, threads_active); if ((t->tqent_flags & TQENT_LIST_MASK) == TQENT_LIST_PENDING) diff --git a/scripts/spdxcheck.pl b/scripts/spdxcheck.pl index 1b3dd6393..59300ea86 100755 --- a/scripts/spdxcheck.pl +++ b/scripts/spdxcheck.pl @@ -128,7 +128,6 @@ my $untagged_patterns = q( include/os/freebsd/zfs/sys/zpl.h include/os/linux/kernel/linux/page_compat.h lib/libspl/include/sys/string.h - lib/libspl/include/sys/trace_spl.h lib/libzdb/libzdb.c lib/libzpool/include/sys/trace_zfs.h module/lua/setjmp/setjmp.S