xfs: recheck reflink / dirty page status before freeing CoW reservations

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

Quoted 18 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Quoted 7 lines

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

I applied this patch on top of v4.15-rc5 kernel, and ran generic/083 generic/269 and generic/270 (where I hit the soft lockup and hang before) multiple times and tests all passed. I also ran all tests in 'enospc' group on 1k/2k/4k XFS with reflink enabled, tests passed too. So

Tested-by Eryu Guan <[email protected]>

Quoted 32 lines · Eryu Guan

Quoted 18 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Quoted 7 lines

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

I applied this patch on top of v4.15-rc5 kernel, and ran generic/083 generic/269 and generic/270 (where I hit the soft lockup and hang before) multiple times and tests all passed. I also ran all tests in 'enospc' group on 1k/2k/4k XFS with reflink enabled, tests passed too. So

Quoted 1 line

Tested-by Eryu Guan <[email protected]>

Sorry, I have to withdraw this tag for now.. I'm seeing soft lockup again in generic/269 run with the patched kernel. I'll do more testings to confirm, paste the soft lockup info here for now:

Quoted 33 lines · Eryu Guan

Quoted 32 lines · Eryu Guan

Quoted 18 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Quoted 7 lines

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

I applied this patch on top of v4.15-rc5 kernel, and ran generic/083 generic/269 and generic/270 (where I hit the soft lockup and hang before) multiple times and tests all passed. I also ran all tests in 'enospc' group on 1k/2k/4k XFS with reflink enabled, tests passed too. So

Quoted 5 lines

Quoted 1 line

Tested-by Eryu Guan <[email protected]>

Sorry, I have to withdraw this tag for now.. I'm seeing soft lockup again in generic/269 run with the patched kernel. I'll do more testings to confirm, paste the soft lockup info here for now:

I ran generic/269 for over 4000 iterations and didn't hit soft lockup, I suspect that previously I tested on wrong/unpatched xfs module..

But occationally I saw fs inconsistency in generic/269, it's hard to reproduce (need 100-200 iterations) but I did see it several times. But it seems like another problem.

