[PATCH] nfs: add minor version to nfs_server_key for fscache

[PATCH] nfs: add minor version to nfs_server_key for fscache

[Scott Mayhew]

If the same NFS filesystem is mounted multiple times with "-o fsc" and
different NFS versions, the following oops can occur.

Fix it by adding the minor version to the struct nfs_server_key.

Note this requires waiting to call nfs_fscache_get_client_cookie()
until after the nfs_client has been initialized.

kernel BUG at fs/cachefiles/namei.c:194!
invalid opcode: 0000 [#1] SMP PTI
Modules linked in: ...
CPU: 1 PID: 6 Comm: kworker/u4:0 Kdump: loaded Not tainted 4.17.3-200.fc28.x86_64 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
Workqueue: fscache_object fscache_object_work_func [fscache]
RIP: 0010:cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles]
RSP: 0000:ffffa37f40347d50 EFLAGS: 00010246
RAX: 0000000000000001 RBX: ffff9327f3e94300 RCX: 0000000000000006
RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffff9327ffd16930
RBP: ffff9327f3e94000 R08: 0000000000000000 R09: 000000000000026f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff9327faa59840
R13: ffff9327faa59840 R14: ffff9327f3e94440 R15: ffff9327f3e94ec0
FS:  0000000000000000(0000) GS:ffff9327ffd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f63f1db28a0 CR3: 000000002020a001 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 ? __queue_work+0x103/0x3e0
 ? __switch_to_asm+0x34/0x70
 cachefiles_lookup_object+0x4b/0xa0 [cachefiles]
 fscache_look_up_object+0x9c/0x110 [fscache]
 ? fscache_parent_ready+0x2a/0x50 [fscache]
 fscache_object_work_func+0x74/0x2b0 [fscache]
 process_one_work+0x187/0x340
 worker_thread+0x2e/0x380
 ? pwq_unbound_release_workfn+0xd0/0xd0
 kthread+0x112/0x130
 ? kthread_create_worker_on_cpu+0x70/0x70
 ret_from_fork+0x35/0x40
Code: ...
RIP: cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles] RSP: ffffa37f40347d50

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/client.c  | 8 +++++---
 fs/nfs/fscache.c | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 377a61654a88..bfd1b4e2852b 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -185,7 +185,6 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
 	cred = rpc_lookup_machine_cred("*");
 	if (!IS_ERR(cred))
 		clp->cl_machine_cred = cred;
-	nfs_fscache_get_client_cookie(clp);
 
 	return clp;
 
