Date: Tue, 18 Jan 2022 18:58:12 GMT From: "Kenneth D. Merry" <ken@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 6e8a2f040017 - main - Update sa(4) comments and man page after review. Message-ID: <202201181858.20IIwCTX045622@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by ken: URL: https://cgit.FreeBSD.org/src/commit/?id=6e8a2f04001735353e445570f0d83aa88d4b9b37 commit 6e8a2f04001735353e445570f0d83aa88d4b9b37 Author: Kenneth D. Merry <ken@FreeBSD.org> AuthorDate: 2022-01-18 15:44:31 +0000 Commit: Kenneth D. Merry <ken@FreeBSD.org> CommitDate: 2022-01-18 18:50:31 +0000 Update sa(4) comments and man page after review. sys/cam/scsi/scsi_sa.c: Add comments explaining the priority order of the various sources of timeout values. Also, explain that the probe that pulls in drive recommended timeouts via the REPORT SUPPORTED OPERATION CODES command is in a race with the thread that creates the sysctl variables. Because of that race, it is important that the sysctl thread not load any timeout values from the kernel environment. share/man/man4/sa.4: Use the Sy macro to emphasize thousandths of a second instead of capitalizing it. Requested by: Warner Losh <imp@freebsd.org> Requested by: Daniel Ebdrup Jensen <debdrup@freebsd.org> Sponsored by: Spectra Logic MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33883 --- share/man/man4/sa.4 | 14 ++++++------- sys/cam/scsi/scsi_sa.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/share/man/man4/sa.4 b/share/man/man4/sa.4 index 6b88fd016522..35ffcdb877d6 100644 --- a/share/man/man4/sa.4 +++ b/share/man/man4/sa.4 @@ -25,7 +25,7 @@ .\" .\" $FreeBSD$ .\" -.Dd January 14, 2022 +.Dd January 18, 2022 .Dt SA 4 .Os .Sh NAME @@ -338,8 +338,9 @@ If the drive does report timeout descriptors, the .Nm driver will use the drive's recommended timeouts for commands. .Pp -The timeouts in use are reported in units of THOUSANDTHS of a second via -the +The timeouts in use are reported in units of +.Sy thousandths +of a second via the .Va kern.cam.sa.%d.timeout.* .Xr sysctl 8 variables. @@ -375,7 +376,6 @@ Loader tunables: .Pp .Bl -tag -compact .It kern.cam.sa.timeout.erase -.It kern.cam.sa.timeout.load .It kern.cam.sa.timeout.locate .It kern.cam.sa.timeout.mode_select .It kern.cam.sa.timeout.mode_sense @@ -398,7 +398,6 @@ values: .Pp .Bl -tag -compact .It kern.cam.sa.%d.timeout.erase -.It kern.cam.sa.%d.timeout.load .It kern.cam.sa.%d.timeout.locate .It kern.cam.sa.%d.timeout.mode_select .It kern.cam.sa.%d.timeout.mode_sense @@ -415,8 +414,9 @@ values: .It kern.cam.sa.%d.timeout.write_filemarks .El .Pp -As mentioned above, the timeouts are set and reported in THOUSANDTHS of a -second, so be sure to account for that when setting them. +As mentioned above, the timeouts are set and reported in +.Sy thousandths +of a second, so be sure to account for that when setting them. .Sh IOCTLS The .Nm diff --git a/sys/cam/scsi/scsi_sa.c b/sys/cam/scsi/scsi_sa.c index b7475102af14..04d013d03a30 100644 --- a/sys/cam/scsi/scsi_sa.c +++ b/sys/cam/scsi/scsi_sa.c @@ -175,6 +175,22 @@ typedef enum { SA_TIMEOUT_TYPE_MAX } sa_timeout_types; +/* + * These are the default timeout values that apply to all tape drives. + * + * We get timeouts from the following places in order of increasing + * priority: + * 1. Driver default timeouts. (Set in the structure below.) + * 2. Timeouts loaded from the drive via REPORT SUPPORTED OPERATION + * CODES. (If the drive supports it, SPC-4/LTO-5 and newer should.) + * 3. Global loader tunables, used for all sa(4) driver instances on + * a machine. + * 4. Instance-specific loader tunables, used for say sa5. + * 5. On the fly user sysctl changes. + * + * Each step will overwrite the timeout value set from the one + * before, so you go from general to most specific. + */ static struct sa_timeout_desc { const char *desc; int value; @@ -2333,6 +2349,12 @@ sasetupdev(struct sa_softc *softc, struct cdev *dev) softc->num_devs_to_destroy++; } +/* + * Load the global (for all sa(4) instances) and per-instance tunable + * values for timeouts for various sa(4) commands. This should be run + * after the default timeouts are fetched from the drive, so the user's + * preference will override the drive's defaults. + */ static void saloadtotunables(struct sa_softc *softc) { @@ -2342,12 +2364,17 @@ saloadtotunables(struct sa_softc *softc) for (i = 0; i < SA_TIMEOUT_TYPE_MAX; i++) { int tmpval, retval; + /* First grab any global timeout setting */ snprintf(tmpstr, sizeof(tmpstr), "kern.cam.sa.timeout.%s", sa_default_timeouts[i].desc); retval = TUNABLE_INT_FETCH(tmpstr, &tmpval); if (retval != 0) softc->timeout_info[i] = tmpval; + /* + * Then overwrite any global timeout settings with + * per-instance timeout settings. + */ snprintf(tmpstr, sizeof(tmpstr), "kern.cam.sa.%u.timeout.%s", softc->periph->unit_number, sa_default_timeouts[i].desc); retval = TUNABLE_INT_FETCH(tmpstr, &tmpval); @@ -2408,6 +2435,15 @@ sasysctlinit(void *context, int pending) snprintf(tmpstr, sizeof(tmpstr), "%s timeout", sa_default_timeouts[i].desc); + /* + * Do NOT change this sysctl declaration to also load any + * tunable values for this sa(4) instance. In other words, + * do not change this to CTLFLAG_RWTUN. This function is + * run in parallel with the probe routine that fetches + * recommended timeout values from the tape drive, and we + * don't want the values from the drive to override the + * user's preference. + */ SYSCTL_ADD_INT(&softc->sysctl_timeout_ctx, SYSCTL_CHILDREN(softc->sysctl_timeout_tree), OID_AUTO, sa_default_timeouts[i].desc, CTLFLAG_RW, @@ -2660,7 +2696,9 @@ saregister(struct cam_periph *periph, void *arg) softc->density_type_bits[3] = SRDS_MEDIUM_TYPE | SRDS_MEDIA; /* * Bump the peripheral refcount for the sysctl thread, in case we - * get invalidated before the thread has a chance to run. + * get invalidated before the thread has a chance to run. Note + * that this runs in parallel with the probe for the timeout + * values. */ cam_periph_acquire(periph); taskqueue_enqueue(taskqueue_thread, &softc->sysctl_task); @@ -2673,7 +2711,20 @@ saregister(struct cam_periph *periph, void *arg) /* * See comment above, try fetching timeout values for drives that - * might support it. Otherwise, use the defaults. + * might support it. Otherwise, use the defaults. + * + * We get timeouts from the following places in order of increasing + * priority: + * 1. Driver default timeouts. + * 2. Timeouts loaded from the drive via REPORT SUPPORTED OPERATION + * CODES. (We kick that off here if SA_FLAG_RSOC_TO_TRY is set.) + * 3. Global loader tunables, used for all sa(4) driver instances on + * a machine. + * 4. Instance-specific loader tunables, used for say sa5. + * 5. On the fly user sysctl changes. + * + * Each step will overwrite the timeout value set from the one + * before, so you go from general to most specific. */ if (softc->flags & SA_FLAG_RSOC_TO_TRY) { /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202201181858.20IIwCTX045622>