svn commit: r328914 - in head/sys: kern ufs/ffs
Warner Losh
imp at bsdimp.com
Tue Feb 6 14:58:14 UTC 2018
On Tue, Feb 6, 2018 at 1:09 AM, Conrad Meyer <cem at freebsd.org> wrote:
> Hi Kirk,
>
> On Mon, Feb 5, 2018 at 4:19 PM, Kirk McKusick <mckusick at 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
More information about the svn-src-all
mailing list