Date: Tue, 6 Feb 2018 07:58:12 -0700 From: Warner Losh <imp@bsdimp.com> To: "Conrad E. Meyer" <cem@freebsd.org> Cc: Kirk McKusick <mckusick@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328914 - in head/sys: kern ufs/ffs Message-ID: <CANCZdfrapj1_aNT3w9-RPqLJ8JuLgz7hLZ45FbB0oQTaTu8ujA@mail.gmail.com> In-Reply-To: <CAG6CVpUr6%2Bp0E5Lny_XrKVv53kaxzrKLTZJjHcFkZkck1j2brA@mail.gmail.com> References: <201802060019.w160Jl1W076251@repo.freebsd.org> <CAG6CVpUr6%2Bp0E5Lny_XrKVv53kaxzrKLTZJjHcFkZkck1j2brA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 6, 2018 at 1:09 AM, Conrad Meyer <cem@freebsd.org> wrote: > Hi Kirk, > > On Mon, Feb 5, 2018 at 4:19 PM, Kirk McKusick <mckusick@freebsd.org> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrapj1_aNT3w9-RPqLJ8JuLgz7hLZ45FbB0oQTaTu8ujA>