Re: [PATCH] xfs: don't assume perags are initialised when trimming AGs

[Bill O'Donnell <bodonnel@redhat.com>]

On Thu, May 01, 2025 at 09:27:24AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When running fstrim immediately after mounting a V4 filesystem,
> the fstrim fails to trim all the free space in the filesystem. It
> only trims the first extent in the by-size free space tree in each
> AG and then returns. If a second fstrim is then run, it runs
> correctly and the entire free space in the filesystem is iterated
> and discarded correctly.
> 
> The problem lies in the setup of the trim cursor - it assumes that
> pag->pagf_longest is valid without either reading the AGF first or
> checking if xfs_perag_initialised_agf(pag) is true or not.
> 
> As a result, when a filesystem is mounted without reading the AGF
> (e.g. a clean mount on a v4 filesystem) and the first operation is a
> fstrim call, pag->pagf_longest is zero and so the free extent search
> starts at the wrong end of the by-size btree and exits after
> discarding the first record in the tree.
> 
> Fix this by deferring the initialisation of tcur->count to after
> we have locked the AGF and guaranteed that the perag is properly
> initialised. We trigger this on tcur->count == 0 after locking the
> AGF, as this will only occur on the first call to
> xfs_trim_gather_extents() for each AG. If we need to iterate,
> tcur->count will be set to the length of the record we need to
> restart at, so we can use this to ensure we only sample a valid
> pag->pagf_longest value for the iteration.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
Needs a "fixes" note in the description. Otherwise, lgtm.
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>


>  fs/xfs/xfs_discard.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index c1a306268ae4..94d0873bcd62 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -167,6 +167,14 @@ xfs_discard_extents(
>  	return error;
>  }
>  
> +/*
> + * Care must be taken setting up the trim cursor as the perags may not have been
> + * initialised when the cursor is initialised. e.g. a clean mount which hasn't
> + * read in AGFs and the first operation run on the mounted fs is a trim. This
> + * can result in perag fields that aren't initialised until
> + * xfs_trim_gather_extents() calls xfs_alloc_read_agf() to lock down the AG for
> + * the free space search.
> + */
>  struct xfs_trim_cur {
>  	xfs_agblock_t	start;
>  	xfs_extlen_t	count;
> @@ -204,6 +212,14 @@ xfs_trim_gather_extents(
>  	if (error)
>  		goto out_trans_cancel;
>  
> +	/*
> +	 * First time through tcur->count will not have been initialised as
> +	 * pag->pagf_longest is not guaranteed to be valid before we read
> +	 * the AGF buffer above.
> +	 */
> +	if (!tcur->count)
> +		tcur->count = pag->pagf_longest;
> +
>  	if (tcur->by_bno) {
>  		/* sub-AG discard request always starts at tcur->start */
>  		cur = xfs_bnobt_init_cursor(mp, tp, agbp, pag);
> @@ -350,7 +366,6 @@ xfs_trim_perag_extents(
>  {
>  	struct xfs_trim_cur	tcur = {
>  		.start		= start,
> -		.count		= pag->pagf_longest,
>  		.end		= end,
>  		.minlen		= minlen,
>  	};
> -- 
> 2.45.2
> 
>