xfs
[Top] [All Lists]

Re: [PATCH] fix mount option parsing in remount

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH] fix mount option parsing in remount
From: Christoph Hellwig <hch@xxxxxx>
Date: Fri, 27 Jun 2008 15:11:36 +0200
In-reply-to: <20080603080019.GB19608@xxxxxx>
References: <20080518153539.GA5218@xxxxxx> <20080603080019.GB19608@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.3.28i
ping^2

On Tue, Jun 03, 2008 at 10:00:19AM +0200, Christoph Hellwig wrote:
> ping?  silently ignoring wrong options causes a lot of confusion for
> users, and the patch sould simple enough.  Anyone take 10 minutes to
> review it and check it in?
> 
> On Sun, May 18, 2008 at 05:35:39PM +0200, Christoph Hellwig wrote:
> > Remount currently happily accept any option thrown at it, although the
> > only filesystem specific option it actually handles is barrier/nobarrier.
> > And it actually doesn't handle these correctly either because it only
> > uses the value it parsed when we're doing a ro->rw transition.  In
> > addition to that there's also a bad bug in xfs_parseargs which doesn't
> > touch the actual option in the mount point except for a single one,
> > XFS_MOUNT_SMALL_INUMS and thus forced any filesystem that's every
> > remounted in some way to not support 64bit inodes with no way to recover
> > unless unmounted.
> > 
> > This patch changes xfs_fs_remount to use it's own linux/parser.h based
> > options parse instead of xfs_parseargs and reject all options except
> > for barrier/nobarrier and to the right thing in general.  Eventually
> > I'd like to have a single big option table used for mount aswell but
> > that can wait for a while.
> > 
> > 
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > 
> > Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
> > ===================================================================
> > --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.c 2008-05-18 
> > 15:22:23.000000000 +0200
> > +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c      2008-05-18 
> > 15:59:32.000000000 +0200
> > @@ -67,6 +67,7 @@
> >  #include <linux/writeback.h>
> >  #include <linux/kthread.h>
> >  #include <linux/freezer.h>
> > +#include <linux/parser.h>
> >  
> >  static struct quotactl_ops xfs_quotactl_operations;
> >  static struct super_operations xfs_super_operations;
> > @@ -1255,6 +1256,19 @@ xfs_fs_statfs(
> >     return 0;
> >  }
> >  
> > +/*
> > + * Eventually we should extend this table and use it for mount, too.
> > + */
> > +enum {
> > +   Opt_barrier, Opt_nobarrier, Opt_err
> > +};
> > +
> > +static match_table_t tokens = {
> > +   {Opt_barrier, "barrier"},
> > +   {Opt_nobarrier, "nobarrier"},
> > +   {Opt_err, NULL}
> > +};
> > +
> >  STATIC int
> >  xfs_fs_remount(
> >     struct super_block      *sb,
> > @@ -1262,36 +1276,54 @@ xfs_fs_remount(
> >     char                    *options)
> >  {
> >     struct xfs_mount        *mp = XFS_M(sb);
> > -   struct xfs_mount_args   *args;
> > -   int                     error;
> > +   substring_t             args[MAX_OPT_ARGS];
> > +   char                    *p;
> >  
> > -   args = xfs_args_allocate(sb, 0);
> > -   if (!args)
> > -           return -ENOMEM;
> > +   while ((p = strsep(&options, ",")) != NULL) {
> > +           int token;
> >  
> > -   error = xfs_parseargs(mp, options, args, 1);
> > -   if (error)
> > -           goto out_free_args;
> > +           if (!*p)
> > +                   continue;
> >  
> > -   if (!(*flags & MS_RDONLY)) {                    /* rw/ro -> rw */
> > -           if (mp->m_flags & XFS_MOUNT_RDONLY)
> > -           mp->m_flags &= ~XFS_MOUNT_RDONLY;
> > -           if (args->flags & XFSMNT_BARRIER) {
> > +           token = match_token(p, tokens, args);
> > +           switch (token) {
> > +           case Opt_barrier:
> >                     mp->m_flags |= XFS_MOUNT_BARRIER;
> > -                   xfs_mountfs_check_barriers(mp);
> > -           } else {
> > +
> > +                   /*
> > +                    * Test if barriers are actually working if we can,
> > +                    * else delay this check until the filesystem is
> > +                    * marked writeable.
> > +                    */
> > +                   if (!(mp->m_flags & XFS_MOUNT_RDONLY))
> > +                           xfs_mountfs_check_barriers(mp);
> > +                   break;
> > +           case Opt_nobarrier:
> >                     mp->m_flags &= ~XFS_MOUNT_BARRIER;
> > +                   break;
> > +           default:
> > +                   printk(KERN_INFO
> > +   "XFS: mount option \"%s\" not support for remount\n", p);
> > +                   return -EINVAL;
> >             }
> > -   } else if (!(mp->m_flags & XFS_MOUNT_RDONLY)) { /* rw -> ro */
> > +   }
> > +
> > +   /* rw/ro -> rw */
> > +   if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
> > +           mp->m_flags &= ~XFS_MOUNT_RDONLY;
> > +           if (mp->m_flags & XFS_MOUNT_BARRIER)
> > +                   xfs_mountfs_check_barriers(mp);
> > +   }
> > +
> > +   /* rw -> ro */
> > +   if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & MS_RDONLY)) {
> >             xfs_filestream_flush(mp);
> >             xfs_sync(mp, SYNC_DATA_QUIESCE);
> >             xfs_attr_quiesce(mp);
> >             mp->m_flags |= XFS_MOUNT_RDONLY;
> >     }
> >  
> > - out_free_args:
> > -   kfree(args);
> > -   return -error;
> > +   return 0;
> >  }
> >  
> >  /*
> ---end quoted text---
---end quoted text---


<Prev in Thread] Current Thread [Next in Thread>