[PATCH] NFS: implement option checking when remounting NFS filesystems

[PATCH] NFS: implement option checking when remounting NFS filesystems

[Jeff Layton]

When remounting an NFS or NFS4 filesystem, the new NFS options are not
respected, yet the remount will still return success. This patch adds
a remount_fs sb op for NFS that checks any new nfs mount options against
the existing ones and fails the mount if any have changed.

This is only implemented for string-based mount options since doing
this with binary options isn't really feasible.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/super.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f4ee9d9..b49f90f 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -207,6 +207,7 @@ static int nfs_xdev_get_sb(struct file_system_type *fs_type,
 		int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
 static void nfs_kill_super(struct super_block *);
 static void nfs_put_super(struct super_block *);
+static int nfs_remount(struct super_block *sb, int *flags, char *raw_data);
 
 static struct file_system_type nfs_fs_type = {
 	.owner		= THIS_MODULE,
@@ -234,6 +235,7 @@ static const struct super_operations nfs_sops = {
 	.umount_begin	= nfs_umount_begin,
 	.show_options	= nfs_show_options,
 	.show_stats	= nfs_show_stats,
+	.remount_fs	= nfs_remount,
 };
 
 #ifdef CONFIG_NFS_V4
@@ -278,6 +280,7 @@ static const struct super_operations nfs4_sops = {
 	.umount_begin	= nfs_umount_begin,
 	.show_options	= nfs_show_options,
 	.show_stats	= nfs_show_stats,
+	.remount_fs	= nfs_remount,
 };
 #endif
 
@@ -1327,6 +1330,73 @@ out_invalid_fh:
 	return -EINVAL;
 }
 
+static int
+nfs_compare_remount_data(struct nfs_server *nfss,
+			 struct nfs_parsed_mount_data *data)
+{
+	if (data->flags != nfss->flags ||
+	    data->rsize != nfss->rsize ||
+	    data->wsize != nfss->wsize ||
+	    data->retrans != nfss->client->cl_timeout->to_retries ||
+	    data->auth_flavors[0] != nfss->client->cl_auth->au_flavor ||
+	    data->acregmin != nfss->acregmin / HZ ||
+	    data->acregmax != nfss->acregmax / HZ ||
+	    data->acdirmin != nfss->acdirmin / HZ ||
+	    data->acdirmax != nfss->acdirmax / HZ ||
+	    data->timeo != (10U * nfss->client->cl_timeout->to_initval / HZ))
+		return -EINVAL;
+	else
+		return 0;
+}
+
+static int
+nfs_remount(struct super_block *sb, int *flags, char *raw_data)
+{
+	int error;
+	struct nfs_server *nfss = sb->s_fs_info;
+	struct nfs_parsed_mount_data *data;
+	struct nfs_mount_data *options = (struct nfs_mount_data *)raw_data;
+	struct nfs4_mount_data *options4 = (struct nfs4_mount_data *)raw_data;
+
+	/*
+	 * Userspace mount programs that send binary options generally send
+	 * them populated with default values. We have no way to know which
+	 * ones were explicitly specified. Fall back to legacy behavior and
+	 * just return success.
+	 */
+	if ((sb->s_type == &nfs4_fs_type && options4->version == 1) ||
+	    (sb->s_type == &nfs_fs_type && options->version >= 1 &&
+	     options->version <= 6))
+		return 0;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	/* fill out struct with values from existing mount */
+	data->flags = nfss->flags;
+	data->rsize = nfss->rsize;
+	data->wsize = nfss->wsize;
+	data->retrans = nfss->client->cl_timeout->to_retries;
+	data->auth_flavors[0] = nfss->client->cl_auth->au_flavor;
+	data->acregmin = nfss->acregmin / HZ;
+	data->acregmax = nfss->acregmax / HZ;
+	data->acdirmin = nfss->acdirmin / HZ;
+	data->acdirmax = nfss->acdirmax / HZ;
+	data->timeo = 10U * nfss->client->cl_timeout->to_initval / HZ;
+
+	/* overwrite those values with any that were specified */
+	error = nfs_parse_mount_options((char *)options, data);
+	if (error < 0)
+		goto out;
+
+	/* compare new mount options with old ones */
+	error = nfs_compare_remount_data(nfss, data);
+out:
+	kfree(data);
+	return error;
+}
+
 /*
  * Initialise the common bits of the superblock
  */
