On Tue, Nov 03, 2020 at 09:27:32AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> In commit 7588cbeec6df, we tried to fix a race stemming from the lack of
> coordination between higher level code that wants to allocate and remap
> CoW fork extents into the data fork. Christoph cites as examples the
> always_cow mode, and a directio write completion racing with writeback.
>
> According to the comments before the goto retry, we want to restart the
> lookup to catch the extent in the data fork, but we don't actually reset
> whichfork or cow_fsb, which means the second try executes using stale
> information. Up until now I think we've gotten lucky that either
> there's something left in the CoW fork to cause cow_fsb to be reset, or
> either data/cow fork sequence numbers have advanced enough to force a
> fresh lookup from the data fork. However, if we reach the retry with an
> empty stable CoW fork and a stable data fork, neither of those things
> happens. The retry foolishly re-calls xfs_convert_blocks on the CoW
> fork which fails again. This time, we toss the write.
>
> I've recently been working on extending reflink to the realtime device.
> When the realtime extent size is larger than a single block, we have to
> force the page cache to CoW the entire rt extent if a write (or
> fallocate) are not aligned with the rt extent size. The strategy I've
> chosen to deal with this is derived from Dave's blocksize > pagesize
> series: dirtying around the write range, and ensuring that writeback
> always starts mapping on an rt extent boundary. This has brought this
> race front and center, since generic/522 blows up immediately.
>
> However, I'm pretty sure this is a bug outright, independent of that.
>
> Fixes: 7588cbeec6df ("xfs: retry COW fork delalloc conversion when no extent was found")
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Yes, this looks pretty sensible:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
