| Message ID | 20190117192004.49346-5-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: properly invalidate cached writeback mapping | expand |
On 1/17/19 12:20 PM, Brian Foster wrote: > Now that the cached writeback mapping is explicitly invalidated on > data fork changes, the EOF trimming band-aid is no longer necessary. > Remove xfs_trim_extent_eof() as well since it has no other users. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 11 ----------- > fs/xfs/libxfs/xfs_bmap.h | 1 - > fs/xfs/xfs_aops.c | 15 --------------- > 3 files changed, 27 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 332eefa2700b..4c73927819c2 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3685,17 +3685,6 @@ xfs_trim_extent( > } > } > > -/* trim extent to within eof */ > -void > -xfs_trim_extent_eof( > - struct xfs_bmbt_irec *irec, > - struct xfs_inode *ip) > - > -{ > - xfs_trim_extent(irec, 0, XFS_B_TO_FSB(ip->i_mount, > - i_size_read(VFS_I(ip)))); > -} > - > /* > * Trim the returned map to the required bounds > */ > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index 09d3ea97cc15..b4ff710d7250 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -181,7 +181,6 @@ static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec) > > void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > xfs_filblks_t len); > -void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index 649e4ad76add..09d8f7690e9e 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -357,19 +357,6 @@ xfs_map_blocks( > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > - /* > - * We have to make sure the cached mapping is within EOF to protect > - * against eofblocks trimming on file release leaving us with a stale > - * mapping. Otherwise, a page for a subsequent file extending buffered > - * write could get picked up by this writeback cycle and written to the > - * wrong blocks. > - * > - * Note that what we really want here is a generic mapping invalidation > - * mechanism to protect us from arbitrary extent modifying contexts, not > - * just eofblocks. > - */ > - xfs_trim_extent_eof(&wpc->imap, ip); > - > /* > * COW fork blocks can overlap data fork blocks even if the blocks > * aren't shared. COW I/O always takes precedent, so we must always > @@ -482,7 +469,6 @@ 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: > @@ -494,7 +480,6 @@ 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; > } > I'll skip this one, it sounds from the last review we decided not to keep it. Thanks though! Allison
On Thu, Jan 17, 2019 at 11:48:58PM -0700, Allison Henderson wrote: > > > On 1/17/19 12:20 PM, Brian Foster wrote: > > Now that the cached writeback mapping is explicitly invalidated on > > data fork changes, the EOF trimming band-aid is no longer necessary. > > Remove xfs_trim_extent_eof() as well since it has no other users. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/libxfs/xfs_bmap.c | 11 ----------- > > fs/xfs/libxfs/xfs_bmap.h | 1 - > > fs/xfs/xfs_aops.c | 15 --------------- > > 3 files changed, 27 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > index 332eefa2700b..4c73927819c2 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.c > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > @@ -3685,17 +3685,6 @@ xfs_trim_extent( > > } > > } > > -/* trim extent to within eof */ > > -void > > -xfs_trim_extent_eof( > > - struct xfs_bmbt_irec *irec, > > - struct xfs_inode *ip) > > - > > -{ > > - xfs_trim_extent(irec, 0, XFS_B_TO_FSB(ip->i_mount, > > - i_size_read(VFS_I(ip)))); > > -} > > - > > /* > > * Trim the returned map to the required bounds > > */ > > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > > index 09d3ea97cc15..b4ff710d7250 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.h > > +++ b/fs/xfs/libxfs/xfs_bmap.h > > @@ -181,7 +181,6 @@ static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec) > > void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > > xfs_filblks_t len); > > -void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); > > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > > int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > > void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > > index 649e4ad76add..09d8f7690e9e 100644 > > --- a/fs/xfs/xfs_aops.c > > +++ b/fs/xfs/xfs_aops.c > > @@ -357,19 +357,6 @@ xfs_map_blocks( > > if (XFS_FORCED_SHUTDOWN(mp)) > > return -EIO; > > - /* > > - * We have to make sure the cached mapping is within EOF to protect > > - * against eofblocks trimming on file release leaving us with a stale > > - * mapping. Otherwise, a page for a subsequent file extending buffered > > - * write could get picked up by this writeback cycle and written to the > > - * wrong blocks. > > - * > > - * Note that what we really want here is a generic mapping invalidation > > - * mechanism to protect us from arbitrary extent modifying contexts, not > > - * just eofblocks. > > - */ > > - xfs_trim_extent_eof(&wpc->imap, ip); > > - > > /* > > * COW fork blocks can overlap data fork blocks even if the blocks > > * aren't shared. COW I/O always takes precedent, so we must always > > @@ -482,7 +469,6 @@ 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: > > @@ -494,7 +480,6 @@ 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; > > } > > > > I'll skip this one, it sounds from the last review we decided not to keep > it. Thanks though! > This one is actually still intended to be part of the series. The previous patch to revalidate on fork sequence number changes eliminates the need for the racy EOF trimming solution. The first patch in the series was just intended to be a stable workaround for older kernels that might have the patch where xfs_trim_extent_eof() was first added. Thanks for the reviews.. Brian > Allison
On Thu, Jan 17, 2019 at 02:20:03PM -0500, Brian Foster wrote: > Now that the cached writeback mapping is explicitly invalidated on > data fork changes, the EOF trimming band-aid is no longer necessary. > Remove xfs_trim_extent_eof() as well since it has no other users. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 332eefa2700b..4c73927819c2 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3685,17 +3685,6 @@ xfs_trim_extent( } } -/* trim extent to within eof */ -void -xfs_trim_extent_eof( - struct xfs_bmbt_irec *irec, - struct xfs_inode *ip) - -{ - xfs_trim_extent(irec, 0, XFS_B_TO_FSB(ip->i_mount, - i_size_read(VFS_I(ip)))); -} - /* * Trim the returned map to the required bounds */ diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 09d3ea97cc15..b4ff710d7250 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -181,7 +181,6 @@ static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec) void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, xfs_filblks_t len); -void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 649e4ad76add..09d8f7690e9e 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -357,19 +357,6 @@ xfs_map_blocks( if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; - /* - * We have to make sure the cached mapping is within EOF to protect - * against eofblocks trimming on file release leaving us with a stale - * mapping. Otherwise, a page for a subsequent file extending buffered - * write could get picked up by this writeback cycle and written to the - * wrong blocks. - * - * Note that what we really want here is a generic mapping invalidation - * mechanism to protect us from arbitrary extent modifying contexts, not - * just eofblocks. - */ - xfs_trim_extent_eof(&wpc->imap, ip); - /* * COW fork blocks can overlap data fork blocks even if the blocks * aren't shared. COW I/O always takes precedent, so we must always @@ -482,7 +469,6 @@ 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: @@ -494,7 +480,6 @@ 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; }
Now that the cached writeback mapping is explicitly invalidated on data fork changes, the EOF trimming band-aid is no longer necessary. Remove xfs_trim_extent_eof() as well since it has no other users. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 11 ----------- fs/xfs/libxfs/xfs_bmap.h | 1 - fs/xfs/xfs_aops.c | 15 --------------- 3 files changed, 27 deletions(-)