On Wed, Mar 27, 2024 at 12:03:08PM +0100, Christoph Hellwig wrote:
> Currently xfs_bmap_del_extent_real frees RT extents before updating
> the bmap btree, while it frees regular blocks after performing the bmap
> btree update. While this behavior goes back to the original commit,
> I can't find any good reason for handling RT extent vs regular block
> freeing differently. We use the same transaction, and unless rmaps
> or reflink are enabled (which currently aren't support for RT inodes)
> there are no transactions rolls or deferred ops that can rely on this
> ordering.
...and the realtime rmap/reflink patchsets will want to reuse the data
device's ordering (bmap -> rmap -> refcount -> efi) for the rt volume.
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
I'm ok with moving this now since I'm mostly going to pave over it later
anyway :)
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_bmap.c | 26 +++++++++-----------------
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 09d4b730ee9709..282b44deb9f864 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5107,8 +5107,7 @@ xfs_bmap_del_extent_real(
> {
> xfs_fsblock_t del_endblock=0; /* first block past del */
> xfs_fileoff_t del_endoff; /* first offset past del */
> - int do_fx; /* free extent at end of routine */
> - int error; /* error return value */
> + int error = 0; /* error return value */
> struct xfs_bmbt_irec got; /* current extent entry */
> xfs_fileoff_t got_endoff; /* first offset past got */
> int i; /* temp state */
> @@ -5151,20 +5150,10 @@ xfs_bmap_del_extent_real(
> return -ENOSPC;
>
> *logflagsp = XFS_ILOG_CORE;
> - if (xfs_ifork_is_realtime(ip, whichfork)) {
> - if (!(bflags & XFS_BMAPI_REMAP)) {
> - error = xfs_rtfree_blocks(tp, del->br_startblock,
> - del->br_blockcount);
> - if (error)
> - return error;
> - }
> -
> - do_fx = 0;
> + if (xfs_ifork_is_realtime(ip, whichfork))
> qfield = XFS_TRANS_DQ_RTBCOUNT;
> - } else {
> - do_fx = 1;
> + else
> qfield = XFS_TRANS_DQ_BCOUNT;
> - }
> nblks = del->br_blockcount;
>
> del_endblock = del->br_startblock + del->br_blockcount;
> @@ -5312,18 +5301,21 @@ xfs_bmap_del_extent_real(
> /*
> * If we need to, add to list of extents to delete.
> */
> - if (do_fx && !(bflags & XFS_BMAPI_REMAP)) {
> + if (!(bflags & XFS_BMAPI_REMAP)) {
> if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
> xfs_refcount_decrease_extent(tp, del);
> + } else if (xfs_ifork_is_realtime(ip, whichfork)) {
> + error = xfs_rtfree_blocks(tp, del->br_startblock,
> + del->br_blockcount);
> } else {
> error = xfs_free_extent_later(tp, del->br_startblock,
> del->br_blockcount, NULL,
> XFS_AG_RESV_NONE,
> ((bflags & XFS_BMAPI_NODISCARD) ||
> del->br_state == XFS_EXT_UNWRITTEN));
> - if (error)
> - return error;
> }
> + if (error)
> + return error;
> }
>
> /*
> --
> 2.39.2
>
>
