On Mon, Jun 04, 2018 at 08:25:33AM -0400, Brian Foster wrote:
> > + if (offset > i_size_read(inode)) {
> > + whichfork = XFS_IO_HOLE;
>
> Not sure what the point of this assignment is if we're going to return,
> but that's not a valid fork either way. Did you mean to assign
> wpc->io_type?
Yes, I did. And this bug actually caused my unexplainable test
failures with 1k file systems..
> > + /*
> > + * If we already have a valid COW mapping keep using it.
> > + */
> > + if (wpc->io_type == XFS_IO_COW &&
> > + xfs_imap_valid(inode, &wpc->imap, offset)) {
> > + wpc->imap_valid = true;
> > + new_type = XFS_IO_COW;
> > }
>
> So this lifts the cow I/O type check from xfs_map_cow(). That path would
> set imap_valid and new_type and return, which essentially looks like a
> "blocks found" case where we go ahead and add the buffer to the ioend.
> Now we set the same values, but we have to call xfs_map_blocks() to make
> sure we cover the cow blocks overlap case that xfs_map_cow() used to
> handle.
>
> Shouldn't passing this check effectively jump straight to to the buffer
> add? Otherwise it looks like we unconditionally attempt to allocate
> blocks in the cow fork.
>
> Alternatively...
It's all a bit of a mess. I've now bitten the bullet and added a
modification counter to the ifork and use that instead of our
previous hacks. Still work in progress, though.
