diff mbox series

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

Message ID 20210412133059.1186634-2-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series xfs: set aside allocation btree blocks from block reservation | expand

Commit Message

Brian Foster April 12, 2021, 1:30 p.m. UTC
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(-)

Comments

Darrick J. Wong April 14, 2021, 12:49 a.m. UTC | #1
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.

--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
>
Brian Foster April 20, 2021, 4:22 p.m. UTC | #2
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
> > 
>
Brian Foster April 20, 2021, 4:23 p.m. UTC | #3
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
> > 
>
diff mbox series

Patch

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;
+		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.