From owner-svn-src-head@freebsd.org Tue Feb 6 14:58:14 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C71FDEDDBBA for ; Tue, 6 Feb 2018 14:58:13 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 64E0D6D40D for ; Tue, 6 Feb 2018 14:58:13 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it0-x22d.google.com with SMTP id b66so2814029itd.5 for ; Tue, 06 Feb 2018 06:58:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=+CNBAoupRAy5xNRH3OC821/IO4NTadjStzTGyUHHiSU=; b=buVWSYaRt3SjNypr3zsx1WGtDjkjeOXT+Td0WK+yY7Ff7rFkL4GnGWNUN7M9BSZhpV ZDaPxGtM2WqgQqZ4Hmnexij8ZbFqubXEa7wq6hE3M6USqu0e6g5B8G3VRQ2zcrSgH2E6 GealFwPrpiDVszcdNHMG/TJdvO6w1+3HJcbCLNSX8DF5pWOwZ2b4YctwTRjhQkk+ar0y hqaFRYjgcxWABkgfCnz6hCphuB4F/tQe3B9yZlKfdhVsaMnTVrbKVtBd1neaFkPYTX8f g/hp0MVV6SyqOj7i6duotTja0Gm3/cODPbSMdaxG1uAOToY5xBWjFcT7NZ8DXD5An8wd wWDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=+CNBAoupRAy5xNRH3OC821/IO4NTadjStzTGyUHHiSU=; b=mbSy6soZl4+/V1U33wDnZtWyFpeRexwP2+2Sq8TZQtYZdvWoz2gvodEssCHMZY9rkk 70gsQURNk5eAg23saMrlSWFeimrofe6kxBNVB+pDnlN3g57zQ8UEcDo4CwL27aea8la7 hHosOAkS3eWowioNl4xRQqNxwrIjsf2On3qHTOTFwPcprivIf1/CFohvysyRDatw/irc boTMs7hSa6FOdW8vpQ6q/I9+sf1fXVqNJF0IN4jk0q3otYLrfoeeX0B7WSHhakzULv39 VE7xCdYFjIP6wYYzqA5SD04ZVhiodSq0rAJFrh/4JI1urmb/g1JfExtA0a3irNuXn/Bz y1pQ== X-Gm-Message-State: APf1xPBJPJJRs8bwdOJePvawYoo++DCRfdCKFxw74/fnaHDv3O3Z5bZZ hCNWrDbpAfCaRbD0+oZnoVeHghcvRMOdEF+C7ZyzQw== X-Google-Smtp-Source: AH8x225Lztp4wY0RCxMFqy0Ctr5RSyIswMlmbiDWzBw3FVTHn6ywkHxx0hZqBBVvoDorHX6NT8pOdn9/MTfYxYSfAks= X-Received: by 10.36.74.200 with SMTP id k191mr3645141itb.69.1517929092627; Tue, 06 Feb 2018 06:58:12 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.79.201.67 with HTTP; Tue, 6 Feb 2018 06:58:12 -0800 (PST) X-Originating-IP: [50.253.99.174] In-Reply-To: References: <201802060019.w160Jl1W076251@repo.freebsd.org> From: Warner Losh Date: Tue, 6 Feb 2018 07:58:12 -0700 X-Google-Sender-Auth: irW2HbRmWJ3hsj4jfrAf_GD1qzc Message-ID: Subject: Re: svn commit: r328914 - in head/sys: kern ufs/ffs To: "Conrad E. Meyer" Cc: Kirk McKusick , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Feb 2018 14:58:14 -0000 On Tue, Feb 6, 2018 at 1:09 AM, Conrad Meyer wrote: > Hi Kirk, > > On Mon, Feb 5, 2018 at 4:19 PM, Kirk McKusick > wrote: > > Author: mckusick > > Date: Tue Feb 6 00:19:46 2018 > > New Revision: 328914 > > URL: https://svnweb.freebsd.org/changeset/base/328914 > > > > Log: > > ... > > The second problem was that the check hash computed at the end of the > > read was incorrect because the calculation of the check hash on > > completion of the read was being done too soon. > > > > - When a read completes we had the following sequence: > > > > - bufdone() > > -- b_ckhashcalc (calculates check hash) > > -- bufdone_finish() > > --- vfs_vmio_iodone() (replaces bogus pages with the cached ones) > > > > - When we are reading a buffer where one or more pages are already > > in memory (but not all pages, or we wouldn't be doing the read), > > the I/O is done with bogus_page mapped in for the pages that exist > > in the VM cache. This mapping is done to avoid corrupting the > > cached pages if there is any I/O overrun. The vfs_vmio_iodone() > > function is responsible for replacing the bogus_page(s) with the > > cached ones. But we were calculating the check hash before the > > bogus_page(s) were replaced. Hence, when we were calculating the > > check hash, we were partly reading from bogus_page, which means > > we calculated a bad check hash (e.g., because multiple pages have > > been mapped to bogus_page, so its contents are indeterminate). > > > > The second fix is to move the check-hash calculation from bufdone() > > to bufdone_finish() after the call to vfs_vmio_iodone() so that it > > computes the check hash over the correct set of pages. > > Does the b_iodone callback have a very similar potential problem? It > is invoked in bufdone(), before bufdone_finish() and > vfs_vmio_iodone(). > > One example b_iodone is the bdone() callback. It seems that b_iodone > -> bdone() can then wake bwait()ers before any VMIO cached content has > been filled in. > > I don't know that any specific consumers of b_iodone are currently > broken, but it seems like maybe the b_iodone callback should really be > in the later location as well. > It looks to me like this part of bufdone_finish() if (bp->b_flags & B_VMIO) { /* * Set B_CACHE if the op was a normal read and no error * occurred. B_CACHE is set for writes in the b*write() * routines. */ if (bp->b_iocmd == BIO_READ && !(bp->b_flags & (B_INVAL|B_NOCACHE)) && !(bp->b_ioflags & BIO_ERROR)) bp->b_flags |= B_CACHE; vfs_vmio_iodone(bp); } belongs before the callback to b_iodone() rather than in bufdone_finish(). It appears that bufdone_finish() isn't called elsewhere, despite being non-static. I'm curious why this is... Warner