Date: Tue, 6 Feb 2018 00:09:03 -0800 From: Conrad Meyer <cem@freebsd.org> To: Kirk McKusick <mckusick@freebsd.org> Cc: 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: <CAG6CVpUr6%2Bp0E5Lny_XrKVv53kaxzrKLTZJjHcFkZkck1j2brA@mail.gmail.com> In-Reply-To: <201802060019.w160Jl1W076251@repo.freebsd.org> References: <201802060019.w160Jl1W076251@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. Best, Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpUr6%2Bp0E5Lny_XrKVv53kaxzrKLTZJjHcFkZkck1j2brA>