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

[Chandan Babu R <chandanrlinux@gmail.com>]

On Friday 9 October 2020 8:51:26 PM IST Darrick J. Wong wrote:
> 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.
>

Ok.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

-- 
chandan