Re: [PATCH v2] xfs: fix COW writeback race

[Christoph Hellwig <hch@lst.de>]

On Tue, Jan 17, 2017 at 12:14:51PM -0500, Brian Foster wrote:
> Got it, thanks. So all of the writeback stuff is protected via
> page/buffer locks, and even if we still had those locks, it doesn't
> matter because the same extent is obviously covered by many page/buffer
> objects.

Yes.

> Yeah, and doing otherwise may break the assumption that larger delallocs
> produce larger physical allocs (re: cowextsz hint and potentially
> preallocation).

Yes - especially for the COW case this might be very important.

> That seems reasonable so long as we skip the parts of the loop that are
> expecting a real (non-hole) startblock.

Agreed.

> Things like the above had me thinking it might be more clear to
> explicitly read the extent and check for delalloc in the caller while
> under the appropriate lock (and if XFS_COW_FORK). That's kind of what I
> was alluding to above wrt to closing the race. That's just an idea,
> however, and doesn't necessarily improve the error handling in the way
> that this patch does (to avoid the transaction overrun). Given that, I'm
> not against what this patch is currently doing so long as we fix up the
> rest of the loop. Your idea of xfs_bmapi_convert() or some such sounds
> like a nice potential cleanup at some point too.

I don't like that idea because it just means even more extent lookups.
xfs_bmapi_write has to read in the extents anyway, so instead of doing
another read under the same lock we'd better reuse this one.