Date: Wed, 29 May 2024 10:28:37 +1000
From: Dave Chinner <david@...morbit.com>
To: Gao Xiang <hsiangkao@...ux.alibaba.com>
Cc: linux-xfs@...r.kernel.org, "Darrick J . Wong" <djwong@...nel.org>,
	Chandan Babu R <chandanbabu@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] xfs: avoid redundant AGFL buffer invalidation

On Tue, May 28, 2024 at 12:12:39PM +0800, Gao Xiang wrote:
> Currently AGFL blocks can be filled from the following three sources:
>  - allocbt free blocks, as in xfs_allocbt_free_block();
>  - rmapbt free blocks, as in xfs_rmapbt_free_block();
>  - refilled from freespace btrees, as in xfs_alloc_fix_freelist().
> 
> Originally, allocbt free blocks would be marked as stale only when they
> put back in the general free space pool as Dave mentioned on IRC, "we
> don't stale AGF metadata btree blocks when they are returned to the
> AGFL .. but once they get put back in the general free space pool, we
> have to make sure the buffers are marked stale as the next user of
> those blocks might be user data...."
> 
> However, after commit ca250b1b3d71 ("xfs: invalidate allocbt blocks
> moved to the free list") and commit edfd9dd54921 ("xfs: move buffer
> invalidation to xfs_btree_free_block"), even allocbt / bmapbt free
> blocks will be invalidated immediately since they may fail to pass
> V5 format validation on writeback even writeback to free space would be
> safe.
> 
> IOWs, IMHO currently there is actually no difference of free blocks
> between AGFL freespace pool and the general free space pool.  So let's
> avoid extra redundant AGFL buffer invalidation, since otherwise we're
> currently facing unnecessary xfs_log_force() due to xfs_trans_binval()
> again on buffers already marked as stale before as below:
> 
> [  333.507469] Call Trace:
> [  333.507862]  xfs_buf_find+0x371/0x6a0       <- xfs_buf_lock
> [  333.508451]  xfs_buf_get_map+0x3f/0x230
> [  333.509062]  xfs_trans_get_buf_map+0x11a/0x280
> [  333.509751]  xfs_free_agfl_block+0xa1/0xd0
> [  333.510403]  xfs_agfl_free_finish_item+0x16e/0x1d0
> [  333.511157]  xfs_defer_finish_noroll+0x1ef/0x5c0
> [  333.511871]  xfs_defer_finish+0xc/0xa0
> [  333.512471]  xfs_itruncate_extents_flags+0x18a/0x5e0
> [  333.513253]  xfs_inactive_truncate+0xb8/0x130
> [  333.513930]  xfs_inactive+0x223/0x270
> 
> xfs_log_force() will take tens of milliseconds with AGF buffer locked.
> It becomes an unnecessary long latency especially on our PMEM devices
> with FSDAX enabled and fsops like xfs_reflink_find_shared() at the same
> time are stuck due to the same AGF lock.  Removing the double
> invalidation on the AGFL blocks does not make this issue go away, but
> this patch fixes for our workloads in reality and it should also work
> by the code analysis.
> 
> Note that I'm not sure I need to remove another redundant one in
> xfs_alloc_ag_vextent_small() since it's unrelated to our workloads.
> Also fstests are passed with this patch.
> 
> Signed-off-by: Gao Xiang <hsiangkao@...ux.alibaba.com>
> ---
> changes since v1:
>  - Get rid of xfs_free_agfl_block() suggested by Dave;
>  - Some commit message refinement.
> 
>  fs/xfs/libxfs/xfs_alloc.c | 28 +---------------------------
>  fs/xfs/libxfs/xfs_alloc.h |  6 ++++--
>  fs/xfs/xfs_extfree_item.c |  4 ++--
>  3 files changed, 7 insertions(+), 31 deletions(-)

Looks fine - it appears to me that all the places that put blocks
onto the freelist will have already invalidated any buffers over
those blocks, so we don't need to do it again when moving them from
the freelist to the freespace btrees.

Reviewed-by: Dave Chinner <dchinner@...hat.com>

-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists