diff mbox series

[1/4] xfs: eof trim writeback mapping as soon as it is cached

Message ID 20190111123032.31538-2-bfoster@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs: properly invalidate cached writeback mapping | expand

Commit Message

Brian Foster Jan. 11, 2019, 12:30 p.m. UTC 
The cached writeback mapping is EOF trimmed to try and avoid races
between post-eof block management and writeback that result in
sending cached data to a stale location. The cached mapping is
currently trimmed on the validation check, which leaves a race
window between the time the mapping is cached and when it is trimmed
against the current inode size.

For example, if a new mapping is cached by delalloc conversion on a
blocksize == page size fs, we could cycle various locks, perform
memory allocations, etc.  in the writeback codepath before the
associated mapping is eventually trimmed to i_size. This leaves
enough time for a post-eof truncate and file append before the
cached mapping is trimmed. The former event essentially invalidates
a range of the cached mapping and the latter bumps the inode size
such the trim on the next writepage event won't trim all of the
invalid blocks. fstest generic/464 reproduces this scenario
occasionally and causes a lost writeback and stale delalloc blocks
warning on inode inactivation.

To work around this problem, trim the cached writeback mapping as
soon as it is cached in addition to on subsequent validation checks.
This is a minor tweak to tighten the race window as much as possible
until a proper invalidation mechanism is available.

Fixes: 40214d128e07 ("xfs: trim writepage mapping to within eof")
Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_aops.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Sasha Levin Jan. 16, 2019, 1:35 p.m. UTC | #1 
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 40214d128e07 xfs: trim writepage mapping to within eof.

The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93, v4.9.150.

v4.20.2: Build OK!
v4.19.15: Build OK!
v4.14.93: Failed to apply! Possible dependencies:
    2d5f4b5bebcc ("xfs: remove unused parameter from xfs_writepage_map")
    5c665e5b5af6 ("xfs: remove xfs_map_cow")
    70c57dcd606f ("xfs: skip CoW writes past EOF when writeback races with truncate")
    a7b28f72ab90 ("xfs: don't use XFS_BMAPI_IGSTATE in xfs_map_blocks")
    b4d8ad7fd3a1 ("xfs: fix s_maxbytes overflow problems")

v4.9.150: Failed to apply! Possible dependencies:
    08438b1e386b ("xfs: plumb in needed functions for range querying of the freespace btrees")
    092d5d9d5812 ("xfs: cleanup xfs_reflink_find_cow_mapping")
    11ef38afe98c ("xfs: make xfs btree stats less huge")
    2d5f4b5bebcc ("xfs: remove unused parameter from xfs_writepage_map")
    5c665e5b5af6 ("xfs: remove xfs_map_cow")
    70c57dcd606f ("xfs: skip CoW writes past EOF when writeback races with truncate")
    755c7bf5ddca ("libxfs: convert ushort to unsigned short")
    a7b28f72ab90 ("xfs: don't use XFS_BMAPI_IGSTATE in xfs_map_blocks")
    af7d20fd83d9 ("xfs: make xfs_btree_magic more generic")
    b4d8ad7fd3a1 ("xfs: fix s_maxbytes overflow problems")
    c8ce540db5f6 ("xfs: remove double-underscore integer types")
    cae028df5344 ("xfs: optimise CRC updates")


How should we proceed with this patch?

--
Thanks,
Sasha
Brian Foster Jan. 16, 2019, 2:10 p.m. UTC | #2 
On Wed, Jan 16, 2019 at 01:35:38PM +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 40214d128e07 xfs: trim writepage mapping to within eof.
> 
> The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93, v4.9.150.
> 
> v4.20.2: Build OK!
> v4.19.15: Build OK!
> v4.14.93: Failed to apply! Possible dependencies:
>     2d5f4b5bebcc ("xfs: remove unused parameter from xfs_writepage_map")
>     5c665e5b5af6 ("xfs: remove xfs_map_cow")
>     70c57dcd606f ("xfs: skip CoW writes past EOF when writeback races with truncate")
>     a7b28f72ab90 ("xfs: don't use XFS_BMAPI_IGSTATE in xfs_map_blocks")
>     b4d8ad7fd3a1 ("xfs: fix s_maxbytes overflow problems")
> 
> v4.9.150: Failed to apply! Possible dependencies:
>     08438b1e386b ("xfs: plumb in needed functions for range querying of the freespace btrees")
>     092d5d9d5812 ("xfs: cleanup xfs_reflink_find_cow_mapping")
>     11ef38afe98c ("xfs: make xfs btree stats less huge")
>     2d5f4b5bebcc ("xfs: remove unused parameter from xfs_writepage_map")
>     5c665e5b5af6 ("xfs: remove xfs_map_cow")
>     70c57dcd606f ("xfs: skip CoW writes past EOF when writeback races with truncate")
>     755c7bf5ddca ("libxfs: convert ushort to unsigned short")
>     a7b28f72ab90 ("xfs: don't use XFS_BMAPI_IGSTATE in xfs_map_blocks")
>     af7d20fd83d9 ("xfs: make xfs_btree_magic more generic")
>     b4d8ad7fd3a1 ("xfs: fix s_maxbytes overflow problems")
>     c8ce540db5f6 ("xfs: remove double-underscore integer types")
>     cae028df5344 ("xfs: optimise CRC updates")
> 
> 
> How should we proceed with this patch?
> 

The writeback code in XFS has seen a decent amount of rework since these
older kernels. I'm not quite sure how stable deals with these conflicts,
but for reference, I think the appended (untested) diff is essentially
equivalent for the above two kernels. It doesn't cover the xfs_map_cow()
case in 4.14, but that code is experimental. Also note that the upstream
patch is still technically not reviewed.

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index b0cccf8a81a8..b93b3064de20 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -421,8 +421,10 @@ xfs_map_blocks(
 	    (!nimaps || isnullstartblock(imap->br_startblock))) {
 		error = xfs_iomap_write_allocate(ip, XFS_DATA_FORK, offset,
 				imap);
-		if (!error)
+		if (!error) {
 			trace_xfs_map_blocks_alloc(ip, offset, count, type, imap);
+			xfs_trim_extent_eof(imap, ip);
+		}
 		return error;
 	}
 
@@ -433,8 +435,10 @@ xfs_map_blocks(
 		ASSERT(imap->br_startblock != DELAYSTARTBLOCK);
 	}
 #endif
-	if (nimaps)
+	if (nimaps) {
 		trace_xfs_map_blocks_found(ip, offset, count, type, imap);
+		xfs_trim_extent_eof(imap, ip);
+	}
 	return 0;
 }
diff mbox series

Patch

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 338b9d9984e0..d9048bcea49c 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -449,6 +449,7 @@  xfs_map_blocks(
 	}
 
 	wpc->imap = imap;
+	xfs_trim_extent_eof(&wpc->imap, ip);
 	trace_xfs_map_blocks_found(ip, offset, count, wpc->io_type, &imap);
 	return 0;
 allocate_blocks:
@@ -459,6 +460,7 @@  xfs_map_blocks(
 	ASSERT(whichfork == XFS_COW_FORK || cow_fsb == NULLFILEOFF ||
 	       imap.br_startoff + imap.br_blockcount <= cow_fsb);
 	wpc->imap = imap;
+	xfs_trim_extent_eof(&wpc->imap, ip);
 	trace_xfs_map_blocks_alloc(ip, offset, count, wpc->io_type, &imap);
 	return 0;
 }