Re: [PATCH] xfs: fix missing CoW blocks writeback conversion retry

[Christoph Hellwig <hch@infradead.org>]

On Tue, Nov 03, 2020 at 09:27:32AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>

Yes, this looks pretty sensible:

Reviewed-by: Christoph Hellwig <hch@lst.de>