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

[Christoph Hellwig <hch@infradead.org>]

On Thu, Oct 24, 2019 at 04:12:58PM -0700, Darrick J. Wong wrote:
> > 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. :)

So looking at the result I'm not sure my suggestion was productive.  It
turns out there is a fair chunk of mount code at the end of the file as
well, and there is literally nothing left of the the code towards the
top of the file except for the Opt_* definitions.  So while I think
using this rewrite as a chance to group the code is still a good
idea, I suspect the better (and actually more natural) place is toward
the bottom of the file.
> 
> --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;
> > > >  	}
> > > >  
> > > > 
> > 
---end quoted text---