[PATCH] xfs: restore old agirotor behavior

[PATCH] xfs: restore old agirotor behavior

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Prior to the removal of xfs_ialloc_next_ag, we would increment the agi
rotor and return the *old* value.  atomic_inc_return returns the new
value, which causes mkfs to allocate the root directory in AG 1.  Put
back the old behavior (at least for mkfs) by subtracting 1 here.

Fixes: 20a5eab49d35 ("xfs: convert xfs_ialloc_next_ag() to an atomic")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_ialloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 65832c74e86c..550c6351e9b6 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1729,7 +1729,7 @@ xfs_dialloc(
 	 * an AG has enough space for file creation.
 	 */
 	if (S_ISDIR(mode))
-		start_agno = atomic_inc_return(&mp->m_agirotor) % mp->m_maxagi;
+		start_agno = (atomic_inc_return(&mp->m_agirotor) - 1) % mp->m_maxagi;
 	else {
 		start_agno = XFS_INO_TO_AGNO(mp, parent);
 		if (start_agno >= mp->m_maxagi)

Re: [PATCH] xfs: restore old agirotor behavior

[Dave Chinner]

On Tue, Feb 21, 2023 at 09:29:00PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Prior to the removal of xfs_ialloc_next_ag, we would increment the agi
> rotor and return the *old* value.  atomic_inc_return returns the new
> value, which causes mkfs to allocate the root directory in AG 1.  Put
> back the old behavior (at least for mkfs) by subtracting 1 here.
> 
> Fixes: 20a5eab49d35 ("xfs: convert xfs_ialloc_next_ag() to an atomic")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_ialloc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 65832c74e86c..550c6351e9b6 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1729,7 +1729,7 @@ xfs_dialloc(
>  	 * an AG has enough space for file creation.
>  	 */
>  	if (S_ISDIR(mode))
> -		start_agno = atomic_inc_return(&mp->m_agirotor) % mp->m_maxagi;
> +		start_agno = (atomic_inc_return(&mp->m_agirotor) - 1) % mp->m_maxagi;
>  	else {
>  		start_agno = XFS_INO_TO_AGNO(mp, parent);
>  		if (start_agno >= mp->m_maxagi)

Change is fine, but it pushes the code to 85 columns. If you clean
it up to:

	if (S_ISDIR(mode)) {
		start_agno = (atomic_inc_return(&mp->m_agirotor) - 1) %
				mp->m_maxagi;
	} else {
		....

Then you can add:

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: restore old agirotor behavior

[Darrick J. Wong]

On Mon, Feb 27, 2023 at 09:02:01AM +1100, Dave Chinner wrote:
> On Tue, Feb 21, 2023 at 09:29:00PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Prior to the removal of xfs_ialloc_next_ag, we would increment the agi
> > rotor and return the *old* value.  atomic_inc_return returns the new
> > value, which causes mkfs to allocate the root directory in AG 1.  Put
> > back the old behavior (at least for mkfs) by subtracting 1 here.
> > 
> > Fixes: 20a5eab49d35 ("xfs: convert xfs_ialloc_next_ag() to an atomic")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_ialloc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index 65832c74e86c..550c6351e9b6 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -1729,7 +1729,7 @@ xfs_dialloc(
> >  	 * an AG has enough space for file creation.
> >  	 */
> >  	if (S_ISDIR(mode))
> > -		start_agno = atomic_inc_return(&mp->m_agirotor) % mp->m_maxagi;
> > +		start_agno = (atomic_inc_return(&mp->m_agirotor) - 1) % mp->m_maxagi;
> >  	else {
> >  		start_agno = XFS_INO_TO_AGNO(mp, parent);
> >  		if (start_agno >= mp->m_maxagi)
> 
> Change is fine, but it pushes the code to 85 columns. If you clean
> it up to:
> 
> 	if (S_ISDIR(mode)) {
> 		start_agno = (atomic_inc_return(&mp->m_agirotor) - 1) %
> 				mp->m_maxagi;
> 	} else {
> 		....
> 
> Then you can add:
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks, merged with tweaks!

--D

> 
> -- 
> Dave Chinner
> david@fromorbit.com