From nobody Tue Jan 18 18:58:12 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 7B0EB1952BAF; Tue, 18 Jan 2022 18:58:14 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JddMT3Rjjz4bhs; Tue, 18 Jan 2022 18:58:13 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642532293; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=VfFS1FRBW5VxpvYhl+fmfeGJrg2H45WiT1g+4F/A8zY=; b=gZq+7yeIcrmCihB+j1qtiId6nBIcJwnaZIGosKbw8a6ExS3lLpA76+3gZ5UVzrUOeyu6Sy 84BX5KSBsXKu+N7F756zVveU7dsVHtwZYvfRRN0k5y/wNURa1zdOBBrOeJlYQv3sENfhzQ iorMPNa8zPzXscQllf4fgzCgvneLBxAJPRJ1auEDIVwvuLFU8KYM9Fd988Fh0JuZakV4Hr ZUggXOyPwwMa9AYRi0kEvj9HIuvsNyNspFXfTBOC3fgUkfui8xA5d+gfz7TxqBOIY1bJFw fLYCrpaIp/VnMV5KBf13bn/Oh3lwI0pIXv5QSPLwPEf9/VbJ8+rcYGo66dd/ZA== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id EFF072677B; Tue, 18 Jan 2022 18:58:12 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 20IIwCQp045623; Tue, 18 Jan 2022 18:58:12 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 20IIwCTX045622; Tue, 18 Jan 2022 18:58:12 GMT (envelope-from git) Date: Tue, 18 Jan 2022 18:58:12 GMT Message-Id: <202201181858.20IIwCTX045622@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: "Kenneth D. Merry" Subject: git: 6e8a2f040017 - main - Update sa(4) comments and man page after review. List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: ken X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 6e8a2f04001735353e445570f0d83aa88d4b9b37 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642532293; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=VfFS1FRBW5VxpvYhl+fmfeGJrg2H45WiT1g+4F/A8zY=; b=dvXQjkGsgBffvVW7FH9AqwnhLARyHQGySJN8uZAWRO7WZf9iKfXCODXaCbIJa4xKWnxOkq rEnLs2ncfuF9zpFnldmAm28yTRL+ZcU+x0W4lX95QvsdCZ7M9to6D87xBO4JNL9uuvAZQx iq0E2FPsGHzpa1eXZTI0pITACcoTLjFlPR6xaOfVXTzr4kW7zVlcQuXh2xLny1DbmBbscS q+/inRyczs8Lz7j2neaJG+QC1P8r8xCipcq3/Tb5d4/JRLWN+Kv1wCs6mGF1wGA5fAb4XG mHm6WbH8K3YVW8cn2LxJ1flXx59td6T5YDoxaK/LJYt2J3GGNBXuGjsPHsv07g== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1642532293; a=rsa-sha256; cv=none; b=d8Dg3kao3mK7mvi/rR3/5DWUu1u+DdD4V5FduHG9OOuAcZobEkMhcaGSO7nuA6A1ZKPuUE vQr2lws4IkQjAFcXfzKyRw5jAh1JHJhqSB4VxVV7SDi11HAw9EIBQp11UFLVNagbydS4+a 6eZe2wQ1SGo8sDVqUIk/YseGz1H1Er/QtZ3g7cboKdOiJxew3aPUxxl0vo2CORiinPgqrp TLWfS40Wu/M5GS4fkvXkr2X1TSgrNqBebhuaI6OC4W/16iX4ubK60vv40tzSMsknrvXbtw kl+J/8m447JbwrDTlB9B+YsqRV6OXBilCUegE2DGa7AfD8nUM4WWqjU2ZgIj7g== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by ken: URL: https://cgit.FreeBSD.org/src/commit/?id=6e8a2f04001735353e445570f0d83aa88d4b9b37 commit 6e8a2f04001735353e445570f0d83aa88d4b9b37 Author: Kenneth D. Merry AuthorDate: 2022-01-18 15:44:31 +0000 Commit: Kenneth D. Merry 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 Requested by: Daniel Ebdrup Jensen 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) { /*