-- 
1.5.3.6

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Jeff Layton wrote:
> When remounting an NFS or NFS4 filesystem, the new NFS options are not
> respected, yet the remount will still return success. This patch adds
> a remount_fs sb op for NFS that checks any new nfs mount options against
> the existing ones and fails the mount if any have changed.
> 
> This is only implemented for string-based mount options since doing
> this with binary options isn't really feasible.

It might be feasible if mount.nfs were smart enough to read /etc/mtab 
and merge the old mount options with the remount options.

[-- Attachment #2: chuck_lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Jeff Layton]

On Fri, 11 Apr 2008 15:01:33 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Jeff Layton wrote:
> > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > respected, yet the remount will still return success. This patch adds
> > a remount_fs sb op for NFS that checks any new nfs mount options against
> > the existing ones and fails the mount if any have changed.
> > 
> > This is only implemented for string-based mount options since doing
> > this with binary options isn't really feasible.
> 
> It might be feasible if mount.nfs were smart enough to read /etc/mtab 
> and merge the old mount options with the remount options.

Yeah, but in that case you'd need a new kernel + a new nfs-utils. If
you're going to do that then you might as well just use the (far
superior) string based options anyway.

IMO, it's not worth the effort to implement this for binary options...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Peter Staubach]

Jeff Layton wrote:
> When remounting an NFS or NFS4 filesystem, the new NFS options are not
> respected, yet the remount will still return success. This patch adds
> a remount_fs sb op for NFS that checks any new nfs mount options against
> the existing ones and fails the mount if any have changed.
>
> This is only implemented for string-based mount options since doing
> this with binary options isn't really feasible.

What about respecting the new options as makes sense and rejecting
those which absolutely can't be changed dynamically?

       ps

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Trond Myklebust]

On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
> Jeff Layton wrote:
> > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > respected, yet the remount will still return success. This patch adds
> > a remount_fs sb op for NFS that checks any new nfs mount options against
> > the existing ones and fails the mount if any have changed.
> >
> > This is only implemented for string-based mount options since doing
> > this with binary options isn't really feasible.
> 
> What about respecting the new options as makes sense and rejecting
> those which absolutely can't be changed dynamically?

If we were to do this, then how should superblocks that are shared
between multiple mountpoints behave?

Cheers
  Trond

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Jeff Layton]

On Fri, 11 Apr 2008 15:32:27 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 
> On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
> > Jeff Layton wrote:
> > > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > > respected, yet the remount will still return success. This patch adds
> > > a remount_fs sb op for NFS that checks any new nfs mount options against
> > > the existing ones and fails the mount if any have changed.
> > >
> > > This is only implemented for string-based mount options since doing
> > > this with binary options isn't really feasible.
> > 
> > What about respecting the new options as makes sense and rejecting
> > those which absolutely can't be changed dynamically?
> 
> If we were to do this, then how should superblocks that are shared
> between multiple mountpoints behave?
> 

I was thinking the same thing and it occurs to me that that's an
existing bug that's not addressed by this patch. Right now, if you have
2 mounts that share a superblock, you can remount one with 'ro' and
*both* will end up being 'ro'.

I think I need to add a check for the sb reference count and return
-EBUSY or something if the sb is shared.

But, back to Peter's question...

In the case of a non-shared superblock, there aren't many options that
I can see that we can just apply. Some of the flags might be doable,
along with some of the ac* settings.

Changing the rsize/wsize means doing a fsinfo call, I think. The timeo
and retries are also tricky. If you have 2 separate sb's sharing a
nfs_client, then you have to be careful.

