Re: [PATCH v7 09/17] xfs: add xfs_remount_rw() helper

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Oct 25, 2019 at 05:53:08AM +0800, Ian Kent wrote:
> On Thu, 2019-10-24 at 08:31 -0700, Darrick J. Wong wrote:
> > On Thu, Oct 24, 2019 at 03:51:22PM +0800, Ian Kent wrote:
> > > Factor the remount read write code into a helper to simplify the
> > > subsequent change from the super block method .remount_fs to the
> > > mount-api fs_context_operations method .reconfigure.
> > > 
> > > This helper is only used by the mount code, so locate it along with
> > > that code.
> > > 
> > > While we are at it change STATIC -> static for
> > > xfs_restore_resvblks().
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_super.c |  119 +++++++++++++++++++++++++++++-----------
> > > ------------
> > >  1 file changed, 67 insertions(+), 52 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 297e6c98742e..c07e41489e75 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -47,6 +47,8 @@ static struct kset *xfs_kset;		/* top-
> > > level xfs sysfs dir */
> > >  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs
> > > attrs */
> > >  #endif
> > >  
> > > +static void xfs_restore_resvblks(struct xfs_mount *mp);
> > 
> > What's the reason for putting xfs_remount_rw above
> > xfs_restore_resvblks?
> > I assume that's related to where you want to land later code chunks?
> 
> In the cover letter:
> 
> Note: the patches "xfs: add xfs_remount_rw() helper" and
>  "xfs: add xfs_remount_ro() helper" that have Reviewed-by attributions
>  each needed a forward declartion added due grouping all the mount
>  related code together. Reviewers may want to check the attribution
>  is still acceptable.
> 
> The fill super method needs quite a few more forward declarations
> too.
> 
> I responded to Christoph's suggestion of grouping the mount code
> together saying this would be needed, and that I thought the
> improvement of grouping the code together was worth the forward
> declarations, and asked if anyone had a different POV on it and
> got no replies, ;)
> 
> The other thing is that the options definitions notionally belong
> near the top of the mount/super block handling code so moving it
> all down seemed like the wrong thing to do ...
> 
> So what do you think of the extra noise of forward declarations
> in this case?

Eh, fine with me.  I was just curious, having speed-read over the
previous iterations. :)

--D

