xfs: factor out nodiscard helpers
PATCH 1/1 xfs: factor out nodiscard helpers
The changes to skip discards of speculative preallocation and unwritten extents introduced several new wrapper functions through the bunmapi -> extent free codepath to reduce churn in all of the associated callers. In several cases, these wrappers simply toggle a single flag to skip or not skip discards for the resulting blocks.
The explicit _nodiscard() wrappers for such an isolated set of callers is a bit overkill. Kill off these wrappers and replace with the calls to the underlying functions in the contexts that need to control discard behavior. Retain the wrappers that preserve the original calling conventions to serve the original purpose of reducing code churn.
This is a refactoring patch and does not change behavior.
---
Hi all,
Darrick had previously indicated that he was going to pull in the v2 of the skip discard set rather than hold it up on refactoring churn. Given that I was playing around with some of Christoph's compromised feedback, here's a followup patch with the associated cleanups that I generally agree with. I actually like this factoring a bit better than that of the v2 because while it still avoids changing all callers of the associated functions, I do prefer to control discard behavior with a parameter rather than if/else statements with calls to completely different functions. The additional benefit is that this removes more code than it adds.
I left struct xfs_free_extent_item alone because I don't really agree that XFS_EXTENT_BUSY_ belong anywhere outside of the busy extent code. IOW, I'd change the xfs_extent_busy_insert() parameter before I'd change xfs_free_extent_item because, despite the flags name, the param really only has two sane values (skip discard or don't). The affected xfs_extent_busy->flags field is used as more of a state than a control flag.
Brian
fs/xfs/libxfs/xfs_alloc.h | 11 ----------- fs/xfs/libxfs/xfs_bmap.c | 16 ++++++++-------- fs/xfs/libxfs/xfs_bmap.h | 11 ----------- fs/xfs/xfs_bmap_util.c | 4 ++-- fs/xfs/xfs_inode.c | 9 +++------ fs/xfs/xfs_inode.h | 16 +++------------- fs/xfs/xfs_trans_extfree.c | 9 ++------- 7 files changed, 18 insertions(+), 58 deletions(-)
fs/xfs/libxfs/xfs_alloc.h +0 -11
@@ -210,17 +210,6 @@ xfs_free_extent(return __xfs_free_extent (tp, bno, len, oinfo, type, false); } -static inline int -xfs_free_extent_nodiscard ( -struct xfs_trans *tp, -xfs_fsblock_t bno, -xfs_extlen_t len, -struct xfs_owner_info *oinfo, -enum xfs_ag_resv_type type) -{ -return __xfs_free_extent (tp, bno, len, oinfo, type, true); -} - int /* error */ xfs_alloc_lookup_le( struct xfs_btree_cur *cur, /* btree cursor */
fs/xfs/libxfs/xfs_bmap.c +8 -8
@@ -5107,15 +5107,15 @@ xfs_bmap_del_extent_real(if (error) goto done; }else { +bool skip_discard = false; +if ((bflags &XFS_BMAPI_NODISCARD ) || - (del->br_state ==XFS_EXT_UNWRITTEN )) { -xfs_bmap_add_free_nodiscard (mp, dfops, - del->br_startblock, del->br_blockcount, -NULL ); - }else { -xfs_bmap_add_free (mp, dfops, del->br_startblock, - del->br_blockcount,NULL ); - } + (del->br_state ==XFS_EXT_UNWRITTEN )) + skip_discard = true; + +__xfs_bmap_add_free (mp, dfops, del->br_startblock, + del->br_blockcount,NULL , + skip_discard); } }
fs/xfs/libxfs/xfs_bmap.h +0 -11
@@ -256,17 +256,6 @@ xfs_bmap_add_free(__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, false); } -static inline void -xfs_bmap_add_free_nodiscard ( -struct xfs_mount *mp, -struct xfs_defer_ops *dfops, -xfs_fsblock_t bno, -xfs_filblks_t len, -struct xfs_owner_info *oinfo) -{ -__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, true); -} -enum xfs_bmap_intent_type {XFS_BMAP_MAP =1 ,XFS_BMAP_UNMAP ,
fs/xfs/xfs_bmap_util.c +2 -2
@@ -871,8 +871,8 @@ xfs_free_eofblocks(* contents of the file are flushed to disk then the files * may be full of holes (ie NULL files bug). */ - error =xfs_itruncate_extents_nodiscard (&tp, ip,XFS_DATA_FORK , -XFS_ISIZE (ip)); + error =xfs_itruncate_extents_flags (&tp, ip,XFS_DATA_FORK , +XFS_ISIZE (ip),XFS_BMAPI_NODISCARD );if (error) {/* * If we get an error at this point we simply don't
fs/xfs/xfs_inode.c +3 -6
@@ -1534,12 +1534,12 @@ xfs_itruncate_clear_reflink_flags(* dirty on error so that transactions can be easily aborted if possible. */ int -__xfs_itruncate_extents ( +xfs_itruncate_extents_flags (struct xfs_trans **tpp,struct xfs_inode *ip,int whichfork,xfs_fsize_t new_size, -bool skip_discard) +int flags) {struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp = *tpp;
@@ -1550,7 +1550,6 @@ __xfs_itruncate_extents(xfs_filblks_t unmap_len;int error =0 ;int done =0 ; -int flags;ASSERT (xfs_isilocked (ip,XFS_ILOCK_EXCL )); ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
@@ -1563,9 +1562,7 @@ __xfs_itruncate_extents(trace_xfs_itruncate_extents_start (ip, new_size); - flags =xfs_bmapi_aflag (whichfork); -if (skip_discard) - flags |=XFS_BMAPI_NODISCARD ; + flags |=xfs_bmapi_aflag (whichfork);/* * Since it is possible for space to become allocated beyond
fs/xfs/xfs_inode.h +3 -13
@@ -415,8 +415,8 @@ uint xfs_ilock_attr_map_shared(struct xfs_inode *);uint xfs_ip2xflags (struct xfs_inode *);int xfs_ifree (struct xfs_trans *,xfs_inode_t *,struct xfs_defer_ops *); -int __xfs_itruncate_extents (struct xfs_trans **,struct xfs_inode *, -int ,xfs_fsize_t ,bool ); +int xfs_itruncate_extents_flags (struct xfs_trans **, +struct xfs_inode *,int ,xfs_fsize_t ,int );void xfs_iext_realloc (xfs_inode_t *,int ,int );void xfs_iunpin_wait (xfs_inode_t *);
@@ -440,17 +440,7 @@ xfs_itruncate_extents( int whichfork, xfs_fsize_t new_size) { - return __xfs_itruncate_extents(tpp, ip, whichfork, new_size, false); -} - -static inline int -xfs_itruncate_extents_nodiscard ( -struct xfs_trans **tpp, -struct xfs_inode *ip, -int whichfork, -xfs_fsize_t new_size) -{ -return __xfs_itruncate_extents (tpp, ip, whichfork, new_size, true); +return xfs_itruncate_extents_flags (tpp, ip, whichfork, new_size,0 ); }/* from xfs_file.c */
fs/xfs/xfs_trans_extfree.c +2 -7
@@ -80,13 +80,8 @@ xfs_trans_free_extent(trace_xfs_bmap_free_deferred (tp->t_mountp, agno,0 , agbno, ext_len); -if (skip_discard) - error =xfs_free_extent_nodiscard (tp, start_block, ext_len, - oinfo,XFS_AG_RESV_NONE ); -else - error =xfs_free_extent (tp, start_block, ext_len, oinfo, -XFS_AG_RESV_NONE ); - + error =__xfs_free_extent (tp, start_block, ext_len, + oinfo,XFS_AG_RESV_NONE , skip_discard);/* * Mark the transaction dirty, even on error. This ensures the * transaction is aborted, which:
Signature · 1 line
2.14.3
This looks much better, and I think the diffstat speaks for itself..
fs/xfs/libxfs/xfs_bmap.c xfs_bmap_del_extent_real( · line 5113
Quoted 12 lines
fs/xfs/libxfs/xfs_bmap.c +3
-0@@ -5107,15 +5107,15 @@ xfs_bmap_del_extent_real(if (error) goto done; }else { +bool skip_discard = false; +if ((bflags &XFS_BMAPI_NODISCARD ) || + (del->br_state ==XFS_EXT_UNWRITTEN ))
No need for the inner braces in the second statement.
fs/xfs/libxfs/xfs_bmap.c xfs_bmap_del_extent_real( · line 5118
Quoted 5 lines
fs/xfs/libxfs/xfs_bmap.c +5
-0+ skip_discard = true; + +__xfs_bmap_add_free (mp, dfops, del->br_startblock, + del->br_blockcount,NULL , + skip_discard);
In fact I'd avoid the variable entirely:
__xfs_bmap_add_free(mp, dfops, del->br_startblock, del->br_blockcount, NULL, del->br_state == XFS_EXT_UNWRITTEN || (bflags & XFS_BMAPI_NODISCARD));
PATCH v2 1/1 xfs: factor out nodiscard helpers
In reply to Brian FosterThe changes to skip discards of speculative preallocation and unwritten extents introduced several new wrapper functions through the bunmapi -> extent free codepath to reduce churn in all of the associated callers. In several cases, these wrappers simply toggle a single flag to skip or not skip discards for the resulting blocks.
The explicit _nodiscard() wrappers for such an isolated set of callers is a bit overkill. Kill off these wrappers and replace with the calls to the underlying functions in the contexts that need to control discard behavior. Retain the wrappers that preserve the original calling conventions to serve the original purpose of reducing code churn.
This is a refactoring patch and does not change behavior.
---
v2:
- Clean up xfs_bmap_del_extent_real().
v1: https://marc.info/?l=linux-xfs&m=152595674924579&w=2
fs/xfs/libxfs/xfs_alloc.h | 11 ----------- fs/xfs/libxfs/xfs_bmap.c | 13 ++++--------- fs/xfs/libxfs/xfs_bmap.h | 11 ----------- fs/xfs/xfs_bmap_util.c | 4 ++-- fs/xfs/xfs_inode.c | 9 +++------ fs/xfs/xfs_inode.h | 16 +++------------- fs/xfs/xfs_trans_extfree.c | 9 ++------- 7 files changed, 14 insertions(+), 59 deletions(-)
fs/xfs/libxfs/xfs_alloc.h +0 -11
@@ -210,17 +210,6 @@ xfs_free_extent(return __xfs_free_extent (tp, bno, len, oinfo, type, false); } -static inline int -xfs_free_extent_nodiscard ( -struct xfs_trans *tp, -xfs_fsblock_t bno, -xfs_extlen_t len, -struct xfs_owner_info *oinfo, -enum xfs_ag_resv_type type) -{ -return __xfs_free_extent (tp, bno, len, oinfo, type, true); -} - int /* error */ xfs_alloc_lookup_le( struct xfs_btree_cur *cur, /* btree cursor */
fs/xfs/libxfs/xfs_bmap.c +4 -9
@@ -5107,15 +5107,10 @@ xfs_bmap_del_extent_real(if (error) goto done; }else { -if ((bflags &XFS_BMAPI_NODISCARD ) || - (del->br_state ==XFS_EXT_UNWRITTEN )) { -xfs_bmap_add_free_nodiscard (mp, dfops, - del->br_startblock, del->br_blockcount, -NULL ); - }else { -xfs_bmap_add_free (mp, dfops, del->br_startblock, - del->br_blockcount,NULL ); - } +__xfs_bmap_add_free (mp, dfops, del->br_startblock, + del->br_blockcount,NULL , + (bflags &XFS_BMAPI_NODISCARD ) || + del->br_state ==XFS_EXT_UNWRITTEN ); } }
fs/xfs/libxfs/xfs_bmap.h +0 -11
@@ -256,17 +256,6 @@ xfs_bmap_add_free(__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, false); } -static inline void -xfs_bmap_add_free_nodiscard ( -struct xfs_mount *mp, -struct xfs_defer_ops *dfops, -xfs_fsblock_t bno, -xfs_filblks_t len, -struct xfs_owner_info *oinfo) -{ -__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, true); -} -enum xfs_bmap_intent_type {XFS_BMAP_MAP =1 ,XFS_BMAP_UNMAP ,
fs/xfs/xfs_bmap_util.c +2 -2
@@ -871,8 +871,8 @@ xfs_free_eofblocks(* contents of the file are flushed to disk then the files * may be full of holes (ie NULL files bug). */ - error =xfs_itruncate_extents_nodiscard (&tp, ip,XFS_DATA_FORK , -XFS_ISIZE (ip)); + error =xfs_itruncate_extents_flags (&tp, ip,XFS_DATA_FORK , +XFS_ISIZE (ip),XFS_BMAPI_NODISCARD );if (error) {/* * If we get an error at this point we simply don't
fs/xfs/xfs_inode.c +3 -6
@@ -1534,12 +1534,12 @@ xfs_itruncate_clear_reflink_flags(* dirty on error so that transactions can be easily aborted if possible. */ int -__xfs_itruncate_extents ( +xfs_itruncate_extents_flags (struct xfs_trans **tpp,struct xfs_inode *ip,int whichfork,xfs_fsize_t new_size, -bool skip_discard) +int flags) {struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp = *tpp;
@@ -1550,7 +1550,6 @@ __xfs_itruncate_extents(xfs_filblks_t unmap_len;int error =0 ;int done =0 ; -int flags;ASSERT (xfs_isilocked (ip,XFS_ILOCK_EXCL )); ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
@@ -1563,9 +1562,7 @@ __xfs_itruncate_extents(trace_xfs_itruncate_extents_start (ip, new_size); - flags =xfs_bmapi_aflag (whichfork); -if (skip_discard) - flags |=XFS_BMAPI_NODISCARD ; + flags |=xfs_bmapi_aflag (whichfork);/* * Since it is possible for space to become allocated beyond
fs/xfs/xfs_inode.h +3 -13
@@ -415,8 +415,8 @@ uint xfs_ilock_attr_map_shared(struct xfs_inode *);uint xfs_ip2xflags (struct xfs_inode *);int xfs_ifree (struct xfs_trans *,xfs_inode_t *,struct xfs_defer_ops *); -int __xfs_itruncate_extents (struct xfs_trans **,struct xfs_inode *, -int ,xfs_fsize_t ,bool ); +int xfs_itruncate_extents_flags (struct xfs_trans **, +struct xfs_inode *,int ,xfs_fsize_t ,int );void xfs_iext_realloc (xfs_inode_t *,int ,int );void xfs_iunpin_wait (xfs_inode_t *);
@@ -440,17 +440,7 @@ xfs_itruncate_extents( int whichfork, xfs_fsize_t new_size) { - return __xfs_itruncate_extents(tpp, ip, whichfork, new_size, false); -} - -static inline int -xfs_itruncate_extents_nodiscard ( -struct xfs_trans **tpp, -struct xfs_inode *ip, -int whichfork, -xfs_fsize_t new_size) -{ -return __xfs_itruncate_extents (tpp, ip, whichfork, new_size, true); +return xfs_itruncate_extents_flags (tpp, ip, whichfork, new_size,0 ); }/* from xfs_file.c */
fs/xfs/xfs_trans_extfree.c +2 -7
@@ -80,13 +80,8 @@ xfs_trans_free_extent(trace_xfs_bmap_free_deferred (tp->t_mountp, agno,0 , agbno, ext_len); -if (skip_discard) - error =xfs_free_extent_nodiscard (tp, start_block, ext_len, - oinfo,XFS_AG_RESV_NONE ); -else - error =xfs_free_extent (tp, start_block, ext_len, oinfo, -XFS_AG_RESV_NONE ); - + error =__xfs_free_extent (tp, start_block, ext_len, + oinfo,XFS_AG_RESV_NONE , skip_discard);/* * Mark the transaction dirty, even on error. This ensures the * transaction is aborted, which:
Signature · 1 line
2.14.3
Quoted 13 lines · Brian Foster
The changes to skip discards of speculative preallocation and unwritten extents introduced several new wrapper functions through the bunmapi -> extent free codepath to reduce churn in all of the associated callers. In several cases, these wrappers simply toggle a single flag to skip or not skip discards for the resulting blocks.
The explicit
_nodiscard()wrappers for such an isolated set of callers is a bit overkill. Kill off these wrappers and replace with the calls to the underlying functions in the contexts that need to control discard behavior. Retain the wrappers that preserve the original calling conventions to serve the original purpose of reducing code churn.
Quoted 4 lines
This is a refactoring patch and does not change behavior.
Signed-off-by Brian Foster <[email protected]>Reviewed-by Christoph Hellwig <[email protected]>
Looks ok,
--D
Quoted 197 lines
---
v2:
- Clean upxfs_bmap_del_extent_real().
v1: https://marc.info/?l=linux-xfs&m=152595674924579&w=2fs/xfs/libxfs/xfs_alloc.h | 11 ----------- fs/xfs/libxfs/xfs_bmap.c | 13 ++++--------- fs/xfs/libxfs/xfs_bmap.h | 11 ----------- fs/xfs/xfs_bmap_util.c | 4 ++-- fs/xfs/xfs_inode.c | 9 +++------ fs/xfs/xfs_inode.h | 16 +++------------- fs/xfs/xfs_trans_extfree.c | 9 ++------- 7 files changed, 14 insertions(+), 59 deletions(-)fs/xfs/libxfs/xfs_alloc.h +0
-11@@ -210,17 +210,6 @@ xfs_free_extent(return __xfs_free_extent (tp, bno, len, oinfo, type, false); } -static inline int -xfs_free_extent_nodiscard ( -struct xfs_trans *tp, -xfs_fsblock_t bno, -xfs_extlen_t len, -struct xfs_owner_info *oinfo, -enum xfs_ag_resv_type type) -{ -return __xfs_free_extent (tp, bno, len, oinfo, type, true); -} - int /* error */ xfs_alloc_lookup_le( struct xfs_btree_cur *cur, /* btree cursor */fs/xfs/libxfs/xfs_bmap.c +4
-9@@ -5107,15 +5107,10 @@ xfs_bmap_del_extent_real(if (error) goto done; }else { -if ((bflags &XFS_BMAPI_NODISCARD ) || - (del->br_state ==XFS_EXT_UNWRITTEN )) { -xfs_bmap_add_free_nodiscard (mp, dfops, - del->br_startblock, del->br_blockcount, -NULL ); - }else { -xfs_bmap_add_free (mp, dfops, del->br_startblock, - del->br_blockcount,NULL ); - } +__xfs_bmap_add_free (mp, dfops, del->br_startblock, + del->br_blockcount,NULL , + (bflags &XFS_BMAPI_NODISCARD ) || + del->br_state ==XFS_EXT_UNWRITTEN ); } }fs/xfs/libxfs/xfs_bmap.h +0
-11@@ -256,17 +256,6 @@ xfs_bmap_add_free(__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, false); } -static inline void -xfs_bmap_add_free_nodiscard ( -struct xfs_mount *mp, -struct xfs_defer_ops *dfops, -xfs_fsblock_t bno, -xfs_filblks_t len, -struct xfs_owner_info *oinfo) -{ -__xfs_bmap_add_free (mp, dfops, bno, len, oinfo, true); -} -enum xfs_bmap_intent_type {XFS_BMAP_MAP =1 ,XFS_BMAP_UNMAP ,fs/xfs/xfs_bmap_util.c +2
-2@@ -871,8 +871,8 @@ xfs_free_eofblocks(* contents of the file are flushed to disk then the files * may be full of holes (ie NULL files bug). */ - error =xfs_itruncate_extents_nodiscard (&tp, ip,XFS_DATA_FORK , -XFS_ISIZE (ip)); + error =xfs_itruncate_extents_flags (&tp, ip,XFS_DATA_FORK , +XFS_ISIZE (ip),XFS_BMAPI_NODISCARD );if (error) {/* * If we get an error at this point we simply don't fs/xfs/xfs_inode.c +3
-6@@ -1534,12 +1534,12 @@ xfs_itruncate_clear_reflink_flags(* dirty on error so that transactions can be easily aborted if possible. */ int -__xfs_itruncate_extents ( +xfs_itruncate_extents_flags (struct xfs_trans **tpp,struct xfs_inode *ip,int whichfork,xfs_fsize_t new_size, -bool skip_discard) +int flags) {struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp = *tpp;@@ -1550,7 +1550,6 @@ __xfs_itruncate_extents(xfs_filblks_t unmap_len;int error =0 ;int done =0 ; -int flags;ASSERT (xfs_isilocked (ip,XFS_ILOCK_EXCL )); ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||@@ -1563,9 +1562,7 @@ __xfs_itruncate_extents(trace_xfs_itruncate_extents_start (ip, new_size); - flags =xfs_bmapi_aflag (whichfork); -if (skip_discard) - flags |=XFS_BMAPI_NODISCARD ; + flags |=xfs_bmapi_aflag (whichfork);/* * Since it is possible for space to become allocated beyond fs/xfs/xfs_inode.h +3
-13@@ -415,8 +415,8 @@ uint xfs_ilock_attr_map_shared(struct xfs_inode *);uint xfs_ip2xflags (struct xfs_inode *);int xfs_ifree (struct xfs_trans *,xfs_inode_t *,struct xfs_defer_ops *); -int __xfs_itruncate_extents (struct xfs_trans **,struct xfs_inode *, -int ,xfs_fsize_t ,bool ); +int xfs_itruncate_extents_flags (struct xfs_trans **, +struct xfs_inode *,int ,xfs_fsize_t ,int );void xfs_iext_realloc (xfs_inode_t *,int ,int );void xfs_iunpin_wait (xfs_inode_t *);@@ -440,17 +440,7 @@ xfs_itruncate_extents( int whichfork, xfs_fsize_t new_size) { - return __xfs_itruncate_extents(tpp, ip, whichfork, new_size, false); -} - -static inline int -xfs_itruncate_extents_nodiscard ( -struct xfs_trans **tpp, -struct xfs_inode *ip, -int whichfork, -xfs_fsize_t new_size) -{ -return __xfs_itruncate_extents (tpp, ip, whichfork, new_size, true); +return xfs_itruncate_extents_flags (tpp, ip, whichfork, new_size,0 ); }/* from xfs_file.c */ fs/xfs/xfs_trans_extfree.c +2
-7@@ -80,13 +80,8 @@ xfs_trans_free_extent(trace_xfs_bmap_free_deferred (tp->t_mountp, agno,0 , agbno, ext_len); -if (skip_discard) - error =xfs_free_extent_nodiscard (tp, start_block, ext_len, - oinfo,XFS_AG_RESV_NONE ); -else - error =xfs_free_extent (tp, start_block, ext_len, oinfo, -XFS_AG_RESV_NONE ); - + error =__xfs_free_extent (tp, start_block, ext_len, + oinfo,XFS_AG_RESV_NONE , skip_discard);/* * Mark the transaction dirty, even on error. This ensures the * transaction is aborted, which: Signature · 6 lines
2.14.3 -- 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