If we want to do that, then we certainly can consider it in the future,
this patch (with a check for shared sb's) should give us a starting
point if we want to do that.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Peter Staubach]

Trond Myklebust wrote:
> On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
>   
>> Jeff Layton wrote:
>>     
>>> When remounting an NFS or NFS4 filesystem, the new NFS options are not
>>> respected, yet the remount will still return success. This patch adds
>>> a remount_fs sb op for NFS that checks any new nfs mount options against
>>> the existing ones and fails the mount if any have changed.
>>>
>>> This is only implemented for string-based mount options since doing
>>> this with binary options isn't really feasible.
>>>       
>> What about respecting the new options as makes sense and rejecting
>> those which absolutely can't be changed dynamically?
>>     
>
> If we were to do this, then how should superblocks that are shared
> between multiple mountpoints behave?

Do you mean if those other mountpoints were mounted with explicit
options and this remount might affect those options?

Isn't the same problem that we potentially have today?

    Thanx...

       ps

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Trond Myklebust]

On Fri, 2008-04-11 at 16:14 -0400, Peter Staubach wrote:
> Trond Myklebust wrote:
> > On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
> >   
> >> Jeff Layton wrote:
> >>     
> >>> When remounting an NFS or NFS4 filesystem, the new NFS options are not
> >>> respected, yet the remount will still return success. This patch adds
> >>> a remount_fs sb op for NFS that checks any new nfs mount options against
> >>> the existing ones and fails the mount if any have changed.
> >>>
> >>> This is only implemented for string-based mount options since doing
> >>> this with binary options isn't really feasible.
> >>>       
> >> What about respecting the new options as makes sense and rejecting
> >> those which absolutely can't be changed dynamically?
> >>     
> >
> > If we were to do this, then how should superblocks that are shared
> > between multiple mountpoints behave?
> 
> Do you mean if those other mountpoints were mounted with explicit
> options and this remount might affect those options?
> 
> Isn't the same problem that we potentially have today?

Sure, but I'm trying to figure out what you mean when you talk about
"rejecting those which absolutely can't be changed dynamically". We can
potentially change pretty much any parameter dynamically if we really
want to (with the sole exception of 'nosharecache').

Cheers
  Trond

Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

[Peter Staubach]

Trond Myklebust wrote:
> On Fri, 2008-04-11 at 16:14 -0400, Peter Staubach wrote:
>   
>> Trond Myklebust wrote:
>>     
>>> On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
>>>   
>>>       
>>>> Jeff Layton wrote:
>>>>     
>>>>         
>>>>> When remounting an NFS or NFS4 filesystem, the new NFS options are not
>>>>> respected, yet the remount will still return success. This patch adds
>>>>> a remount_fs sb op for NFS that checks any new nfs mount options against
>>>>> the existing ones and fails the mount if any have changed.
>>>>>
>>>>> This is only implemented for string-based mount options since doing
>>>>> this with binary options isn't really feasible.
>>>>>       
>>>>>           
>>>> What about respecting the new options as makes sense and rejecting
>>>> those which absolutely can't be changed dynamically?
>>>>     
>>>>         
>>> If we were to do this, then how should superblocks that are shared
>>> between multiple mountpoints behave?
>>>       
>> Do you mean if those other mountpoints were mounted with explicit
>> options and this remount might affect those options?
>>
>> Isn't the same problem that we potentially have today?
>>     
>
> Sure, but I'm trying to figure out what you mean when you talk about
> "rejecting those which absolutely can't be changed dynamically". We can
> potentially change pretty much any parameter dynamically if we really
> want to (with the sole exception of 'nosharecache').

I hadn't actually put enough thought into which options can be
changed dynamically and which ones can not.  I presume that we
could change anything that we want, but I think that it would
be good to ensure that the changes would be safe.

In the past, I have seen problems with adjusting wsize dynamically.
It needs to be done carefully or potential problems with the
server not being able to detect duplicate requests occur.  That,
of course, was with a different implementation, but it has made
me wary ever since.

    Thanx...

       ps