svn commit: r328914 - in head/sys: kern ufs/ffs

[Conrad Meyer]

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.

Best,
Conrad