@@ -397,7 +396,7 @@ nfs_found_client(const struct nfs_client_initdata *cl_init,
  */
 struct nfs_client *nfs_get_client(const struct nfs_client_initdata *cl_init)
 {
-	struct nfs_client *clp, *new = NULL;
+	struct nfs_client *clp, *ret, *new = NULL;
 	struct nfs_net *nn = net_generic(cl_init->net, nfs_net_id);
 	const struct nfs_rpc_ops *rpc_ops = cl_init->nfs_mod->rpc_ops;
 
@@ -422,7 +421,10 @@ struct nfs_client *nfs_get_client(const struct nfs_client_initdata *cl_init)
 					&nn->nfs_client_list);
 			spin_unlock(&nn->nfs_client_lock);
 			new->cl_flags = cl_init->init_flags;
-			return rpc_ops->init_client(new, cl_init);
+			ret = rpc_ops->init_client(new, cl_init);
+			if (!IS_ERR(ret))
+				nfs_fscache_get_client_cookie(new);
+			return ret;
 		}
 
 		spin_unlock(&nn->nfs_client_lock);
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 4dc887813c71..c32146319944 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -35,6 +35,7 @@ static DEFINE_SPINLOCK(nfs_fscache_keys_lock);
 struct nfs_server_key {
 	struct {
 		uint16_t	nfsversion;		/* NFS protocol version */
+		uint16_t	minorversion;		/* NFSv4 minor version */
 		uint16_t	family;			/* address family */
 		__be16		port;			/* IP port */
 	} hdr;
@@ -59,6 +60,7 @@ void nfs_fscache_get_client_cookie(struct nfs_client *clp)
 
 	memset(&key, 0, sizeof(key));
 	key.hdr.nfsversion = clp->rpc_ops->version;
+	key.hdr.minorversion = clp->cl_minorversion;
 	key.hdr.family = clp->cl_addr.ss_family;
 
 	switch (clp->cl_addr.ss_family) {
-- 
2.14.4

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[Benjamin Coddington]

So there's two changes here, the first is that we don't call
nfs_fscache_get_client_cookie() if rpc_ops->init_client fails, which 
makes
sense.  The second is that we create separate fscache indexes for minor
versions.  Why do we need separate caches if the nfs_client is the same? 
  I
thought that the nfs_client is just there to have something to hang the
superblock caches from..

Is the problem that we can take down one nfs_client and then initialize 
a
new one, but the nfs_server_key is exactly the same, so now we start to 
add
new objects before fscache has cleaned up old references?

Ben

On 11 Jul 2018, at 8:22, Scott Mayhew wrote:

> If the same NFS filesystem is mounted multiple times with "-o fsc" and
> different NFS versions, the following oops can occur.
>
> Fix it by adding the minor version to the struct nfs_server_key.
>
> Note this requires waiting to call nfs_fscache_get_client_cookie()
> until after the nfs_client has been initialized.
>
> kernel BUG at fs/cachefiles/namei.c:194!
> invalid opcode: 0000 [#1] SMP PTI
> Modules linked in: ...
> CPU: 1 PID: 6 Comm: kworker/u4:0 Kdump: loaded Not tainted 
> 4.17.3-200.fc28.x86_64 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 1.10.2-2.fc27 04/01/2014
> Workqueue: fscache_object fscache_object_work_func [fscache]
> RIP: 0010:cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles]
> RSP: 0000:ffffa37f40347d50 EFLAGS: 00010246
> RAX: 0000000000000001 RBX: ffff9327f3e94300 RCX: 0000000000000006
> RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffff9327ffd16930
> RBP: ffff9327f3e94000 R08: 0000000000000000 R09: 000000000000026f
> R10: 0000000000000000 R11: 0000000000000001 R12: ffff9327faa59840
> R13: ffff9327faa59840 R14: ffff9327f3e94440 R15: ffff9327f3e94ec0
> FS:  0000000000000000(0000) GS:ffff9327ffd00000(0000) 
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f63f1db28a0 CR3: 000000002020a001 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  ? __queue_work+0x103/0x3e0
>  ? __switch_to_asm+0x34/0x70
>  cachefiles_lookup_object+0x4b/0xa0 [cachefiles]
>  fscache_look_up_object+0x9c/0x110 [fscache]
>  ? fscache_parent_ready+0x2a/0x50 [fscache]
>  fscache_object_work_func+0x74/0x2b0 [fscache]
>  process_one_work+0x187/0x340
>  worker_thread+0x2e/0x380
>  ? pwq_unbound_release_workfn+0xd0/0xd0
>  kthread+0x112/0x130
>  ? kthread_create_worker_on_cpu+0x70/0x70
>  ret_from_fork+0x35/0x40
> Code: ...
> RIP: cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles] RSP: 
> ffffa37f40347d50
>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfs/client.c  | 8 +++++---
>  fs/nfs/fscache.c | 2 ++
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 377a61654a88..bfd1b4e2852b 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -185,7 +185,6 @@ struct nfs_client *nfs_alloc_client(const struct 
> nfs_client_initdata *cl_init)
>  	cred = rpc_lookup_machine_cred("*");
>  	if (!IS_ERR(cred))
>  		clp->cl_machine_cred = cred;
> -	nfs_fscache_get_client_cookie(clp);
>
>  	return clp;
>
> @@ -397,7 +396,7 @@ nfs_found_client(const struct nfs_client_initdata 
> *cl_init,
>   */
>  struct nfs_client *nfs_get_client(const struct nfs_client_initdata 
> *cl_init)
>  {
> -	struct nfs_client *clp, *new = NULL;
> +	struct nfs_client *clp, *ret, *new = NULL;
>  	struct nfs_net *nn = net_generic(cl_init->net, nfs_net_id);
>  	const struct nfs_rpc_ops *rpc_ops = cl_init->nfs_mod->rpc_ops;
>
> @@ -422,7 +421,10 @@ struct nfs_client *nfs_get_client(const struct 
> nfs_client_initdata *cl_init)
>  					&nn->nfs_client_list);
>  			spin_unlock(&nn->nfs_client_lock);
>  			new->cl_flags = cl_init->init_flags;
> -			return rpc_ops->init_client(new, cl_init);
> +			ret = rpc_ops->init_client(new, cl_init);
> +			if (!IS_ERR(ret))
> +				nfs_fscache_get_client_cookie(new);
> +			return ret;
>  		}
>
>  		spin_unlock(&nn->nfs_client_lock);
> diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> index 4dc887813c71..c32146319944 100644
> --- a/fs/nfs/fscache.c
> +++ b/fs/nfs/fscache.c
> @@ -35,6 +35,7 @@ static DEFINE_SPINLOCK(nfs_fscache_keys_lock);
>  struct nfs_server_key {
>  	struct {
>  		uint16_t	nfsversion;		/* NFS protocol version */
> +		uint16_t	minorversion;		/* NFSv4 minor version */
>  		uint16_t	family;			/* address family */
>  		__be16		port;			/* IP port */
>  	} hdr;
> @@ -59,6 +60,7 @@ void nfs_fscache_get_client_cookie(struct nfs_client 
> *clp)
>
>  	memset(&key, 0, sizeof(key));
>  	key.hdr.nfsversion = clp->rpc_ops->version;
> +	key.hdr.minorversion = clp->cl_minorversion;
>  	key.hdr.family = clp->cl_addr.ss_family;
>
>  	switch (clp->cl_addr.ss_family) {
> -- 
> 2.14.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[Scott Mayhew]

On Wed, 11 Jul 2018, Benjamin Coddington wrote:

> So there's two changes here, the first is that we don't call
> nfs_fscache_get_client_cookie() if rpc_ops->init_client fails, which makes
> sense.  The second is that we create separate fscache indexes for minor
> versions.  Why do we need separate caches if the nfs_client is the same?  I
> thought that the nfs_client is just there to have something to hang the
> superblock caches from..
> 
> Is the problem that we can take down one nfs_client and then initialize a
> new one, but the nfs_server_key is exactly the same, so now we start to add
> new objects before fscache has cleaned up old references?

The nfs_clients aren't the same, and nothing's being torn down.

I'm using steved's "runcthon" script from cthon which performs
multiple cthon runs simultaneiously.  I have different superblocks,
nfs_servers, nfs_clients, and fscache_cookies.  But the keys for the
v4.x mounts are all the same.  David indicated that was problematic, so
I suggested adding the minor version to the key, which he said was fine.

crash> mount|grep nfs
ffff9fb43b544900 ffff9fb3f5cb2000 rpc_pipefs sunrpc
/var/lib/nfs/rpc_pipefs
ffff9fb42b9b0d80 ffff9fb424c34000 nfs    192.168.122.3:/export/nfsv3tcp
/mnt/nfsv3tcp
ffff9fb42b9b2d80 ffff9fb3f5cb2800 nfs4   192.168.122.3:/export/nfsv4tcp
/mnt/nfsv4tcp
ffff9fb42b9cfc80 ffff9fb42b9a5000 nfs4   192.168.122.3:/export/nfsv41tcp
/mnt/nfsv41tcp
ffff9fb42b9b2180 ffff9fb42aaaa000 nfs4   192.168.122.3:/export/nfsv42tcp
/mnt/nfsv42tcp
crash> super_block.s_fs_info ffff9fb424c34000
  s_fs_info = 0xffff9fb424f11000
crash> super_block.s_fs_info ffff9fb3f5cb2800
  s_fs_info = 0xffff9fb4397d3400
crash> super_block.s_fs_info ffff9fb42b9a5000
  s_fs_info = 0xffff9fb3f676c000
crash> super_block.s_fs_info ffff9fb42aaaa000
  s_fs_info = 0xffff9fb4397d1400
crash> nfs_server.nfs_client 0xffff9fb424f11000
  nfs_client = 0xffff9fb424f10800
crash> nfs_server.nfs_client 0xffff9fb4397d3400
  nfs_client = 0xffff9fb438a36000
crash> nfs_server.nfs_client 0xffff9fb3f676c000
  nfs_client = 0xffff9fb424f13800
crash> nfs_server.nfs_client 0xffff9fb4397d1400
  nfs_client = 0xffff9fb43afb2800
crash> nfs_client.fscache 0xffff9fb424f10800
  fscache = 0xffff9fb43904c220
crash> nfs_client.fscache 0xffff9fb438a36000
  fscache = 0xffff9fb424f78a18
crash> nfs_client.fscache 0xffff9fb424f13800
  fscache = 0xffff9fb43904c330
crash> nfs_client.fscache 0xffff9fb43afb2800
  fscache = 0xffff9fb424f78cc0
crash> fscache_cookie.key 0xffff9fb43904c220
    key = 0xa8c0000000020003
crash> fscache_cookie.key 0xffff9fb424f78a18
    key = 0xa8c0010800020004 <-----------------+
crash> fscache_cookie.key 0xffff9fb43904c330   |
    key = 0xa8c0010800020004 <-----------------+---- duplicate keys
crash> fscache_cookie.key 0xffff9fb424f78cc0   |
    key = 0xa8c0010800020004 <-----------------+

> 
> Ben
> 
> On 11 Jul 2018, at 8:22, Scott Mayhew wrote:
> 
> > If the same NFS filesystem is mounted multiple times with "-o fsc" and
> > different NFS versions, the following oops can occur.
> > 
> > Fix it by adding the minor version to the struct nfs_server_key.
> > 
> > Note this requires waiting to call nfs_fscache_get_client_cookie()
> > until after the nfs_client has been initialized.
> > 
> > kernel BUG at fs/cachefiles/namei.c:194!
> > invalid opcode: 0000 [#1] SMP PTI
> > Modules linked in: ...
> > CPU: 1 PID: 6 Comm: kworker/u4:0 Kdump: loaded Not tainted
> > 4.17.3-200.fc28.x86_64 #1
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > 1.10.2-2.fc27 04/01/2014
> > Workqueue: fscache_object fscache_object_work_func [fscache]
> > RIP: 0010:cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles]
> > RSP: 0000:ffffa37f40347d50 EFLAGS: 00010246
> > RAX: 0000000000000001 RBX: ffff9327f3e94300 RCX: 0000000000000006
> > RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffff9327ffd16930
> > RBP: ffff9327f3e94000 R08: 0000000000000000 R09: 000000000000026f
> > R10: 0000000000000000 R11: 0000000000000001 R12: ffff9327faa59840
> > R13: ffff9327faa59840 R14: ffff9327f3e94440 R15: ffff9327f3e94ec0
> > FS:  0000000000000000(0000) GS:ffff9327ffd00000(0000)
> > knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f63f1db28a0 CR3: 000000002020a001 CR4: 00000000003606e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  ? __queue_work+0x103/0x3e0
> >  ? __switch_to_asm+0x34/0x70
> >  cachefiles_lookup_object+0x4b/0xa0 [cachefiles]
> >  fscache_look_up_object+0x9c/0x110 [fscache]
> >  ? fscache_parent_ready+0x2a/0x50 [fscache]
> >  fscache_object_work_func+0x74/0x2b0 [fscache]
> >  process_one_work+0x187/0x340
> >  worker_thread+0x2e/0x380
> >  ? pwq_unbound_release_workfn+0xd0/0xd0
> >  kthread+0x112/0x130
> >  ? kthread_create_worker_on_cpu+0x70/0x70
> >  ret_from_fork+0x35/0x40
> > Code: ...
> > RIP: cachefiles_walk_to_object.cold.16+0x124/0x18c [cachefiles] RSP:
> > ffffa37f40347d50
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfs/client.c  | 8 +++++---
> >  fs/nfs/fscache.c | 2 ++
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 377a61654a88..bfd1b4e2852b 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -185,7 +185,6 @@ struct nfs_client *nfs_alloc_client(const struct
> > nfs_client_initdata *cl_init)
> >  	cred = rpc_lookup_machine_cred("*");
> >  	if (!IS_ERR(cred))
> >  		clp->cl_machine_cred = cred;
> > -	nfs_fscache_get_client_cookie(clp);
> > 
> >  	return clp;
> > 
> > @@ -397,7 +396,7 @@ nfs_found_client(const struct nfs_client_initdata
> > *cl_init,
> >   */
> >  struct nfs_client *nfs_get_client(const struct nfs_client_initdata
> > *cl_init)
> >  {
> > -	struct nfs_client *clp, *new = NULL;
> > +	struct nfs_client *clp, *ret, *new = NULL;
> >  	struct nfs_net *nn = net_generic(cl_init->net, nfs_net_id);
> >  	const struct nfs_rpc_ops *rpc_ops = cl_init->nfs_mod->rpc_ops;
> > 
> > @@ -422,7 +421,10 @@ struct nfs_client *nfs_get_client(const struct
> > nfs_client_initdata *cl_init)
> >  					&nn->nfs_client_list);
> >  			spin_unlock(&nn->nfs_client_lock);
> >  			new->cl_flags = cl_init->init_flags;
> > -			return rpc_ops->init_client(new, cl_init);
> > +			ret = rpc_ops->init_client(new, cl_init);
> > +			if (!IS_ERR(ret))
> > +				nfs_fscache_get_client_cookie(new);
> > +			return ret;
> >  		}
> > 
> >  		spin_unlock(&nn->nfs_client_lock);
> > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> > index 4dc887813c71..c32146319944 100644
> > --- a/fs/nfs/fscache.c
> > +++ b/fs/nfs/fscache.c
> > @@ -35,6 +35,7 @@ static DEFINE_SPINLOCK(nfs_fscache_keys_lock);
> >  struct nfs_server_key {
> >  	struct {
> >  		uint16_t	nfsversion;		/* NFS protocol version */
> > +		uint16_t	minorversion;		/* NFSv4 minor version */
> >  		uint16_t	family;			/* address family */
> >  		__be16		port;			/* IP port */
> >  	} hdr;
> > @@ -59,6 +60,7 @@ void nfs_fscache_get_client_cookie(struct nfs_client
> > *clp)
> > 
> >  	memset(&key, 0, sizeof(key));
> >  	key.hdr.nfsversion = clp->rpc_ops->version;
> > +	key.hdr.minorversion = clp->cl_minorversion;
> >  	key.hdr.family = clp->cl_addr.ss_family;
> > 
> >  	switch (clp->cl_addr.ss_family) {
> > -- 
> > 2.14.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[Trond Myklebust]

T24gV2VkLCAyMDE4LTA3LTExIGF0IDEyOjI0IC0wNDAwLCBTY290dCBNYXloZXcgd3JvdGU6DQo+
IE9uIFdlZCwgMTEgSnVsIDIwMTgsIEJlbmphbWluIENvZGRpbmd0b24gd3JvdGU6DQo+IA0KPiA+
IFNvIHRoZXJlJ3MgdHdvIGNoYW5nZXMgaGVyZSwgdGhlIGZpcnN0IGlzIHRoYXQgd2UgZG9uJ3Qg
Y2FsbA0KPiA+IG5mc19mc2NhY2hlX2dldF9jbGllbnRfY29va2llKCkgaWYgcnBjX29wcy0+aW5p
dF9jbGllbnQgZmFpbHMsDQo+ID4gd2hpY2ggbWFrZXMNCj4gPiBzZW5zZS4gIFRoZSBzZWNvbmQg
aXMgdGhhdCB3ZSBjcmVhdGUgc2VwYXJhdGUgZnNjYWNoZSBpbmRleGVzIGZvcg0KPiA+IG1pbm9y
DQo+ID4gdmVyc2lvbnMuICBXaHkgZG8gd2UgbmVlZCBzZXBhcmF0ZSBjYWNoZXMgaWYgdGhlIG5m
c19jbGllbnQgaXMgdGhlDQo+ID4gc2FtZT8gIEkNCj4gPiB0aG91Z2h0IHRoYXQgdGhlIG5mc19j
bGllbnQgaXMganVzdCB0aGVyZSB0byBoYXZlIHNvbWV0aGluZyB0byBoYW5nDQo+ID4gdGhlDQo+
ID4gc3VwZXJibG9jayBjYWNoZXMgZnJvbS4uDQo+ID4gDQo+ID4gSXMgdGhlIHByb2JsZW0gdGhh
dCB3ZSBjYW4gdGFrZSBkb3duIG9uZSBuZnNfY2xpZW50IGFuZCB0aGVuDQo+ID4gaW5pdGlhbGl6
ZSBhDQo+ID4gbmV3IG9uZSwgYnV0IHRoZSBuZnNfc2VydmVyX2tleSBpcyBleGFjdGx5IHRoZSBz
YW1lLCBzbyBub3cgd2UNCj4gPiBzdGFydCB0byBhZGQNCj4gPiBuZXcgb2JqZWN0cyBiZWZvcmUg
ZnNjYWNoZSBoYXMgY2xlYW5lZCB1cCBvbGQgcmVmZXJlbmNlcz8NCj4gDQo+IFRoZSBuZnNfY2xp
ZW50cyBhcmVuJ3QgdGhlIHNhbWUsIGFuZCBub3RoaW5nJ3MgYmVpbmcgdG9ybiBkb3duLg0KPiAN
Cj4gSSdtIHVzaW5nIHN0ZXZlZCdzICJydW5jdGhvbiIgc2NyaXB0IGZyb20gY3Rob24gd2hpY2gg
cGVyZm9ybXMNCj4gbXVsdGlwbGUgY3Rob24gcnVucyBzaW11bHRhbmVpb3VzbHkuICBJIGhhdmUg
ZGlmZmVyZW50IHN1cGVyYmxvY2tzLA0KPiBuZnNfc2VydmVycywgbmZzX2NsaWVudHMsIGFuZCBm
c2NhY2hlX2Nvb2tpZXMuICBCdXQgdGhlIGtleXMgZm9yIHRoZQ0KPiB2NC54IG1vdW50cyBhcmUg
YWxsIHRoZSBzYW1lLiAgRGF2aWQgaW5kaWNhdGVkIHRoYXQgd2FzIHByb2JsZW1hdGljLA0KPiBz
bw0KPiBJIHN1Z2dlc3RlZCBhZGRpbmcgdGhlIG1pbm9yIHZlcnNpb24gdG8gdGhlIGtleSwgd2hp
Y2ggaGUgc2FpZCB3YXMNCj4gZmluZS4NCg0KU28gd2hhdCBpcyB0aGUgdXNlIGNhc2UgZm9yIGhh
dmluZyBhIE5GU3Y0IGFuZCBORlN2NC4xIG1vdW50IG9mIHRoZQ0Kc2FtZSBmaWxlc3lzdGVtPyBJ
IGFncmVlIHdlIHNob3VsZG4ndCBjcmFzaCwgYnV0IEknbSBsb3N0IGFzIHRvIHdoeQ0Kc29tZW9u
ZSB3b3VsZCB3YW50IHRvIGRvIHRoaXMuDQoNCklPVzogV2h5IGlzbid0IHRoZSByaWdodCB0aGlu
ZyB0byBkbyBoZXJlIGp1c3QgdG8gcmVtb3ZlIHRoZSBib2d1cw0KQlVHX09OKCk/DQoNCi0tIA0K
VHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIEhhbW1lcnNwYWNl
DQp0cm9uZC5teWtsZWJ1c3RAaGFtbWVyc3BhY2UuY29tDQoNCg==

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[Scott Mayhew]

On Wed, 11 Jul 2018, Trond Myklebust wrote:

> On Wed, 2018-07-11 at 12:24 -0400, Scott Mayhew wrote:
> > On Wed, 11 Jul 2018, Benjamin Coddington wrote:
> > 
> > > So there's two changes here, the first is that we don't call
> > > nfs_fscache_get_client_cookie() if rpc_ops->init_client fails,
> > > which makes
> > > sense.  The second is that we create separate fscache indexes for
> > > minor
> > > versions.  Why do we need separate caches if the nfs_client is the
> > > same?  I
> > > thought that the nfs_client is just there to have something to hang
> > > the
> > > superblock caches from..
> > > 
> > > Is the problem that we can take down one nfs_client and then
> > > initialize a
> > > new one, but the nfs_server_key is exactly the same, so now we
> > > start to add
> > > new objects before fscache has cleaned up old references?
> > 
> > The nfs_clients aren't the same, and nothing's being torn down.
> > 
> > I'm using steved's "runcthon" script from cthon which performs
> > multiple cthon runs simultaneiously.  I have different superblocks,
> > nfs_servers, nfs_clients, and fscache_cookies.  But the keys for the
> > v4.x mounts are all the same.  David indicated that was problematic,
> > so
> > I suggested adding the minor version to the key, which he said was
> > fine.
> 
> So what is the use case for having a NFSv4 and NFSv4.1 mount of the
> same filesystem? I agree we shouldn't crash, but I'm lost as to why
> someone would want to do this.
> 
> IOW: Why isn't the right thing to do here just to remove the bogus
> BUG_ON()?
>
The bug was originally filed by steved a few years ago and that was his
reproducer.  Looking at some of the customer cases attached to the bug
now, it looks like they're not using different NFS versions... in which
case the patch wouldn't help.  Let me see if there are any vmcores still
available from those cases.

-Scott 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[David Howells]

Scott Mayhew <smayhew@redhat.com> wrote:

> kernel BUG at fs/cachefiles/namei.c:194!

Note that the cachefiles dumps a load of info immediately prior to this BUG()
which should be included in the description.

Can you also try the four patches at the top of this branch from Kiran Kumar
Modukuri?

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-fixes

The topmost patch removes the BUG() and lets it fall through to the wait.  The
"Fix missing clear of the CACHEFILES_OBJECT_ACTIVE flag" patch fixes one of
the causes of the BUG.

David

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[Scott Mayhew]

On Fri, 13 Jul 2018, David Howells wrote:

> Scott Mayhew <smayhew@redhat.com> wrote:
> 
> > kernel BUG at fs/cachefiles/namei.c:194!
> 
> Note that the cachefiles dumps a load of info immediately prior to this BUG()
> which should be included in the description.
> 
> Can you also try the four patches at the top of this branch from Kiran Kumar
> Modukuri?
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-fixes
> 
> The topmost patch removes the BUG() and lets it fall through to the wait.  The
> "Fix missing clear of the CACHEFILES_OBJECT_ACTIVE flag" patch fixes one of
> the causes of the BUG.

With v4.18-rc5 plus those four patches, I no longer get the "Unexpected
object collision" message when running cthon (and no oops either,
obviously).

-Scott
> 
> David

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[David Howells]

Scott Mayhew <smayhew@redhat.com> wrote:

> With v4.18-rc5 plus those four patches, I no longer get the "Unexpected
> object collision" message when running cthon (and no oops either,
> obviously).

Thanks!

David

Re: [PATCH] nfs: add minor version to nfs_server_key for fscache

[David Howells]

Trond Myklebust <trondmy@hammerspace.com> wrote:

> So what is the use case for having a NFSv4 and NFSv4.1 mount of the
> same filesystem? I agree we shouldn't crash, but I'm lost as to why
> someone would want to do this.

Note that one thing I'm thinking of adding to the mount overhaul is the
ability for the VFS core to either upcall or open a config file to find
default parameters for a new mount.  This would make it easier to configure
network and protocol parameters across automounts.

> IOW: Why isn't the right thing to do here just to remove the bogus
> BUG_ON()?

Because it's not exactly bogus - or, rather, it should now be redundant with
the upper layer (fscache) weeding out collisions between live cookies - but
that will still throw a warning that you're getting a collision, say between
an nfs-4.1 and a nfs-4.2 mount.

The problem is that fscache isn't equipped to deal with handling multiple
users of the same cache object and the cache coherence implications there of.
I'm looking to change the former by changing the data I/O interface to take an
iterator and stop it from tracking netfs pages beyond that, but managing cache
coherence has to be up to the network filesystem.

David

[PATCH] nfs: add minor version to nfs_server_key for fscache

[Dave Wysochanski]

From: Scott Mayhew <smayhew@redhat.com>

An NFS client that mounts multiple exports from the same NFS
server with higher NFSv4 versions disabled (i.e. 4.2) and without
forcing a specific NFS version results in fscache index cookie
collisions and the following messages:
[  570.004348] FS-Cache: Duplicate cookie detected

Each nfs_client structure should have its own fscache index cookie,
so add the minorversion to nfs_server_key.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=200145
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/client.c     | 1 +
 fs/nfs/fscache.c    | 2 ++
 fs/nfs/nfs4client.c | 1 -
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 989c30c98511..f1ff3076e4a4 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -153,6 +153,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
 	if ((clp = kzalloc(sizeof(*clp), GFP_KERNEL)) == NULL)
 		goto error_0;
 
+	clp->cl_minorversion = cl_init->minorversion;
 	clp->cl_nfs_mod = cl_init->nfs_mod;
 	if (!try_module_get(clp->cl_nfs_mod->owner))
 		goto error_dealloc;
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 52270bfac120..1abf126c2df4 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -31,6 +31,7 @@ static DEFINE_SPINLOCK(nfs_fscache_keys_lock);
 struct nfs_server_key {
 	struct {
 		uint16_t	nfsversion;		/* NFS protocol version */
+		uint32_t	minorversion;		/* NFSv4 minor version */
 		uint16_t	family;			/* address family */
 		__be16		port;			/* IP port */
 	} hdr;
@@ -55,6 +56,7 @@ void nfs_fscache_get_client_cookie(struct nfs_client *clp)
 
 	memset(&key, 0, sizeof(key));
 	key.hdr.nfsversion = clp->rpc_ops->version;
+	key.hdr.minorversion = clp->cl_minorversion;
 	key.hdr.family = clp->cl_addr.ss_family;
 
 	switch (clp->cl_addr.ss_family) {
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 0cd767e5c977..0bd77cc1f639 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -216,7 +216,6 @@ struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
 	INIT_LIST_HEAD(&clp->cl_ds_clients);
 	rpc_init_wait_queue(&clp->cl_rpcwaitq, "NFS client");
 	clp->cl_state = 1 << NFS4CLNT_LEASE_EXPIRED;
-	clp->cl_minorversion = cl_init->minorversion;
 	clp->cl_mvops = nfs_v4_minor_ops[cl_init->minorversion];
 	clp->cl_mig_gen = 1;
 #if IS_ENABLED(CONFIG_NFS_V4_1)
-- 
2.24.1