From owner-svn-src-stable-12@freebsd.org Thu Dec 19 01:34:35 2019 Return-Path: Delivered-To: svn-src-stable-12@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 839F91CE4D0; Thu, 19 Dec 2019 01:34:35 +0000 (UTC) (envelope-from mav@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) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47dZDW2wXLz42cP; Thu, 19 Dec 2019 01:34:35 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 5F8A77255; Thu, 19 Dec 2019 01:34:35 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id xBJ1YZFV043003; Thu, 19 Dec 2019 01:34:35 GMT (envelope-from mav@FreeBSD.org) Received: (from mav@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id xBJ1YZi2043002; Thu, 19 Dec 2019 01:34:35 GMT (envelope-from mav@FreeBSD.org) Message-Id: <201912190134.xBJ1YZi2043002@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mav set sender to mav@FreeBSD.org using -f From: Alexander Motin Date: Thu, 19 Dec 2019 01:34:35 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r355895 - stable/12/sys/geom X-SVN-Group: stable-12 X-SVN-Commit-Author: mav X-SVN-Commit-Paths: stable/12/sys/geom X-SVN-Commit-Revision: 355895 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Dec 2019 01:34:35 -0000 Author: mav Date: Thu Dec 19 01:34:34 2019 New Revision: 355895 URL: https://svnweb.freebsd.org/changeset/base/355895 Log: MFC r355451: Remove some branching from GEOM_DISK hot path. pp->private just can not be NULL in those places. In g_disk_start() and g_disk_ioctl() both dp != NULL and !dp->d_destroyed should always be true if disk_gone() and disk_destroy() are used properly, since GEOM does not send requests to errored providers. If the protocol is not followed, then no amount of additional checks here give real safety. In g_disk_access() though the checks are useful, since GEOM blocks only new opens for errored providers, but allows closes. It should not happen if disk_gone() and disk_destroy() are used properly, but may otherwise. To improve cases when disk_gone() is not used, call it from disk_destroy(). It does not give full guaranties, but it errors the provider and makes GEOM block unwanted requests at least after some race. Modified: stable/12/sys/geom/geom_disk.c Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/geom/geom_disk.c ============================================================================== --- stable/12/sys/geom/geom_disk.c Thu Dec 19 01:32:15 2019 (r355894) +++ stable/12/sys/geom/geom_disk.c Thu Dec 19 01:34:34 2019 (r355895) @@ -108,7 +108,7 @@ g_disk_access(struct g_provider *pp, int r, int w, int pp->name, r, w, e); g_topology_assert(); sc = pp->private; - if (sc == NULL || (dp = sc->dp) == NULL || dp->d_destroyed) { + if ((dp = sc->dp) == NULL || dp->d_destroyed) { /* * Allow decreasing access count even if disk is not * available anymore. @@ -274,6 +274,8 @@ g_disk_ioctl(struct g_provider *pp, u_long cmd, void * sc = pp->private; dp = sc->dp; + KASSERT(dp != NULL && !dp->d_destroyed, + ("g_disk_ioctl(%lx) on destroyed disk %s", cmd, pp->name)); if (dp->d_ioctl == NULL) return (ENOIOCTL); @@ -432,10 +434,9 @@ g_disk_start(struct bio *bp) biotrack(bp, __func__); sc = bp->bio_to->private; - if (sc == NULL || (dp = sc->dp) == NULL || dp->d_destroyed) { - g_io_deliver(bp, ENXIO); - return; - } + dp = sc->dp; + KASSERT(dp != NULL && !dp->d_destroyed, + ("g_disk_start(%p) on destroyed disk %s", bp, bp->bio_to->name)); error = EJUSTRETURN; switch(bp->bio_cmd) { case BIO_DELETE: @@ -893,8 +894,9 @@ void disk_destroy(struct disk *dp) { - g_cancel_event(dp); + disk_gone(dp); dp->d_destroyed = 1; + g_cancel_event(dp); if (dp->d_devstat != NULL) devstat_remove_entry(dp->d_devstat); g_post_event(g_disk_destroy, dp, M_WAITOK, NULL); @@ -919,6 +921,16 @@ disk_gone(struct disk *dp) struct g_provider *pp; mtx_pool_lock(mtxpool_sleep, dp); + + /* + * Second wither call makes no sense, plus we can not access the list + * of providers without topology lock after calling wither once. + */ + if (dp->d_goneflag != 0) { + mtx_pool_unlock(mtxpool_sleep, dp); + return; + } + dp->d_goneflag = 1; /* @@ -943,13 +955,11 @@ disk_gone(struct disk *dp) mtx_pool_unlock(mtxpool_sleep, dp); gp = dp->d_geom; - if (gp != NULL) { - pp = LIST_FIRST(&gp->provider); - if (pp != NULL) { - KASSERT(LIST_NEXT(pp, provider) == NULL, - ("geom %p has more than one provider", gp)); - g_wither_provider(pp, ENXIO); - } + pp = LIST_FIRST(&gp->provider); + if (pp != NULL) { + KASSERT(LIST_NEXT(pp, provider) == NULL, + ("geom %p has more than one provider", gp)); + g_wither_provider(pp, ENXIO); } }