Re: [PATCH v2.2 2/3] xfs: make xfs_growfs_rt update secondary superblocks

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Chandan Babu R <chandanrlinux@gmail.com>
Cc: linux-xfs@vger.kernel.org, sandeen@redhat.com
Subject: Re: [PATCH v2.2 2/3] xfs: make xfs_growfs_rt update secondary superblocks
Date: Fri, 9 Oct 2020 08:21:26 -0700	[thread overview]
Message-ID: <20201009152126.GS6540@magnolia> (raw)
In-Reply-To: <2785429.vsROyPpyBe@garuda>

On Fri, Oct 09, 2020 at 03:21:38PM +0530, Chandan Babu R wrote:
> On Friday 9 October 2020 3:49:05 AM IST Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we call growfs on the data device, we update the secondary
> > superblocks to reflect the updated filesystem geometry.  We need to do
> > this for growfs on the realtime volume too, because a future xfs_repair
> > run could try to fix the filesystem using a backup superblock.
> > 
> > This was observed by the online superblock scrubbers while running
> > xfs/233.  One can also trigger this by growing an rt volume, cycling the
> > mount, and creating new rt files.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2.2: don't update on error, don't fail to free memory on error
> > ---
> >  fs/xfs/xfs_rtalloc.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 1c3969807fb9..f9119ba3e9d0 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -18,7 +18,7 @@
> >  #include "xfs_trans_space.h"
> >  #include "xfs_icache.h"
> >  #include "xfs_rtalloc.h"
> > -
> > +#include "xfs_sb.h"
> >  
> >  /*
> >   * Read and return the summary information for a given extent size,
> > @@ -1102,7 +1102,13 @@ xfs_growfs_rt(
> >  		if (error)
> >  			break;
> >  	}
> > +	if (error)
> > +		goto out_free;
> >  
> > +	/* Update secondary superblocks now the physical grow has completed */
> > +	error = xfs_update_secondary_sbs(mp);
> > +
> > +out_free:
> >  	/*
> >  	 * Free the fake mp structure.
> >  	 */
> > 
> 
> How about ...
> 
> if (!error) {
> 	/* Update secondary superblocks now the physical grow has completed */
> 	error = xfs_update_secondary_sbs(mp);
> }
> 
> /*
>  * Free the fake mp structure.
>  */
> ...
> ... 
> 
> With the above construct we can get rid of the goto label.

I'd rather not start doing that, because (a) we generally don't do that
in xfs and (b) in a cycle or two I'm going to add more in-memory state
changes between the secondary super update and freeing the fake mp, and
I'd prefer to start all that by having the error case jump to out_free.

--D

> -- 
> chandan
> 
> 
> 