* Re: [PATCH] fix mount option parsing in remount
2008-05-18 15:35 [PATCH] fix mount option parsing in remount Christoph Hellwig
@ 2008-06-03 8:00 ` Christoph Hellwig
2008-06-27 13:11 ` Christoph Hellwig
2008-06-03 12:51 ` Eric Sandeen
2008-06-30 6:21 ` Timothy Shimmin
2 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2008-06-03 8:00 UTC (permalink / raw)
To: xfs
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@lst.de>
>
> 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---
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] fix mount option parsing in remount
2008-06-03 8:00 ` Christoph Hellwig
@ 2008-06-27 13:11 ` Christoph Hellwig
2008-06-27 14:49 ` Eric Sandeen
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2008-06-27 13:11 UTC (permalink / raw)
To: xfs
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@lst.de>
> >
> > 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---
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] fix mount option parsing in remount
2008-06-27 13:11 ` Christoph Hellwig
@ 2008-06-27 14:49 ` Eric Sandeen
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sandeen @ 2008-06-27 14:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Christoph Hellwig wrote:
> 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?
I sorta reviewed it earlier but I'll give it my formal ACK.
-Eric
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix mount option parsing in remount
2008-05-18 15:35 [PATCH] fix mount option parsing in remount Christoph Hellwig
2008-06-03 8:00 ` Christoph Hellwig
@ 2008-06-03 12:51 ` Eric Sandeen
2008-06-03 13:05 ` Christoph Hellwig
2008-06-30 6:21 ` Timothy Shimmin
2 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2008-06-03 12:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
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@lst.de>
>
> 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.
Maybe expand this a little:
/*
* Today only the barrier/nobarrier xfs mount options are handled
* on remount. This table should be expanded to cover all options,
* and used for the initial mount path, too.
*/
... I'd also like this for the mount path because for example:
mount -t xfs -o barrier=0 /dev/blah /mnt/foo
works... but does not turn off barriers.
So as a first step/example this looks ok but I'd sure like to see it
extended soon. Maybe when I'm bored... :)
-Eric
> + */
> +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;
> }
>
> /*
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] fix mount option parsing in remount
2008-06-03 12:51 ` Eric Sandeen
@ 2008-06-03 13:05 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2008-06-03 13:05 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Christoph Hellwig, xfs
On Tue, Jun 03, 2008 at 07:51:41AM -0500, Eric Sandeen wrote:
> Maybe expand this a little:
>
> /*
> * Today only the barrier/nobarrier xfs mount options are handled
> * on remount. This table should be expanded to cover all options,
> * and used for the initial mount path, too.
> */
>
> ... I'd also like this for the mount path because for example:
>
> mount -t xfs -o barrier=0 /dev/blah /mnt/foo
>
> works... but does not turn off barriers.
>
> So as a first step/example this looks ok but I'd sure like to see it
> extended soon. Maybe when I'm bored... :)
Doh. But yeah, mount path should be switched over soon. I alredy
posted the patches to kill the xfs_mount_args gunk which have been on
the list for a while and happily ignored, and switching to the parser.h
helper ontop of that is quite easy. I just want to wait a little bit
so that any problems with the previous patch are shaked out before the
new one gets added.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix mount option parsing in remount
2008-05-18 15:35 [PATCH] fix mount option parsing in remount Christoph Hellwig
2008-06-03 8:00 ` Christoph Hellwig
2008-06-03 12:51 ` Eric Sandeen
@ 2008-06-30 6:21 ` Timothy Shimmin
2008-06-30 16:18 ` Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Timothy Shimmin @ 2008-06-30 6:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
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@lst.de>
>
> 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);
typo: s/support/supported/
Looking at ext3 and other XFS out of curiosity:
"XFS: unknown mount option [%s].", this_char
"EXT3-fs: Unrecognized mount option \"%s\" "
./smbfs/getopt.c: printk("%s: Unrecognized mount option %s\n", caller, token);
Though I see for remount it is more a question of support
versus recognising it or not. Ok.
> + 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;
> }
>
I'm a little confused why we call xfs_mountfs_check_barriers(mp) in 2 places.
Oh okay,
so you want it tested if we have barrier option and
currently not-readonly or after we transition to not-readonly.
And you have it around the parsing code because you don't care about
what it might transition to in the current not-readonly case.
I think we ideally should have a qa test for this.
We have 017 but it doesn't do any barrier or illegal options.
--Tim
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] fix mount option parsing in remount
2008-06-30 6:21 ` Timothy Shimmin
@ 2008-06-30 16:18 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2008-06-30 16:18 UTC (permalink / raw)
To: Timothy Shimmin; +Cc: Christoph Hellwig, xfs
On Mon, Jun 30, 2008 at 04:21:45PM +1000, Timothy Shimmin wrote:
> typo: s/support/supported/
>
> Looking at ext3 and other XFS out of curiosity:
> "XFS: unknown mount option [%s].", this_char
> "EXT3-fs: Unrecognized mount option \"%s\" "
> ./smbfs/getopt.c: printk("%s: Unrecognized mount option %s\n", caller, token);
> Though I see for remount it is more a question of support
> versus recognising it or not. Ok.
I've fixed the typo. The recognized vs supported thing will get a
little better once we switch to the parser table for mount, at which
point we can make the message more fine-grained and report either
unrecognized or unsupported. But until we need the full blown parser
table I'd rather keep it simple. But once this one and my patch series
to kill struct xfs_mount_args is in the full-blown switch to the table
driven parser is quite easy.
> I'm a little confused why we call xfs_mountfs_check_barriers(mp) in 2 places.
> Oh okay,
> so you want it tested if we have barrier option and
> currently not-readonly or after we transition to not-readonly.
> And you have it around the parsing code because you don't care about
> what it might transition to in the current not-readonly case.
Yes, if the filesystem is currently read-write we want to test the
barriers immediately, if it's read-only we can't test it just yet but
have to wait until the filesystems is remounted r/w.
> I think we ideally should have a qa test for this.
> We have 017 but it doesn't do any barrier or illegal options.
Makes sense. I'll look into doing a QA test.
Signed-off-by: Christoph Hellwig <hch@lst.de>
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-06-29 14:03:01.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c 2008-06-30 14:42:02.000000000 +0200
@@ -66,6 +66,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;
@@ -147,6 +148,23 @@ xfs_args_allocate(
#define MNTOPT_XDSM "xdsm" /* DMI enabled (DMAPI / XDSM) */
#define MNTOPT_DMI "dmi" /* DMI enabled (DMAPI / XDSM) */
+/*
+ * Table driven mount option parser.
+ *
+ * Currently only used for remount, but it will be used for mount
+ * in the future, too.
+ */
+enum {
+ Opt_barrier, Opt_nobarrier, Opt_err
+};
+
+static match_table_t tokens = {
+ {Opt_barrier, "barrier"},
+ {Opt_nobarrier, "nobarrier"},
+ {Opt_err, NULL}
+};
+
+
STATIC unsigned long
suffix_strtoul(char *s, char **endp, unsigned int base)
{
@@ -1261,36 +1279,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 supported 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;
}
/*
^ permalink raw reply [flat|nested] 8+ messages in thread