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

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

[Darrick J. Wong]

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>
---
 fs/xfs/xfs_aops.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 5bf37afae5e9..4304c6416fbb 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -346,8 +346,8 @@ xfs_map_blocks(
 	ssize_t			count = i_blocksize(inode);
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, offset + count);
-	xfs_fileoff_t		cow_fsb = NULLFILEOFF;
-	int			whichfork = XFS_DATA_FORK;
+	xfs_fileoff_t		cow_fsb;
+	int			whichfork;
 	struct xfs_bmbt_irec	imap;
 	struct xfs_iext_cursor	icur;
 	int			retries = 0;
@@ -381,6 +381,8 @@ xfs_map_blocks(
 	 * landed in a hole and we skip the block.
 	 */
 retry:
+	cow_fsb = NULLFILEOFF;
+	whichfork = XFS_DATA_FORK;
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	ASSERT(ip->i_df.if_format != XFS_DINODE_FMT_BTREE ||
 	       (ip->i_df.if_flags & XFS_IFEXTENTS));

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

[Christoph Hellwig]

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>