Re: [PATCH v3 1/2] xfs: set a mount flag when perag reservation is active

[Brian Foster <bfoster@redhat.com>]

On Tue, Apr 13, 2021 at 05:49:50PM -0700, Darrick J. Wong wrote:
> On Mon, Apr 12, 2021 at 09:30:58AM -0400, Brian Foster wrote:
> > perag reservation is enabled at mount time on a per AG basis. The
> > upcoming in-core allocation btree accounting mechanism needs to know
> > when reservation is enabled and that all perag AGF contexts are
> > initialized. As a preparation step, set a flag in the mount
> > structure and unconditionally initialize the pagf on all mounts
> > where at least one reservation is active.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag_resv.c | 24 ++++++++++++++----------
> >  fs/xfs/xfs_mount.h          |  1 +
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> > index 6c5f8d10589c..9b2fc4abad2c 100644
> > --- a/fs/xfs/libxfs/xfs_ag_resv.c
> > +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> > @@ -254,6 +254,7 @@ xfs_ag_resv_init(
> >  	xfs_extlen_t			ask;
> >  	xfs_extlen_t			used;
> >  	int				error = 0;
> > +	bool				has_resv = false;
> >  
> >  	/* Create the metadata reservation. */
> >  	if (pag->pag_meta_resv.ar_asked == 0) {
> > @@ -291,6 +292,8 @@ xfs_ag_resv_init(
> >  			if (error)
> >  				goto out;
> >  		}
> > +		if (ask)
> > +			has_resv = true;
> >  	}
> >  
> >  	/* Create the RMAPBT metadata reservation */
> > @@ -304,18 +307,19 @@ xfs_ag_resv_init(
> >  		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
> >  		if (error)
> >  			goto out;
> > +		if (ask)
> > +			has_resv = true;
> >  	}
> >  
> > -#ifdef DEBUG
> > -	/* need to read in the AGF for the ASSERT below to work */
> > -	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
> > -	if (error)
> > -		return error;
> > -
> > -	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> > -	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> > -	       pag->pagf_freeblks + pag->pagf_flcount);
> > -#endif
> > +	if (has_resv) {
> > +		mp->m_has_agresv = true;
> 
> If the metadata reservation succeeds but the rmapbt reservation fails
> with ENOSPC, won't we fail to set m_has_agresv true here?  We don't fail
> the entire mount if ENOSPC happens, which means that there's a slight
> chance of doing the wrong thing here if all the AGs are (somehow) like
> that.
> 

Yes it looks like we would skip setting the mount flag (and initializing
the pagf). I suppose we should probably just lift the out label up
before this whole branch.

Brian

> --D
> 
> > +		error = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0);
> > +		if (error)
> > +			return error;
> > +		ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> > +		       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> > +		       pag->pagf_freeblks + pag->pagf_flcount);
> > +	}
> >  out:
> >  	return error;
> >  }
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 81829d19596e..8847ffd29777 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -139,6 +139,7 @@ typedef struct xfs_mount {
> >  	bool			m_fail_unmount;
> >  	bool			m_finobt_nores; /* no per-AG finobt resv. */
> >  	bool			m_update_sb;	/* sb needs update in mount */
> > +	bool			m_has_agresv;	/* perag reservations active */
> >  
> >  	/*
> >  	 * Bitsets of per-fs metadata that have been checked and/or are sick.
> > -- 
> > 2.26.3
> > 
>