| Message ID | 20140211133013.1ccf8615@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Feb 10, 2014, at 21:30, NeilBrown <neilb@suse.de> wrote: > > (I seem to have quite a pile of NFS issues lately.... some if it is tidying up > issues from before Christmas, some of it just keeps on coming :-) > > If you run "mount -a" it will attempt to mount all filesystems listed > in /etc/fstab, but filesystems that are already mounted will not be mounted > again. So it is normally safe to run "mount -a" multiple times. > > However if an NFS mount in /etc/fstab has the "nosharecache" option set, > mount doesn't notice that it is already mounted as so mounts it again. So > repeated "mount -a" is no longer safe. > > This happens because the prevention of multiple mounts happens in do_add_mount > in fs/namespace.c: > > err = -EBUSY; > if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb && > path->mnt->mnt_root == path->dentry) > goto unlock; > > > i.e. if the exact same superblock is being mounted in the exact same place, > return EBUSY. > With nosharecache, every mount attempt produces a new superblock so this test > never fires. > > One way to address this would be to have a different option, e.g. > sharecache=27 > > where the '27' is an arbitrary number meaning that if two mount attempts have > different sharecache numbers they will have different superblocks. If they > have the same sharecache number they can have the same superblock. > This is not the most elegant interface ever and I would be very happy for > suggestions to improve it. Maybe a string rather than a number ??? > > This probably isn't a very serious issue, but is a regression in terms of the > usability of "mount -a" and I think it would be best to fix it if the cost is > not too high. > > Below is my patch to implement the "sharecache=%u" syntax. > > Any ideas? What are people using nosharecache for these days, and why is this not another argument for just getting rid of it?
On Mon, 10 Feb 2014 23:19:09 -0500 Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > On Feb 10, 2014, at 21:30, NeilBrown <neilb@suse.de> wrote: > > > > > (I seem to have quite a pile of NFS issues lately.... some if it is tidying up > > issues from before Christmas, some of it just keeps on coming :-) > > > > If you run "mount -a" it will attempt to mount all filesystems listed > > in /etc/fstab, but filesystems that are already mounted will not be mounted > > again. So it is normally safe to run "mount -a" multiple times. > > > > However if an NFS mount in /etc/fstab has the "nosharecache" option set, > > mount doesn't notice that it is already mounted as so mounts it again. So > > repeated "mount -a" is no longer safe. > > > > This happens because the prevention of multiple mounts happens in do_add_mount > > in fs/namespace.c: > > > > err = -EBUSY; > > if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb && > > path->mnt->mnt_root == path->dentry) > > goto unlock; > > > > > > i.e. if the exact same superblock is being mounted in the exact same place, > > return EBUSY. > > With nosharecache, every mount attempt produces a new superblock so this test > > never fires. > > > > One way to address this would be to have a different option, e.g. > > sharecache=27 > > > > where the '27' is an arbitrary number meaning that if two mount attempts have > > different sharecache numbers they will have different superblocks. If they > > have the same sharecache number they can have the same superblock. > > This is not the most elegant interface ever and I would be very happy for > > suggestions to improve it. Maybe a string rather than a number ??? > > > > This probably isn't a very serious issue, but is a regression in terms of the > > usability of "mount -a" and I think it would be best to fix it if the cost is > > not too high. > > > > Below is my patch to implement the "sharecache=%u" syntax. > > > > Any ideas? > > What are people using nosharecache for these days, and why is this not another argument for just getting rid of it? Baby or bath water? In the particular case that brought this up for me they are actually using "nosharetransport" which is a similar option I provide in SLES because there are cases where sharing the transport reduces performance, and cases where sharing the transport interacts poorly with particular details (of particular versions) of the Netapp filer. Seeing the root problem is the same for nosharecache and nosharetransport it seems reasonable to report it upstream as an issue with nosharecache. If you don't think this is worth "fixing", I certainly don't feel strongly enough about it to argue. Thanks, NeilBrown
diff --git a/fs/nfs/client.c b/fs/nfs/client.c index 1d09289c8f0e..7ed2f1e37a18 100644 --- a/fs/nfs/client.c +++ b/fs/nfs/client.c @@ -779,6 +779,7 @@ static int nfs_init_server(struct nfs_server *server, server->acregmax = data->acregmax * HZ; server->acdirmin = data->acdirmin * HZ; server->acdirmax = data->acdirmax * HZ; + server->cache_id = data->cache_id; /* Start lockd here, before we might error out */ error = nfs_start_lockd(server); @@ -931,6 +932,7 @@ void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *sour target->caps = source->caps; target->options = source->options; target->auth_info = source->auth_info; + target->cache_id = source->cache_id; } EXPORT_SYMBOL_GPL(nfs_server_copy_userdata); diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index 8b5cc04a8611..b1c2b9164f25 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -95,6 +95,7 @@ struct nfs_parsed_mount_data { unsigned int minorversion; char *fscache_uniq; bool need_mount; + unsigned int cache_id; struct { struct sockaddr_storage address; diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c index dbb3e1f30c68..25f426db68f9 100644 --- a/fs/nfs/nfs4client.c +++ b/fs/nfs/nfs4client.c @@ -997,6 +997,7 @@ static int nfs4_init_server(struct nfs_server *server, server->acregmax = data->acregmax * HZ; server->acdirmin = data->acdirmin * HZ; server->acdirmax = data->acdirmax * HZ; + server->cache_id = data->cache_id; server->port = data->nfs_server.port; diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 910ed906eb82..27edb581affa 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -103,6 +103,7 @@ enum { Opt_mountport, Opt_mountvers, Opt_minorversion, + Opt_sharecacheid, /* Mount options that take string arguments */ Opt_nfsvers, @@ -145,6 +146,7 @@ static const match_table_t nfs_mount_option_tokens = { { Opt_rdirplus, "rdirplus" }, { Opt_nordirplus, "nordirplus" }, { Opt_sharecache, "sharecache" }, + { Opt_sharecacheid, "sharecache=%s" }, { Opt_nosharecache, "nosharecache" }, { Opt_resvport, "resvport" }, { Opt_noresvport, "noresvport" }, @@ -665,6 +667,10 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss, else seq_puts(m, nfs_infop->nostr); } + if (!(nfss->flags & NFS_MOUNT_UNSHARED) && + nfss->cache_id) + seq_printf(m, ",sharecache=%u", nfss->cache_id); + rcu_read_lock(); seq_printf(m, ",proto=%s", rpc_peeraddr2str(nfss->client, RPC_DISPLAY_NETID)); @@ -1407,6 +1413,12 @@ static int nfs_parse_mount_options(char *raw, goto out_invalid_value; mnt->minorversion = option; break; + case Opt_sharecacheid: + if (nfs_get_option_ul(args, &option) || + option <= 0) + goto out_invalid_value; + mnt->cache_id = option; + break; /* * options that take text values @@ -2457,6 +2469,9 @@ static int nfs_compare_super(struct super_block *sb, void *data) /* Note: NFS_MOUNT_UNSHARED == NFS4_MOUNT_UNSHARED */ if (old->flags & NFS_MOUNT_UNSHARED) return 0; + if ((old->cache_id || server->cache_id) && + old->cache_id != server->cache_id) + return 0; if (memcmp(&old->fsid, &server->fsid, sizeof(old->fsid)) != 0) return 0; return nfs_compare_mount_options(sb, server, mntflags); diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h index 1150ea41b626..8e66d95283e1 100644 --- a/include/linux/nfs_fs_sb.h +++ b/include/linux/nfs_fs_sb.h @@ -145,6 +145,7 @@ struct nfs_server { #define NFS_OPTION_FSCACHE 0x00000001 /* - local caching enabled */ #define NFS_OPTION_MIGRATION 0x00000002 /* - NFSv4 migration enabled */ + unsigned int cache_id; /* For NFS_MOUNT_UNSHARED */ struct nfs_fsid fsid; __u64 maxfilesize; /* maximum file size */ struct timespec time_delta; /* smallest time granularity */
(I seem to have quite a pile of NFS issues lately.... some if it is tidying up issues from before Christmas, some of it just keeps on coming :-) If you run "mount -a" it will attempt to mount all filesystems listed in /etc/fstab, but filesystems that are already mounted will not be mounted again. So it is normally safe to run "mount -a" multiple times. However if an NFS mount in /etc/fstab has the "nosharecache" option set, mount doesn't notice that it is already mounted as so mounts it again. So repeated "mount -a" is no longer safe. This happens because the prevention of multiple mounts happens in do_add_mount in fs/namespace.c: err = -EBUSY; if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb && path->mnt->mnt_root == path->dentry) goto unlock; i.e. if the exact same superblock is being mounted in the exact same place, return EBUSY. With nosharecache, every mount attempt produces a new superblock so this test never fires. One way to address this would be to have a different option, e.g. sharecache=27 where the '27' is an arbitrary number meaning that if two mount attempts have different sharecache numbers they will have different superblocks. If they have the same sharecache number they can have the same superblock. This is not the most elegant interface ever and I would be very happy for suggestions to improve it. Maybe a string rather than a number ??? This probably isn't a very serious issue, but is a regression in terms of the usability of "mount -a" and I think it would be best to fix it if the cost is not too high. Below is my patch to implement the "sharecache=%u" syntax. Any ideas? Thanks, NeilBrown