_check_xfs_filesystem: filesystem on /dev/sda6 is inconsistent (r) *** xfs_repair -n output ***
Phase 1 - find and verify superblock...
Phase 2 - using internal log
- zero log...
- scan filesystem freespace and inode maps...
sb_fdblocks 8178, counted 8188
- found root inode chunk
Phase 3 - for each AG...
- scan (but don't clear) agi unlinked lists...
- process known inodes and perform inode discovery...
- agno = 0
- agno = 1
- agno = 2
- agno = 3
- process newly discovered inodes...
Phase 4 - check for duplicate blocks...
- setting up duplicate extent list...
- check for inodes claiming duplicate blocks...
- agno = 1
- agno = 3
- agno = 2
- agno = 0
No modify flag set, skipping phase 5
Phase 6 - check inode connectivity...
- traversing filesystem ...
- traversal finished ...
- moving disconnected inodes to lost+found ...
Phase 7 - verify link counts...
No modify flag set, skipping filesystem flush and exiting. *** end xfs_repair output

Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c |    7 +++++
 fs/xfs/xfs_icache.c      |   61 +++++++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 22 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +0 -0

index a01cef4..7bd933f 100644

fs/xfs/libxfs/xfs_bmap.c +0 -0

 		/* in hole or beyoned EOF? */
 		if (eof || bma.got.br_startoff > bno) {
 			if (flags & XFS_BMAPI_DELALLOC) {

fs/xfs/xfs_icache.c +4 -0

index 1f84562..3fbcc03 100644
@@ -1654,6 +1654,35 @@ xfs_inode_clear_eofblocks_tag(
 			trace_xfs_perag_clear_eofblocks, XFS_ICI_EOFBLOCKS_TAG);
 }
 
+/* Is this a good time to reap the CoW reservations for this file? */
+static bool
+xfs_can_free_cowblocks(
+	struct xfs_inode	*ip,

fs/xfs/xfs_icache.c +25 -23

+	}
+
+	/*
+	 * If the mapping is dirty or under writeback we cannot touch the
+	 * CoW fork.  Leave it alone if we're in the midst of a directio.
+	 */
+	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
+	    atomic_read(&VFS_I(ip)->i_dio_count))
+		return false;
+
+	return true;
+}
+
 /*
  * Automatic CoW Reservation Freeing
  *

@@ -1672,29 +1701,12 @@ xfs_inode_free_cowblocks(
 	int			flags,
 	void			*args)
 {
-	int ret;
-	struct xfs_eofblocks *eofb = args;
-	int match;
+	struct xfs_eofblocks	*eofb = args;
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	int			match;
+	int			ret = 0;
 
-	/*
-	 * Just clear the tag if we have an empty cow fork or none at all. It's
-	 * possible the inode was fully unshared since it was originally tagged.
-	 */
-	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
-		trace_xfs_inode_free_cowblocks_invalid(ip);
-		xfs_inode_clear_cowblocks_tag(ip);
-		return 0;
-	}
-
-	/*
-	 * If the mapping is dirty or under writeback we cannot touch the
-	 * CoW fork.  Leave it alone if we're in the midst of a directio.
-	 */
-	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
-	    atomic_read(&VFS_I(ip)->i_dio_count))
+	if (!xfs_can_free_cowblocks(ip, ifp))
 		return 0;
 
 	if (eofb) {

@@ -1715,7 +1727,12 @@ xfs_inode_free_cowblocks(
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 
-	ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
+	/*
+	 * Check again, nobody else should be able to dirty blocks or change
+	 * the reflink iflag now that we have the first two locks held.
+	 */
+	if (xfs_can_free_cowblocks(ip, ifp))
+		ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
 
 	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 38 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c |    7 +++++
 fs/xfs/xfs_icache.c      |   61 +++++++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 22 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +0 -0

index a01cef4..7bd933f 100644
@@ -4311,6 +4311,13 @@ xfs_bmapi_write(
 	while (bno < end && n < *nmap) {
 		bool			need_alloc = false, wasdelay = false;
 

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_write( · line 4320

Quoted 7 lines

fs/xfs/libxfs/xfs_bmap.c +7 -0

+		/*
+		 * CoW fork conversions should /never/ hit EOF.  There should
+		 * always be something for us to work on.
+		 */
+		ASSERT(!eof || !(flags & XFS_BMAPI_CONVERT) ||
+			       !(flags & XFS_BMAPI_COWFORK));
+

The hunk just below asserts for BMAPI_COWFORK in a case that explicitly considers eof. That makes the logic confusing to follow IMO, but I'm more wondering whether pushing something like ASSERT(!((flags & CONVERT) && (flags & COWFORK))) down into that hunk is effectively the same thing..? I.e., is it also true that we should not find a hole in the (CONVERT & COW) case?

Quoted 22 lines

fs/xfs/libxfs/xfs_bmap.c +0 -0

 		/* in hole or beyoned EOF? */
 		if (eof || bma.got.br_startoff > bno) {
 			if (flags & XFS_BMAPI_DELALLOC) {

fs/xfs/xfs_icache.c +11 -0

index 1f84562..3fbcc03 100644
@@ -1654,6 +1654,35 @@ xfs_inode_clear_eofblocks_tag(
 			trace_xfs_perag_clear_eofblocks, XFS_ICI_EOFBLOCKS_TAG);
 }
 
+/* Is this a good time to reap the CoW reservations for this file? */
+static bool
+xfs_can_free_cowblocks(
+	struct xfs_inode	*ip,
+	struct xfs_ifork	*ifp)
+{
+	/*
+	 * Just clear the tag if we have an empty cow fork or none at all. It's
+	 * possible the inode was fully unshared since it was originally tagged.
+	 */
+	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {

fs/xfs/xfs_icache.c xfs_inode_clear_eofblocks_tag( · line 1670

Quoted 3 lines

fs/xfs/xfs_icache.c +3 -0

+		trace_xfs_inode_free_cowblocks_invalid(ip);
+		xfs_inode_clear_cowblocks_tag(ip);
+		return false;

I think the flag update and tracepoint should probably remain in the caller. They're somewhat misplaced for a "xfs_can_do_something()" helper, particularly if it's ever exported and used in other contexts in the future. Otherwise seems fine.

Brian

Quoted 67 lines

fs/xfs/xfs_icache.c +25 -23

+	}
+
+	/*
+	 * If the mapping is dirty or under writeback we cannot touch the
+	 * CoW fork.  Leave it alone if we're in the midst of a directio.
+	 */
+	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
+	    atomic_read(&VFS_I(ip)->i_dio_count))
+		return false;
+
+	return true;
+}
+
 /*
  * Automatic CoW Reservation Freeing
  *

@@ -1672,29 +1701,12 @@ xfs_inode_free_cowblocks(
 	int			flags,
 	void			*args)
 {
-	int ret;
-	struct xfs_eofblocks *eofb = args;
-	int match;
+	struct xfs_eofblocks	*eofb = args;
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	int			match;
+	int			ret = 0;
 
-	/*
-	 * Just clear the tag if we have an empty cow fork or none at all. It's
-	 * possible the inode was fully unshared since it was originally tagged.
-	 */
-	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
-		trace_xfs_inode_free_cowblocks_invalid(ip);
-		xfs_inode_clear_cowblocks_tag(ip);
-		return 0;
-	}
-
-	/*
-	 * If the mapping is dirty or under writeback we cannot touch the
-	 * CoW fork.  Leave it alone if we're in the midst of a directio.
-	 */
-	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
-	    atomic_read(&VFS_I(ip)->i_dio_count))
+	if (!xfs_can_free_cowblocks(ip, ifp))
 		return 0;
 
 	if (eofb) {

@@ -1715,7 +1727,12 @@ xfs_inode_free_cowblocks(
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 
-	ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
+	/*
+	 * Check again, nobody else should be able to dirty blocks or change
+	 * the reflink iflag now that we have the first two locks held.
+	 */
+	if (xfs_can_free_cowblocks(ip, ifp))
+		ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
 
 	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 39 lines · Brian Foster

Quoted 38 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c |    7 +++++
 fs/xfs/xfs_icache.c      |   61 +++++++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 22 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +0 -0

index a01cef4..7bd933f 100644
@@ -4311,6 +4311,13 @@ xfs_bmapi_write(
 	while (bno < end && n < *nmap) {
 		bool			need_alloc = false, wasdelay = false;
 

Quoted 14 lines

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_write( · line 4320

Quoted 7 lines

fs/xfs/libxfs/xfs_bmap.c +7 -0

+		/*
+		 * CoW fork conversions should /never/ hit EOF.  There should
+		 * always be something for us to work on.
+		 */
+		ASSERT(!eof || !(flags & XFS_BMAPI_CONVERT) ||
+			       !(flags & XFS_BMAPI_COWFORK));
+

The hunk just below asserts for BMAPI_COWFORK in a case that explicitly considers eof. That makes the logic confusing to follow IMO, but I'm more wondering whether pushing something like ASSERT(!((flags & CONVERT) && (flags & COWFORK))) down into that hunk is effectively the same thing..? I.e., is it also true that we should not find a hole in the (CONVERT & COW) case?

Yes.

Quoted 22 lines

Quoted 22 lines

fs/xfs/libxfs/xfs_bmap.c +0 -0

 		/* in hole or beyoned EOF? */
 		if (eof || bma.got.br_startoff > bno) {
 			if (flags & XFS_BMAPI_DELALLOC) {

fs/xfs/xfs_icache.c +11 -0

index 1f84562..3fbcc03 100644
@@ -1654,6 +1654,35 @@ xfs_inode_clear_eofblocks_tag(
 			trace_xfs_perag_clear_eofblocks, XFS_ICI_EOFBLOCKS_TAG);
 }
 
+/* Is this a good time to reap the CoW reservations for this file? */
+static bool
+xfs_can_free_cowblocks(
+	struct xfs_inode	*ip,
+	struct xfs_ifork	*ifp)
+{
+	/*
+	 * Just clear the tag if we have an empty cow fork or none at all. It's
+	 * possible the inode was fully unshared since it was originally tagged.
+	 */
+	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {

Quoted 8 lines

fs/xfs/xfs_icache.c xfs_inode_clear_eofblocks_tag( · line 1670

Quoted 3 lines

fs/xfs/xfs_icache.c +3 -0

+		trace_xfs_inode_free_cowblocks_invalid(ip);
+		xfs_inode_clear_cowblocks_tag(ip);
+		return false;

I think the flag update and tracepoint should probably remain in the caller. They're somewhat misplaced for a "xfs_can_do_something()" helper, particularly if it's ever exported and used in other contexts in the future. Otherwise seems fine.

Hmm. There's a subtlety to step around here, which is that this predicate can return false to mean "nothing here to see" or to mean "cannot clear anything at this time". We want the trace+clear for the first case, but not the second.

I suppose this function could return the regular error code int and the caller can figure out what that means to it, but then all the post-check stuff ends up duplicated in the callers... so maybe I should just rename it xfs_prep_free_cowblocks().

And change the comment to:

/*
* Set ourselves up to free CoW blocks from this file. If it's already
* clean then we can bail out quickly, but otherwise we must back off if
* the file is undergoing some kind of write.
*/

--D

Quoted 66 lines

Brian

Quoted 64 lines

fs/xfs/xfs_icache.c +25 -22

+	}
+
+	/*
+	 * If the mapping is dirty or under writeback we cannot touch the
+	 * CoW fork.  Leave it alone if we're in the midst of a directio.
+	 */
+	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
+	    atomic_read(&VFS_I(ip)->i_dio_count))
+		return false;
+
+	return true;
+}
+
 /*
  * Automatic CoW Reservation Freeing
  *

@@ -1672,29 +1701,12 @@ xfs_inode_free_cowblocks(
 	int			flags,
 	void			*args)
 {
-	int ret;
-	struct xfs_eofblocks *eofb = args;
-	int match;
+	struct xfs_eofblocks	*eofb = args;
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	int			match;
+	int			ret = 0;
 
-	/*
-	 * Just clear the tag if we have an empty cow fork or none at all. It's
-	 * possible the inode was fully unshared since it was originally tagged.
-	 */
-	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
-		trace_xfs_inode_free_cowblocks_invalid(ip);
-		xfs_inode_clear_cowblocks_tag(ip);
-		return 0;
-	}
-
-	/*
-	 * If the mapping is dirty or under writeback we cannot touch the
-	 * CoW fork.  Leave it alone if we're in the midst of a directio.
-	 */
-	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
-	    atomic_read(&VFS_I(ip)->i_dio_count))
+	if (!xfs_can_free_cowblocks(ip, ifp))
 		return 0;
 
 	if (eofb) {

@@ -1715,7 +1727,12 @@ xfs_inode_free_cowblocks(
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 
-	ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
+	/*
+	 * Check again, nobody else should be able to dirty blocks or change
+	 * the reflink iflag now that we have the first two locks held.
+	 */
+	if (xfs_can_free_cowblocks(ip, ifp))
+		ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
 

Quoted 10 lines

Quoted 6 lines

fs/xfs/xfs_icache.c +0 -1

 	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 93 lines · Darrick J. Wong

Quoted 39 lines · Brian Foster

Quoted 38 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The underlying issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c |    7 +++++
 fs/xfs/xfs_icache.c      |   61 +++++++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 22 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +0 -0

index a01cef4..7bd933f 100644
@@ -4311,6 +4311,13 @@ xfs_bmapi_write(
 	while (bno < end && n < *nmap) {
 		bool			need_alloc = false, wasdelay = false;
 

Quoted 14 lines

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_write( · line 4320

Quoted 7 lines

fs/xfs/libxfs/xfs_bmap.c +7 -0

+		/*
+		 * CoW fork conversions should /never/ hit EOF.  There should
+		 * always be something for us to work on.
+		 */
+		ASSERT(!eof || !(flags & XFS_BMAPI_CONVERT) ||
+			       !(flags & XFS_BMAPI_COWFORK));
+

The hunk just below asserts for BMAPI_COWFORK in a case that explicitly considers eof. That makes the logic confusing to follow IMO, but I'm more wondering whether pushing something like ASSERT(!((flags & CONVERT) && (flags & COWFORK))) down into that hunk is effectively the same thing..? I.e., is it also true that we should not find a hole in the (CONVERT & COW) case?

Yes.

Quoted 22 lines

Quoted 22 lines

fs/xfs/libxfs/xfs_bmap.c +0 -0

 		/* in hole or beyoned EOF? */
 		if (eof || bma.got.br_startoff > bno) {
 			if (flags & XFS_BMAPI_DELALLOC) {

fs/xfs/xfs_icache.c +11 -0

index 1f84562..3fbcc03 100644
@@ -1654,6 +1654,35 @@ xfs_inode_clear_eofblocks_tag(
 			trace_xfs_perag_clear_eofblocks, XFS_ICI_EOFBLOCKS_TAG);
 }
 
+/* Is this a good time to reap the CoW reservations for this file? */
+static bool
+xfs_can_free_cowblocks(
+	struct xfs_inode	*ip,
+	struct xfs_ifork	*ifp)
+{
+	/*
+	 * Just clear the tag if we have an empty cow fork or none at all. It's
+	 * possible the inode was fully unshared since it was originally tagged.
+	 */
+	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {

Quoted 8 lines

fs/xfs/xfs_icache.c xfs_inode_clear_eofblocks_tag( · line 1670

Quoted 3 lines

fs/xfs/xfs_icache.c +3 -0

+		trace_xfs_inode_free_cowblocks_invalid(ip);
+		xfs_inode_clear_cowblocks_tag(ip);
+		return false;

I think the flag update and tracepoint should probably remain in the caller. They're somewhat misplaced for a "xfs_can_do_something()" helper, particularly if it's ever exported and used in other contexts in the future. Otherwise seems fine.

Hmm. There's a subtlety to step around here, which is that this predicate can return false to mean "nothing here to see" or to mean "cannot clear anything at this time". We want the trace+clear for the first case, but not the second.

Quoted 5 lines

I suppose this function could return the regular error code int and the caller can figure out what that means to it, but then all the post-check stuff ends up duplicated in the callers... so maybe I should just rename it xfs_prep_free_cowblocks().

Are both checks necessarily required to be repeated under lock to fix the bug? IOW, Could the !fork || !flag check remain in the caller to cover the first case?

Quoted 8 lines

And change the comment to:

/*
* Set ourselves up to free CoW blocks from this file. If it's already
* clean then we can bail out quickly, but otherwise we must back off if
* the file is undergoing some kind of write.
*/

That sounds reasonable too.

Brian

Quoted 78 lines

--D

Quoted 66 lines

Brian

Quoted 64 lines

fs/xfs/xfs_icache.c +25 -22

+	}
+
+	/*
+	 * If the mapping is dirty or under writeback we cannot touch the
+	 * CoW fork.  Leave it alone if we're in the midst of a directio.
+	 */
+	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
+	    atomic_read(&VFS_I(ip)->i_dio_count))
+		return false;
+
+	return true;
+}
+
 /*
  * Automatic CoW Reservation Freeing
  *

@@ -1672,29 +1701,12 @@ xfs_inode_free_cowblocks(
 	int			flags,
 	void			*args)
 {
-	int ret;
-	struct xfs_eofblocks *eofb = args;
-	int match;
+	struct xfs_eofblocks	*eofb = args;
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	int			match;
+	int			ret = 0;
 
-	/*
-	 * Just clear the tag if we have an empty cow fork or none at all. It's
-	 * possible the inode was fully unshared since it was originally tagged.
-	 */
-	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
-		trace_xfs_inode_free_cowblocks_invalid(ip);
-		xfs_inode_clear_cowblocks_tag(ip);
-		return 0;
-	}
-
-	/*
-	 * If the mapping is dirty or under writeback we cannot touch the
-	 * CoW fork.  Leave it alone if we're in the midst of a directio.
-	 */
-	if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
-	    atomic_read(&VFS_I(ip)->i_dio_count))
+	if (!xfs_can_free_cowblocks(ip, ifp))
 		return 0;
 
 	if (eofb) {

@@ -1715,7 +1727,12 @@ xfs_inode_free_cowblocks(
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 
-	ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
+	/*
+	 * Check again, nobody else should be able to dirty blocks or change
+	 * the reflink iflag now that we have the first two locks held.
+	 */
+	if (xfs_can_free_cowblocks(ip, ifp))
+		ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
 

Quoted 10 lines

Quoted 6 lines

fs/xfs/xfs_icache.c +0 -1

 	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Eryu Guan reported seeing occasional hangs when running generic/269 with a new fsstress that supports clonerange/deduperange. The cause of this hang is an infinite loop when we convert the CoW fork extents from unwritten to real just prior to writing the pages out; the infinite loop happens because there's nothing in the CoW fork to convert, and so it spins forever.

The fundamental issue here is that when we go to perform these CoW fork conversions, we're supposed to have an extent waiting for us, but the low space CoW reaper has snuck in and blown them away! There are four conditions that can dissuade the reaper from touching our file -- no reflink iflag; dirty page cache; writeback in progress; or directio in progress. We check the four conditions prior to taking the locks, but we neglect to recheck them once we have the locks, which is how we end up whacking the writeback that's in progress.

Therefore, refactor the four checks into a helper function and call it once again once we have the locks to make sure we really want to reap the inode. While we're at it, add an ASSERT for this weird condition so that we'll fail noisily if we ever screw this up again.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

Tested-by Eryu Guan <[email protected]>

---
v2: improve comments, minor refactors suggested by Brian Foster
---

 fs/xfs/libxfs/xfs_bmap.c |   10 +++++++
 fs/xfs/xfs_icache.c      |   63 +++++++++++++++++++++++++++++++---------------
 2 files changed, 51 insertions(+), 22 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +9 -1

index a01cef4..3567db6 100644
@@ -4311,8 +4311,16 @@ xfs_bmapi_write(
 	while (bno < end && n < *nmap) {
 		bool			need_alloc = false, wasdelay = false;
 
-		/* in hole or beyoned EOF? */
+		/* in hole or beyond EOF? */
 		if (eof || bma.got.br_startoff > bno) {
+			/*
+			 * CoW fork conversions should /never/ hit EOF or
+			 * holes.  There should always be something for us
+			 * to work on.
+			 */
+			ASSERT(!((flags & XFS_BMAPI_CONVERT) &&
+			         (flags & XFS_BMAPI_COWFORK)));
+
 			if (flags & XFS_BMAPI_DELALLOC) {
 				/*
 				 * For the COW fork we can reasonably get a

fs/xfs/xfs_icache.c +42 -22

index 1f84562..76df647 100644
@@ -1655,28 +1655,15 @@ xfs_inode_clear_eofblocks_tag(
 }
 
 /*
- * Automatic CoW Reservation Freeing
- *
- * These functions automatically garbage collect leftover CoW reservations
- * that were made on behalf of a cowextsize hint when we start to run out
- * of quota or when the reservations sit around for too long.  If the file
- * has dirty pages or is undergoing writeback, its CoW reservations will
- * be retained.
- *
- * The actual garbage collection piggybacks off the same code that runs
- * the speculative EOF preallocation garbage collector.
+ * Set ourselves up to free CoW blocks from this file.  If it's already clean
+ * then we can bail out quickly, but otherwise we must back off if the file
+ * is undergoing some kind of write.
  */
-STATIC int
-xfs_inode_free_cowblocks(
+static bool
+xfs_prep_free_cowblocks(
 	struct xfs_inode	*ip,
-	int			flags,
-	void			*args)
+	struct xfs_ifork	*ifp)
 {
-	int ret;
-	struct xfs_eofblocks *eofb = args;
-	int match;
-	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
-
 	/*
 	 * Just clear the tag if we have an empty cow fork or none at all. It's
 	 * possible the inode was fully unshared since it was originally tagged.

@@ -1684,7 +1671,7 @@ xfs_inode_free_cowblocks(
 	if (!xfs_is_reflink_inode(ip) || !ifp->if_bytes) {
 		trace_xfs_inode_free_cowblocks_invalid(ip);
 		xfs_inode_clear_cowblocks_tag(ip);
-		return 0;
+		return false;
 	}
 
 	/*

@@ -1695,6 +1682,35 @@ xfs_inode_free_cowblocks(
 	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
 	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
 	    atomic_read(&VFS_I(ip)->i_dio_count))
+		return false;
+
+	return true;
+}
+
+/*
+ * Automatic CoW Reservation Freeing
+ *
+ * These functions automatically garbage collect leftover CoW reservations
+ * that were made on behalf of a cowextsize hint when we start to run out
+ * of quota or when the reservations sit around for too long.  If the file
+ * has dirty pages or is undergoing writeback, its CoW reservations will
+ * be retained.
+ *
+ * The actual garbage collection piggybacks off the same code that runs
+ * the speculative EOF preallocation garbage collector.
+ */
+STATIC int
+xfs_inode_free_cowblocks(
+	struct xfs_inode	*ip,
+	int			flags,
+	void			*args)
+{
+	struct xfs_eofblocks	*eofb = args;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	int			match;
+	int			ret = 0;
+
+	if (!xfs_prep_free_cowblocks(ip, ifp))
 		return 0;
 
 	if (eofb) {

@@ -1715,7 +1731,12 @@ xfs_inode_free_cowblocks(
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
 
-	ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
+	/*
+	 * Check again, nobody else should be able to dirty blocks or change
+	 * the reflink iflag now that we have the first two locks held.
+	 */
+	if (xfs_prep_free_cowblocks(ip, ifp))
+		ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
 
 	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html