* iext handling fixes and cleanup
@ 2024-04-30 12:55 Christoph Hellwig
2024-04-30 12:56 ` [PATCH 1/3] xfs: upgrade the extent counters in xfs_reflink_end_cow_extent later Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2024-04-30 12:55 UTC (permalink / raw)
To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs
Hi all,
this series fixes two unlikely to hit iext handling bugs in
xfs_reflink_end_cow and then cleanups up the iext count updgrade
handling.
It has been split from a larger series that needs more work.
Diffstat:
libxfs/xfs_attr.c | 5 ----
libxfs/xfs_bmap.c | 5 ----
libxfs/xfs_inode_fork.c | 57 +++++++++++++++++++++---------------------------
libxfs/xfs_inode_fork.h | 6 +----
xfs_bmap_item.c | 4 ---
xfs_bmap_util.c | 24 ++++----------------
xfs_dquot.c | 5 ----
xfs_iomap.c | 9 +------
xfs_reflink.c | 23 +++++--------------
xfs_rtalloc.c | 5 ----
10 files changed, 45 insertions(+), 98 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] xfs: upgrade the extent counters in xfs_reflink_end_cow_extent later 2024-04-30 12:55 iext handling fixes and cleanup Christoph Hellwig @ 2024-04-30 12:56 ` Christoph Hellwig 2024-04-30 12:56 ` [PATCH 2/3] xfs: remove a racy if_bytes check in xfs_reflink_end_cow_extent Christoph Hellwig 2024-04-30 12:56 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig 2 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2024-04-30 12:56 UTC (permalink / raw) To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs Defer the extent counter size upgrade until we know we're going to modify the extent mapping. This also defers dirtying the transaction and will allow us safely back out later in the function in later changes. Fixes: 4f86bb4b66c9 ("xfs: Conditionally upgrade existing inodes to use large extent counters") Reviewed-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_reflink.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 7da0e8f961d351..9ce37d366534c3 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -751,14 +751,6 @@ xfs_reflink_end_cow_extent( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, - XFS_IEXT_REFLINK_END_COW_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_REFLINK_END_COW_CNT); - if (error) - goto out_cancel; - /* * In case of racing, overlapping AIO writes no COW extents might be * left by the time I/O completes for the loser of the race. In that @@ -787,6 +779,14 @@ xfs_reflink_end_cow_extent( del = got; xfs_trim_extent(&del, *offset_fsb, end_fsb - *offset_fsb); + error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + XFS_IEXT_REFLINK_END_COW_CNT); + if (error == -EFBIG) + error = xfs_iext_count_upgrade(tp, ip, + XFS_IEXT_REFLINK_END_COW_CNT); + if (error) + goto out_cancel; + /* Grab the corresponding mapping in the data fork. */ nmaps = 1; error = xfs_bmapi_read(ip, del.br_startoff, del.br_blockcount, &data, -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] xfs: remove a racy if_bytes check in xfs_reflink_end_cow_extent 2024-04-30 12:55 iext handling fixes and cleanup Christoph Hellwig 2024-04-30 12:56 ` [PATCH 1/3] xfs: upgrade the extent counters in xfs_reflink_end_cow_extent later Christoph Hellwig @ 2024-04-30 12:56 ` Christoph Hellwig 2024-04-30 12:56 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig 2 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2024-04-30 12:56 UTC (permalink / raw) To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs Accessing if_bytes without the ilock is racy. Remove the initial if_bytes == 0 check in xfs_reflink_end_cow_extent and let ext_iext_lookup_extent fail for this case after we've taken the ilock. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/xfs_reflink.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 9ce37d366534c3..0ab2ef5b58f6c4 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -731,12 +731,6 @@ xfs_reflink_end_cow_extent( int nmaps; int error; - /* No COW extents? That's easy! */ - if (ifp->if_bytes == 0) { - *offset_fsb = end_fsb; - return 0; - } - resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK); error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, XFS_TRANS_RESERVE, &tp); -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] xfs: simplify iext overflow checking and upgrade 2024-04-30 12:55 iext handling fixes and cleanup Christoph Hellwig 2024-04-30 12:56 ` [PATCH 1/3] xfs: upgrade the extent counters in xfs_reflink_end_cow_extent later Christoph Hellwig 2024-04-30 12:56 ` [PATCH 2/3] xfs: remove a racy if_bytes check in xfs_reflink_end_cow_extent Christoph Hellwig @ 2024-04-30 12:56 ` Christoph Hellwig 2024-04-30 21:43 ` Dave Chinner 2 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2024-04-30 12:56 UTC (permalink / raw) To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs Currently the calls to xfs_iext_count_may_overflow and xfs_iext_count_upgrade are always paired. Merge them into a single function to simplify the callers and the actual check and upgrade logic itself. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_attr.c | 5 +-- fs/xfs/libxfs/xfs_bmap.c | 5 +-- fs/xfs/libxfs/xfs_inode_fork.c | 57 +++++++++++++++------------------- fs/xfs/libxfs/xfs_inode_fork.h | 6 ++-- fs/xfs/xfs_bmap_item.c | 4 +-- fs/xfs/xfs_bmap_util.c | 24 +++----------- fs/xfs/xfs_dquot.c | 5 +-- fs/xfs/xfs_iomap.c | 9 ++---- fs/xfs/xfs_reflink.c | 9 ++---- fs/xfs/xfs_rtalloc.c | 5 +-- 10 files changed, 41 insertions(+), 88 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 1c2a27fce08a9d..ded92ccefe9f6d 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1050,11 +1050,8 @@ xfs_attr_set( return error; if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) { - error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, + error = xfs_iext_count_ensure(args->trans, dp, XFS_ATTR_FORK, XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(args->trans, dp, - XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); if (error) goto out_trans_cancel; } diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 6053f5e5c71eec..3debd0d561b812 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -4621,11 +4621,8 @@ xfs_bmapi_convert_delalloc( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); - error = xfs_iext_count_may_overflow(ip, whichfork, + error = xfs_iext_count_ensure(tp, ip, whichfork, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto out_trans_cancel; diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 7d660a9739090a..82e670dd1212c4 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -765,53 +765,46 @@ xfs_ifork_verify_local_attr( return 0; } +/* + * Check if the inode fork supports adding nr_to_add more extents. + * + * If it doesn't but we can upgrade it to large extent counters, do the upgrade. + * If we can't upgrade or are already using big counters but still can't fit the + * additional extents, return -EFBIG. + */ int -xfs_iext_count_may_overflow( +xfs_iext_count_ensure( + struct xfs_trans *tp, struct xfs_inode *ip, int whichfork, - int nr_to_add) + uint nr_to_add) { + struct xfs_mount *mp = ip->i_mount; + bool has_large = + xfs_inode_has_large_extent_counts(ip); struct xfs_ifork *ifp = xfs_ifork_ptr(ip, whichfork); - uint64_t max_exts; uint64_t nr_exts; + ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR); + if (whichfork == XFS_COW_FORK) return 0; - max_exts = xfs_iext_max_nextents(xfs_inode_has_large_extent_counts(ip), - whichfork); - - if (XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS)) - max_exts = 10; - + /* no point in upgrading if if_nextents overflows */ nr_exts = ifp->if_nextents + nr_to_add; - if (nr_exts < ifp->if_nextents || nr_exts > max_exts) + if (nr_exts < ifp->if_nextents) return -EFBIG; - return 0; -} - -/* - * Upgrade this inode's extent counter fields to be able to handle a potential - * increase in the extent count by nr_to_add. Normally this is the same - * quantity that caused xfs_iext_count_may_overflow() to return -EFBIG. - */ -int -xfs_iext_count_upgrade( - struct xfs_trans *tp, - struct xfs_inode *ip, - uint nr_to_add) -{ - ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR); - - if (!xfs_has_large_extent_counts(ip->i_mount) || - xfs_inode_has_large_extent_counts(ip) || - XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS)) + if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REDUCE_MAX_IEXTENTS) && + nr_exts > 10) return -EFBIG; - ip->i_diflags2 |= XFS_DIFLAG2_NREXT64; - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - + if (nr_exts > xfs_iext_max_nextents(has_large, whichfork)) { + if (has_large || !xfs_has_large_extent_counts(mp)) + return -EFBIG; + ip->i_diflags2 |= XFS_DIFLAG2_NREXT64; + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + } return 0; } diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h index bd53eb951b6515..9e1456f5cc2c85 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.h +++ b/fs/xfs/libxfs/xfs_inode_fork.h @@ -256,10 +256,8 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); int xfs_ifork_verify_local_data(struct xfs_inode *ip); int xfs_ifork_verify_local_attr(struct xfs_inode *ip); -int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, - int nr_to_add); -int xfs_iext_count_upgrade(struct xfs_trans *tp, struct xfs_inode *ip, - uint nr_to_add); +int xfs_iext_count_ensure(struct xfs_trans *tp, struct xfs_inode *ip, + int whichfork, uint nr_to_add); bool xfs_ifork_is_realtime(struct xfs_inode *ip, int whichfork); /* returns true if the fork has extents but they are not read in yet. */ diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c index d27859a684aa69..38067d02ee3ca7 100644 --- a/fs/xfs/xfs_bmap_item.c +++ b/fs/xfs/xfs_bmap_item.c @@ -524,9 +524,7 @@ xfs_bmap_recover_work( else iext_delta = XFS_IEXT_PUNCH_HOLE_CNT; - error = xfs_iext_count_may_overflow(ip, work->bi_whichfork, iext_delta); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, iext_delta); + error = xfs_iext_count_ensure(tp, ip, work->bi_whichfork, iext_delta); if (error) goto err_cancel; diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 53aa90a0ee3a85..11062bddba0e5e 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -713,11 +713,8 @@ xfs_alloc_file_space( if (error) break; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto error; @@ -775,10 +772,8 @@ xfs_unmap_extent( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_PUNCH_HOLE_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, XFS_IEXT_PUNCH_HOLE_CNT); if (error) goto out_trans_cancel; @@ -1054,10 +1049,8 @@ xfs_insert_file_space( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_PUNCH_HOLE_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, XFS_IEXT_PUNCH_HOLE_CNT); if (error) goto out_trans_cancel; @@ -1283,23 +1276,16 @@ xfs_swap_extent_rmap( trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec); if (xfs_bmap_is_real_extent(&uirec)) { - error = xfs_iext_count_may_overflow(ip, - XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_SWAP_RMAP_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_SWAP_RMAP_CNT); if (error) goto out; } if (xfs_bmap_is_real_extent(&irec)) { - error = xfs_iext_count_may_overflow(tip, + error = xfs_iext_count_ensure(tp, tip, XFS_DATA_FORK, XFS_IEXT_SWAP_RMAP_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_SWAP_RMAP_CNT); if (error) goto out; } diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 13aba84bd64afb..c2e66d392399dd 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -341,11 +341,8 @@ xfs_dquot_disk_alloc( goto err_cancel; } - error = xfs_iext_count_may_overflow(quotip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, quotip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, quotip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto err_cancel; diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index 9ce0f6b9df93e6..9dfb8e1a5a55cb 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -299,9 +299,7 @@ xfs_iomap_write_direct( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, nr_exts); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, nr_exts); + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, nr_exts); if (error) goto out_trans_cancel; @@ -625,11 +623,8 @@ xfs_iomap_write_unwritten( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_WRITE_UNWRITTEN_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_WRITE_UNWRITTEN_CNT); if (error) goto error_on_bmapi_transaction; diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 0ab2ef5b58f6c4..323d5494149698 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -773,11 +773,8 @@ xfs_reflink_end_cow_extent( del = got; xfs_trim_extent(&del, *offset_fsb, end_fsb - *offset_fsb); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_REFLINK_END_COW_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_REFLINK_END_COW_CNT); if (error) goto out_cancel; @@ -1277,9 +1274,7 @@ xfs_reflink_remap_extent( if (dmap_written) ++iext_delta; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, iext_delta); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, iext_delta); + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, iext_delta); if (error) goto out_cancel; diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c index b476a876478d93..37edf4c5ce73ad 100644 --- a/fs/xfs/xfs_rtalloc.c +++ b/fs/xfs/xfs_rtalloc.c @@ -695,11 +695,8 @@ xfs_growfs_rt_alloc( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_ensure(tp, ip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto out_trans_cancel; -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] xfs: simplify iext overflow checking and upgrade 2024-04-30 12:56 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig @ 2024-04-30 21:43 ` Dave Chinner 2024-05-01 4:35 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Dave Chinner @ 2024-04-30 21:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Chandan Babu R, Darrick J. Wong, linux-xfs On Tue, Apr 30, 2024 at 02:56:02PM +0200, Christoph Hellwig wrote: > Currently the calls to xfs_iext_count_may_overflow and > xfs_iext_count_upgrade are always paired. Merge them into a single > function to simplify the callers and the actual check and upgrade > logic itself. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_attr.c | 5 +-- > fs/xfs/libxfs/xfs_bmap.c | 5 +-- > fs/xfs/libxfs/xfs_inode_fork.c | 57 +++++++++++++++------------------- > fs/xfs/libxfs/xfs_inode_fork.h | 6 ++-- > fs/xfs/xfs_bmap_item.c | 4 +-- > fs/xfs/xfs_bmap_util.c | 24 +++----------- > fs/xfs/xfs_dquot.c | 5 +-- > fs/xfs/xfs_iomap.c | 9 ++---- > fs/xfs/xfs_reflink.c | 9 ++---- > fs/xfs/xfs_rtalloc.c | 5 +-- > 10 files changed, 41 insertions(+), 88 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 1c2a27fce08a9d..ded92ccefe9f6d 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1050,11 +1050,8 @@ xfs_attr_set( > return error; > > if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) { > - error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, > + error = xfs_iext_count_ensure(args->trans, dp, XFS_ATTR_FORK, > XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); > - if (error == -EFBIG) > - error = xfs_iext_count_upgrade(args->trans, dp, > - XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); > if (error) > goto out_trans_cancel; > } > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 6053f5e5c71eec..3debd0d561b812 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -4621,11 +4621,8 @@ xfs_bmapi_convert_delalloc( > xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_trans_ijoin(tp, ip, 0); > > - error = xfs_iext_count_may_overflow(ip, whichfork, > + error = xfs_iext_count_ensure(tp, ip, whichfork, > XFS_IEXT_ADD_NOSPLIT_CNT); > - if (error == -EFBIG) > - error = xfs_iext_count_upgrade(tp, ip, > - XFS_IEXT_ADD_NOSPLIT_CNT); > if (error) > goto out_trans_cancel; > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > index 7d660a9739090a..82e670dd1212c4 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.c > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -765,53 +765,46 @@ xfs_ifork_verify_local_attr( > return 0; > } > > +/* > + * Check if the inode fork supports adding nr_to_add more extents. > + * > + * If it doesn't but we can upgrade it to large extent counters, do the upgrade. > + * If we can't upgrade or are already using big counters but still can't fit the > + * additional extents, return -EFBIG. > + */ > int > -xfs_iext_count_may_overflow( > +xfs_iext_count_ensure( Everything looks fine, but the name isn't very good. What, exactly, is this function ensuring about the iext count? The function is extending the iext count if needed, so to me the function name should reflect what it actually does. xfs_iext_count_extend() seems like a much better name - it tells the reader what the code is actually doing (i.e. we may have to extend the iext count before performing this operation) and it makes it obvious when it is done out of place.... -Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] xfs: simplify iext overflow checking and upgrade 2024-04-30 21:43 ` Dave Chinner @ 2024-05-01 4:35 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2024-05-01 4:35 UTC (permalink / raw) To: Dave Chinner Cc: Christoph Hellwig, Chandan Babu R, Darrick J. Wong, linux-xfs On Wed, May 01, 2024 at 07:43:18AM +1000, Dave Chinner wrote: > xfs_iext_count_extend() seems like a much better name - it tells the > reader what the code is actually doing (i.e. we may have to extend > the iext count before performing this operation) and it makes it > obvious when it is done out of place.... Sure, I can change the name. Btw, it would be nice to trim your reply a bit more, I had to scroll down a few pages of quotes text yo get here. ^ permalink raw reply [flat|nested] 8+ messages in thread
* iext handling fixes and cleanup v2 @ 2024-05-02 7:33 Christoph Hellwig 2024-05-02 7:33 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2024-05-02 7:33 UTC (permalink / raw) To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs Hi all, this series fixes two unlikely to hit iext handling bugs in xfs_reflink_end_cow and then cleanups up the iext count updgrade handling. It has been split from a larger series that needs more work. Changes since v1: - rename xfs_iext_count_ensure to xfs_iext_count_extend Diffstat: libxfs/xfs_attr.c | 5 ---- libxfs/xfs_bmap.c | 5 ---- libxfs/xfs_inode_fork.c | 57 +++++++++++++++++++++--------------------------- libxfs/xfs_inode_fork.h | 6 +---- xfs_bmap_item.c | 4 --- xfs_bmap_util.c | 23 ++++--------------- xfs_dquot.c | 5 ---- xfs_iomap.c | 9 +------ xfs_reflink.c | 23 +++++-------------- xfs_rtalloc.c | 5 ---- 10 files changed, 45 insertions(+), 97 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] xfs: simplify iext overflow checking and upgrade 2024-05-02 7:33 iext handling fixes and cleanup v2 Christoph Hellwig @ 2024-05-02 7:33 ` Christoph Hellwig 2024-05-02 7:55 ` Dave Chinner 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2024-05-02 7:33 UTC (permalink / raw) To: Chandan Babu R, Darrick J. Wong; +Cc: linux-xfs Currently the calls to xfs_iext_count_may_overflow and xfs_iext_count_upgrade are always paired. Merge them into a single function to simplify the callers and the actual check and upgrade logic itself. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_attr.c | 5 +-- fs/xfs/libxfs/xfs_bmap.c | 5 +-- fs/xfs/libxfs/xfs_inode_fork.c | 57 +++++++++++++++------------------- fs/xfs/libxfs/xfs_inode_fork.h | 6 ++-- fs/xfs/xfs_bmap_item.c | 4 +-- fs/xfs/xfs_bmap_util.c | 23 +++----------- fs/xfs/xfs_dquot.c | 5 +-- fs/xfs/xfs_iomap.c | 9 ++---- fs/xfs/xfs_reflink.c | 9 ++---- fs/xfs/xfs_rtalloc.c | 5 +-- 10 files changed, 41 insertions(+), 87 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 1c2a27fce08a9d..c8d1c3ac19dcad 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1050,11 +1050,8 @@ xfs_attr_set( return error; if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) { - error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, + error = xfs_iext_count_extend(args->trans, dp, XFS_ATTR_FORK, XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(args->trans, dp, - XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)); if (error) goto out_trans_cancel; } diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index be515bac8ea1e0..1ea444bef3b1c3 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -4651,11 +4651,8 @@ xfs_bmapi_convert_one_delalloc( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); - error = xfs_iext_count_may_overflow(ip, whichfork, + error = xfs_iext_count_extend(tp, ip, whichfork, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto out_trans_cancel; diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 7d660a9739090a..9d11ae01590919 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -765,53 +765,46 @@ xfs_ifork_verify_local_attr( return 0; } +/* + * Check if the inode fork supports adding nr_to_add more extents. + * + * If it doesn't but we can upgrade it to large extent counters, do the upgrade. + * If we can't upgrade or are already using big counters but still can't fit the + * additional extents, return -EFBIG. + */ int -xfs_iext_count_may_overflow( +xfs_iext_count_extend( + struct xfs_trans *tp, struct xfs_inode *ip, int whichfork, - int nr_to_add) + uint nr_to_add) { + struct xfs_mount *mp = ip->i_mount; + bool has_large = + xfs_inode_has_large_extent_counts(ip); struct xfs_ifork *ifp = xfs_ifork_ptr(ip, whichfork); - uint64_t max_exts; uint64_t nr_exts; + ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR); + if (whichfork == XFS_COW_FORK) return 0; - max_exts = xfs_iext_max_nextents(xfs_inode_has_large_extent_counts(ip), - whichfork); - - if (XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS)) - max_exts = 10; - + /* no point in upgrading if if_nextents overflows */ nr_exts = ifp->if_nextents + nr_to_add; - if (nr_exts < ifp->if_nextents || nr_exts > max_exts) + if (nr_exts < ifp->if_nextents) return -EFBIG; - return 0; -} - -/* - * Upgrade this inode's extent counter fields to be able to handle a potential - * increase in the extent count by nr_to_add. Normally this is the same - * quantity that caused xfs_iext_count_may_overflow() to return -EFBIG. - */ -int -xfs_iext_count_upgrade( - struct xfs_trans *tp, - struct xfs_inode *ip, - uint nr_to_add) -{ - ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR); - - if (!xfs_has_large_extent_counts(ip->i_mount) || - xfs_inode_has_large_extent_counts(ip) || - XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS)) + if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REDUCE_MAX_IEXTENTS) && + nr_exts > 10) return -EFBIG; - ip->i_diflags2 |= XFS_DIFLAG2_NREXT64; - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - + if (nr_exts > xfs_iext_max_nextents(has_large, whichfork)) { + if (has_large || !xfs_has_large_extent_counts(mp)) + return -EFBIG; + ip->i_diflags2 |= XFS_DIFLAG2_NREXT64; + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + } return 0; } diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h index bd53eb951b6515..2373d12fd474f0 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.h +++ b/fs/xfs/libxfs/xfs_inode_fork.h @@ -256,10 +256,8 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); int xfs_ifork_verify_local_data(struct xfs_inode *ip); int xfs_ifork_verify_local_attr(struct xfs_inode *ip); -int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, - int nr_to_add); -int xfs_iext_count_upgrade(struct xfs_trans *tp, struct xfs_inode *ip, - uint nr_to_add); +int xfs_iext_count_extend(struct xfs_trans *tp, struct xfs_inode *ip, + int whichfork, uint nr_to_add); bool xfs_ifork_is_realtime(struct xfs_inode *ip, int whichfork); /* returns true if the fork has extents but they are not read in yet. */ diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c index d27859a684aa69..a19d62e78aa1e6 100644 --- a/fs/xfs/xfs_bmap_item.c +++ b/fs/xfs/xfs_bmap_item.c @@ -524,9 +524,7 @@ xfs_bmap_recover_work( else iext_delta = XFS_IEXT_PUNCH_HOLE_CNT; - error = xfs_iext_count_may_overflow(ip, work->bi_whichfork, iext_delta); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, iext_delta); + error = xfs_iext_count_extend(tp, ip, work->bi_whichfork, iext_delta); if (error) goto err_cancel; diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 2e6f08198c0719..9cafb3cb5817ae 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -713,11 +713,8 @@ xfs_alloc_file_space( if (error) break; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto error; @@ -774,10 +771,8 @@ xfs_unmap_extent( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_PUNCH_HOLE_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, XFS_IEXT_PUNCH_HOLE_CNT); if (error) goto out_trans_cancel; @@ -1053,10 +1048,8 @@ xfs_insert_file_space( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_PUNCH_HOLE_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, XFS_IEXT_PUNCH_HOLE_CNT); if (error) goto out_trans_cancel; @@ -1282,23 +1275,17 @@ xfs_swap_extent_rmap( trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec); if (xfs_bmap_is_real_extent(&uirec)) { - error = xfs_iext_count_may_overflow(ip, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_SWAP_RMAP_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_SWAP_RMAP_CNT); if (error) goto out; } if (xfs_bmap_is_real_extent(&irec)) { - error = xfs_iext_count_may_overflow(tip, + error = xfs_iext_count_extend(tp, tip, XFS_DATA_FORK, XFS_IEXT_SWAP_RMAP_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_SWAP_RMAP_CNT); if (error) goto out; } diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 43acb4f0d17433..c1b211c260a9d5 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -341,11 +341,8 @@ xfs_dquot_disk_alloc( goto err_cancel; } - error = xfs_iext_count_may_overflow(quotip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, quotip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, quotip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto err_cancel; diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index f5d0ed45721b5c..1bcc2027806810 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -299,9 +299,7 @@ xfs_iomap_write_direct( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, nr_exts); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, nr_exts); + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, nr_exts); if (error) goto out_trans_cancel; @@ -617,11 +615,8 @@ xfs_iomap_write_unwritten( if (error) return error; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_WRITE_UNWRITTEN_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_WRITE_UNWRITTEN_CNT); if (error) goto error_on_bmapi_transaction; diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 3bb5318f699ddc..2db72a1ac288dd 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -759,11 +759,8 @@ xfs_reflink_end_cow_extent( del = got; xfs_trim_extent(&del, *offset_fsb, end_fsb - *offset_fsb); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_REFLINK_END_COW_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_REFLINK_END_COW_CNT); if (error) goto out_cancel; @@ -1263,9 +1260,7 @@ xfs_reflink_remap_extent( if (dmap_written) ++iext_delta; - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, iext_delta); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, iext_delta); + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, iext_delta); if (error) goto out_cancel; diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c index 150f544445ca82..5a7ddfed1bb855 100644 --- a/fs/xfs/xfs_rtalloc.c +++ b/fs/xfs/xfs_rtalloc.c @@ -695,11 +695,8 @@ xfs_growfs_rt_alloc( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); - error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, + error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, XFS_IEXT_ADD_NOSPLIT_CNT); - if (error == -EFBIG) - error = xfs_iext_count_upgrade(tp, ip, - XFS_IEXT_ADD_NOSPLIT_CNT); if (error) goto out_trans_cancel; -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] xfs: simplify iext overflow checking and upgrade 2024-05-02 7:33 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig @ 2024-05-02 7:55 ` Dave Chinner 0 siblings, 0 replies; 8+ messages in thread From: Dave Chinner @ 2024-05-02 7:55 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Chandan Babu R, Darrick J. Wong, linux-xfs On Thu, May 02, 2024 at 09:33:55AM +0200, Christoph Hellwig wrote: > Currently the calls to xfs_iext_count_may_overflow and > xfs_iext_count_upgrade are always paired. Merge them into a single > function to simplify the callers and the actual check and upgrade > logic itself. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Looks good now. Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-05-02 7:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-30 12:55 iext handling fixes and cleanup Christoph Hellwig 2024-04-30 12:56 ` [PATCH 1/3] xfs: upgrade the extent counters in xfs_reflink_end_cow_extent later Christoph Hellwig 2024-04-30 12:56 ` [PATCH 2/3] xfs: remove a racy if_bytes check in xfs_reflink_end_cow_extent Christoph Hellwig 2024-04-30 12:56 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig 2024-04-30 21:43 ` Dave Chinner 2024-05-01 4:35 ` Christoph Hellwig -- strict thread matches above, loose matches on Subject: below -- 2024-05-02 7:33 iext handling fixes and cleanup v2 Christoph Hellwig 2024-05-02 7:33 ` [PATCH 3/3] xfs: simplify iext overflow checking and upgrade Christoph Hellwig 2024-05-02 7:55 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox