| Message ID | 20201008221905.GR6540@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
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.
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 > > >
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>
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. */