> Ian
> 
> > 
> > --D
> > 
> > > +
> > >  /*
> > >   * Table driven mount option parser.
> > >   */
> > > @@ -455,6 +457,68 @@ xfs_mount_free(
> > >  	kmem_free(mp);
> > >  }
> > >  
> > > +static int
> > > +xfs_remount_rw(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	struct xfs_sb		*sbp = &mp->m_sb;
> > > +	int			error;
> > > +
> > > +	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
> > > +		xfs_warn(mp,
> > > +			"ro->rw transition prohibited on norecovery
> > > mount");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> > > +	    xfs_sb_has_ro_compat_feature(sbp,
> > > XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
> > > +		xfs_warn(mp,
> > > +	"ro->rw transition prohibited on unknown (0x%x) ro-compat
> > > filesystem",
> > > +			(sbp->sb_features_ro_compat &
> > > +				XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	mp->m_flags &= ~XFS_MOUNT_RDONLY;
> > > +
> > > +	/*
> > > +	 * If this is the first remount to writeable state we might
> > > have some
> > > +	 * superblock changes to update.
> > > +	 */
> > > +	if (mp->m_update_sb) {
> > > +		error = xfs_sync_sb(mp, false);
> > > +		if (error) {
> > > +			xfs_warn(mp, "failed to write sb changes");
> > > +			return error;
> > > +		}
> > > +		mp->m_update_sb = false;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Fill out the reserve pool if it is empty. Use the stashed
> > > value if
> > > +	 * it is non-zero, otherwise go with the default.
> > > +	 */
> > > +	xfs_restore_resvblks(mp);
> > > +	xfs_log_work_queue(mp);
> > > +
> > > +	/* Recover any CoW blocks that never got remapped. */
> > > +	error = xfs_reflink_recover_cow(mp);
> > > +	if (error) {
> > > +		xfs_err(mp,
> > > +			"Error %d recovering leftover CoW
> > > allocations.", error);
> > > +			xfs_force_shutdown(mp,
> > > SHUTDOWN_CORRUPT_INCORE);
> > > +		return error;
> > > +	}
> > > +	xfs_start_block_reaping(mp);
> > > +
> > > +	/* Create the per-AG metadata reservation pool .*/
> > > +	error = xfs_fs_reserve_ag_blocks(mp);
> > > +	if (error && error != -ENOSPC)
> > > +		return error;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  struct proc_xfs_info {
> > >  	uint64_t	flag;
> > >  	char		*str;
> > > @@ -1169,7 +1233,7 @@ xfs_save_resvblks(struct xfs_mount *mp)
> > >  	xfs_reserve_blocks(mp, &resblks, NULL);
> > >  }
> > >  
> > > -STATIC void
> > > +static void
> > >  xfs_restore_resvblks(struct xfs_mount *mp)
> > >  {
> > >  	uint64_t resblks;
> > > @@ -1307,57 +1371,8 @@ xfs_fs_remount(
> > >  
> > >  	/* ro -> rw */
> > >  	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & SB_RDONLY))
> > > {
> > > -		if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
> > > -			xfs_warn(mp,
> > > -		"ro->rw transition prohibited on norecovery mount");
> > > -			return -EINVAL;
> > > -		}
> > > -
> > > -		if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> > > -		    xfs_sb_has_ro_compat_feature(sbp,
> > > -					XFS_SB_FEAT_RO_COMPAT_UNKNOWN))
> > > {
> > > -			xfs_warn(mp,
> > > -"ro->rw transition prohibited on unknown (0x%x) ro-compat
> > > filesystem",
> > > -				(sbp->sb_features_ro_compat &
> > > -					XFS_SB_FEAT_RO_COMPAT_UNKNOWN))
> > > ;
> > > -			return -EINVAL;
> > > -		}
> > > -
> > > -		mp->m_flags &= ~XFS_MOUNT_RDONLY;
> > > -
> > > -		/*
> > > -		 * If this is the first remount to writeable state we
> > > -		 * might have some superblock changes to update.
> > > -		 */
> > > -		if (mp->m_update_sb) {
> > > -			error = xfs_sync_sb(mp, false);
> > > -			if (error) {
> > > -				xfs_warn(mp, "failed to write sb
> > > changes");
> > > -				return error;
> > > -			}
> > > -			mp->m_update_sb = false;
> > > -		}
> > > -
> > > -		/*
> > > -		 * Fill out the reserve pool if it is empty. Use the
> > > stashed
> > > -		 * value if it is non-zero, otherwise go with the
> > > default.
> > > -		 */
> > > -		xfs_restore_resvblks(mp);
> > > -		xfs_log_work_queue(mp);
> > > -
> > > -		/* Recover any CoW blocks that never got remapped. */
> > > -		error = xfs_reflink_recover_cow(mp);
> > > -		if (error) {
> > > -			xfs_err(mp,
> > > -	"Error %d recovering leftover CoW allocations.", error);
> > > -			xfs_force_shutdown(mp,
> > > SHUTDOWN_CORRUPT_INCORE);
> > > -			return error;
> > > -		}
> > > -		xfs_start_block_reaping(mp);
> > > -
> > > -		/* Create the per-AG metadata reservation pool .*/
> > > -		error = xfs_fs_reserve_ag_blocks(mp);
> > > -		if (error && error != -ENOSPC)
> > > +		error = xfs_remount_rw(mp);
> > > +		if (error)
> > >  			return error;
> > >  	}
> > >  
> > > 
>