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@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> > 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?
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;
> > }
> >
> >
