[PATCH 0/4] Don't cache pNFS deviceids in violation of RFC5661 rules

[PATCH 0/4] Don't cache pNFS deviceids in violation of RFC5661 rules

[Trond Myklebust]

RFC5661 says that we can only cache a pNFS deviceid if at least one of 2
conditions is satisfied:

1) There are outstanding layouts that reference that stateid
2) The server set the NOTIFY_DEVICEID4_CHANGE and NOTIFY_DEVICEID4_DELETE
   in its reply to the GETDEVICEINFO call.

Trond Myklebust (4):
  NFSv4.1: Convert pNFS deviceid to use kfree_rcu()
  NFSv4.1: Cleanup - don't opencode nfs4_put_deviceid_node()
  NFSv4.1: Allow getdeviceinfo to return notification info back to
    caller
  NFSv4.1: Don't cache deviceids that have no notifications

 fs/nfs/blocklayout/dev.c                  |  2 +-
 fs/nfs/filelayout/filelayoutdev.c         |  2 +-
 fs/nfs/flexfilelayout/flexfilelayoutdev.c |  2 +-
 fs/nfs/nfs4proc.c                         |  7 +++++++
 fs/nfs/nfs4xdr.c                          | 14 +++++---------
 fs/nfs/objlayout/objio_osd.c              |  2 +-
 fs/nfs/pnfs.h                             |  3 +++
 fs/nfs/pnfs_dev.c                         | 21 +++++++++++++--------
 include/linux/nfs_xdr.h                   |  2 ++
 9 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.1.0

[PATCH 1/4] NFSv4.1: Convert pNFS deviceid to use kfree_rcu()

[Trond Myklebust]

Use of synchronize_rcu() when unmounting and potentially freeing a lot
of deviceids is problematic. There really is no reason why we can't just
use kfree_rcu() here.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/blocklayout/dev.c                  | 2 +-
 fs/nfs/filelayout/filelayoutdev.c         | 2 +-
 fs/nfs/flexfilelayout/flexfilelayoutdev.c | 2 +-
 fs/nfs/objlayout/objio_osd.c              | 2 +-
 fs/nfs/pnfs.h                             | 1 +
 fs/nfs/pnfs_dev.c                         | 6 ++----
 6 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
index 5aed4f98df41..e535599a0719 100644
--- a/fs/nfs/blocklayout/dev.c
+++ b/fs/nfs/blocklayout/dev.c
@@ -33,7 +33,7 @@ bl_free_deviceid_node(struct nfs4_deviceid_node *d)
 		container_of(d, struct pnfs_block_dev, node);
 
 	bl_free_device(dev);
-	kfree(dev);
+	kfree_rcu(dev, node.rcu);
 }
 
 static int
diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c
index 4f372e224603..4946ef40ba87 100644
--- a/fs/nfs/filelayout/filelayoutdev.c
+++ b/fs/nfs/filelayout/filelayoutdev.c
@@ -55,7 +55,7 @@ nfs4_fl_free_deviceid(struct nfs4_file_layout_dsaddr *dsaddr)
 			nfs4_pnfs_ds_put(ds);
 	}
 	kfree(dsaddr->stripe_indices);
-	kfree(dsaddr);
+	kfree_rcu(dsaddr, id_node.rcu);
 }
 
 /* Decode opaque device data and return the result */
diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
index e2c01f204a95..77a2d026aa12 100644
--- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
+++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
@@ -30,7 +30,7 @@ void nfs4_ff_layout_free_deviceid(struct nfs4_ff_layout_ds *mirror_ds)
 {
 	nfs4_print_deviceid(&mirror_ds->id_node.deviceid);
 	nfs4_pnfs_ds_put(mirror_ds->ds);
-	kfree(mirror_ds);
+	kfree_rcu(mirror_ds, id_node.rcu);
 }
 
 /* Decode opaque device data and construct new_ds using it */
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index 24e1d7403c0b..8b5e0e687d5e 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -57,7 +57,7 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
 
 	dprintk("%s: free od=%p\n", __func__, de->od.od);
 	osduld_put_device(de->od.od);
-	kfree(de);
+	kfree_rcu(d, rcu);
 }
 
 struct objio_segment {
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 635f0865671c..a1fc16c971a7 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -302,6 +302,7 @@ struct nfs4_deviceid_node {
 	unsigned long 			flags;
 	unsigned long			timestamp_unavailable;
 	struct nfs4_deviceid		deviceid;
+	struct rcu_head			rcu;
 	atomic_t			ref;
 };
 
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index aa2ec0015183..bf23ac97d57d 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -175,8 +175,8 @@ __nfs4_find_get_deviceid(struct nfs_server *server,
 	rcu_read_lock();
 	d = _lookup_deviceid(server->pnfs_curr_ld, server->nfs_client, id,
 			hash);
-	if (d != NULL)
-		atomic_inc(&d->ref);
+	if (d != NULL && !atomic_inc_not_zero(&d->ref))
+		d = NULL;
 	rcu_read_unlock();
 	return d;
 }
@@ -236,7 +236,6 @@ nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *ld,
 	}
 	hlist_del_init_rcu(&d->node);
 	spin_unlock(&nfs4_deviceid_lock);
-	synchronize_rcu();
 
 	/* balance the initial ref set in pnfs_insert_deviceid */
 	if (atomic_dec_and_test(&d->ref))
@@ -321,7 +320,6 @@ _deviceid_purge_client(const struct nfs_client *clp, long hash)
 	if (hlist_empty(&tmp))
 		return;
 
-	synchronize_rcu();
 	while (!hlist_empty(&tmp)) {
 		d = hlist_entry(tmp.first, struct nfs4_deviceid_node, tmpnode);
 		hlist_del(&d->tmpnode);
-- 
2.1.0

[PATCH 2/4] NFSv4.1: Cleanup - don't opencode nfs4_put_deviceid_node()

[Trond Myklebust]

There really is no reason to do so.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/pnfs_dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index bf23ac97d57d..7e07f4ba4822 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -238,8 +238,7 @@ nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *ld,
 	spin_unlock(&nfs4_deviceid_lock);
 
 	/* balance the initial ref set in pnfs_insert_deviceid */
-	if (atomic_dec_and_test(&d->ref))
-		d->ld->free_deviceid_node(d);
+	nfs4_put_deviceid_node(d);
 }
 EXPORT_SYMBOL_GPL(nfs4_delete_deviceid);
 
@@ -323,8 +322,7 @@ _deviceid_purge_client(const struct nfs_client *clp, long hash)
 	while (!hlist_empty(&tmp)) {
 		d = hlist_entry(tmp.first, struct nfs4_deviceid_node, tmpnode);
 		hlist_del(&d->tmpnode);
-		if (atomic_dec_and_test(&d->ref))
-			d->ld->free_deviceid_node(d);
+		nfs4_put_deviceid_node(d);
 	}
 }
 
-- 
2.1.0

[PATCH 3/4] NFSv4.1: Allow getdeviceinfo to return notification info back to caller

[Trond Myklebust]

We are only allowed to cache deviceinfo if the server supports notifications
and actually promises to call us back when changes occur. Right now, we
request those notifications, but then we don't check the server's reply.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c       |  5 +++++
 fs/nfs/nfs4xdr.c        | 14 +++++---------
 include/linux/nfs_xdr.h |  2 ++
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 627f37c44456..ba8b2b5e98a1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7944,6 +7944,8 @@ _nfs4_proc_getdeviceinfo(struct nfs_server *server,
 {
 	struct nfs4_getdeviceinfo_args args = {
 		.pdev = pdev,
+		.notify_types = NOTIFY_DEVICEID4_CHANGE |
+			NOTIFY_DEVICEID4_DELETE,
 	};
 	struct nfs4_getdeviceinfo_res res = {
 		.pdev = pdev,
@@ -7958,6 +7960,9 @@ _nfs4_proc_getdeviceinfo(struct nfs_server *server,
 
 	dprintk("--> %s\n", __func__);
 	status = nfs4_call_sync(server->client, server, &msg, &args.seq_args, &res.seq_res, 0);
+	if (res.notification & ~args.notify_types)
+		dprintk("%s: unsupported notification\n", __func__);
+
 	dprintk("<-- %s status=%d\n", __func__, status);
 
 	return status;
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5c399ec41079..6b28a605c697 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1920,7 +1920,7 @@ encode_getdeviceinfo(struct xdr_stream *xdr,
 
 	p = reserve_space(xdr, 4 + 4);
 	*p++ = cpu_to_be32(1);			/* bitmap length */
-	*p++ = cpu_to_be32(NOTIFY_DEVICEID4_CHANGE | NOTIFY_DEVICEID4_DELETE);
+	*p++ = cpu_to_be32(args->notify_types);
 }
 
 static void
@@ -5753,8 +5753,9 @@ out_overflow:
 
 #if defined(CONFIG_NFS_V4_1)
 static int decode_getdeviceinfo(struct xdr_stream *xdr,
-				struct pnfs_device *pdev)
+				struct nfs4_getdeviceinfo_res *res)
 {
+	struct pnfs_device *pdev = res->pdev;
 	__be32 *p;
 	uint32_t len, type;
 	int status;
@@ -5802,12 +5803,7 @@ static int decode_getdeviceinfo(struct xdr_stream *xdr,
 		if (unlikely(!p))
 			goto out_overflow;
 
-		if (be32_to_cpup(p++) &
-		    ~(NOTIFY_DEVICEID4_CHANGE | NOTIFY_DEVICEID4_DELETE)) {
-			dprintk("%s: unsupported notification\n",
-				__func__);
-		}
-
+		res->notification = be32_to_cpup(p++);
 		for (i = 1; i < len; i++) {
 			if (be32_to_cpup(p++)) {
 				dprintk("%s: unsupported notification\n",
@@ -7061,7 +7057,7 @@ static int nfs4_xdr_dec_getdeviceinfo(struct rpc_rqst *rqstp,
 	status = decode_sequence(xdr, &res->seq_res, rqstp);
 	if (status != 0)
 		goto out;
-	status = decode_getdeviceinfo(xdr, res->pdev);
+	status = decode_getdeviceinfo(xdr, res);
 out:
 	return status;
 }
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 4cb3eaa89cf7..3d88908fd140 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -255,11 +255,13 @@ struct nfs4_layoutget {
 struct nfs4_getdeviceinfo_args {
 	struct nfs4_sequence_args seq_args;
 	struct pnfs_device *pdev;
+	__u32 notify_types;
 };
 
 struct nfs4_getdeviceinfo_res {
 	struct nfs4_sequence_res seq_res;
 	struct pnfs_device *pdev;
+	__u32 notification;
 };
 
 struct nfs4_layoutcommit_args {
-- 
2.1.0

[PATCH 4/4] NFSv4.1: Don't cache deviceids that have no notifications

[Trond Myklebust]

The spec says that once all layouts that reference a given deviceid
have been returned, then we are only allowed to continue to cache
the deviceid if the metadata server supports notifications.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 2 ++
 fs/nfs/pnfs.h     | 2 ++
 fs/nfs/pnfs_dev.c | 9 +++++++++
 3 files changed, 13 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ba8b2b5e98a1..9ff8c63c9cac 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7962,6 +7962,8 @@ _nfs4_proc_getdeviceinfo(struct nfs_server *server,
 	status = nfs4_call_sync(server->client, server, &msg, &args.seq_args, &res.seq_res, 0);
 	if (res.notification & ~args.notify_types)
 		dprintk("%s: unsupported notification\n", __func__);
+	if (res.notification != args.notify_types)
+		pdev->nocache = 1;
 
 	dprintk("<-- %s status=%d\n", __func__, status);
 
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index a1fc16c971a7..b5654e8da936 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -203,6 +203,7 @@ struct pnfs_device {
 	struct page **pages;
 	unsigned int  pgbase;
 	unsigned int  pglen;	/* reply buffer length */
+	unsigned char nocache : 1;/* May not be cached */
 };
 
 #define NFS4_PNFS_GETDEVLIST_MAXNUM 16
@@ -291,6 +292,7 @@ void pnfs_error_mark_layout_for_return(struct inode *inode,
 enum {
 	NFS_DEVICEID_INVALID = 0,       /* set when MDS clientid recalled */
 	NFS_DEVICEID_UNAVAILABLE,	/* device temporarily unavailable */
+	NFS_DEVICEID_NOCACHE,		/* device may not be cached */
 };
 
 /* pnfs_dev.c */
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 7e07f4ba4822..2961fcd7a2df 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -149,6 +149,8 @@ nfs4_get_device_info(struct nfs_server *server,
 	 */
 	d = server->pnfs_curr_ld->alloc_deviceid_node(server, pdev,
 			gfp_flags);
+	if (d && pdev->nocache)
+		set_bit(NFS_DEVICEID_NOCACHE, &d->flags);
 
 out_free_pages:
 	for (i = 0; i < max_pages; i++)
@@ -235,6 +237,7 @@ nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *ld,
 		return;
 	}
 	hlist_del_init_rcu(&d->node);
+	clear_bit(NFS_DEVICEID_NOCACHE, &d->flags);
 	spin_unlock(&nfs4_deviceid_lock);
 
 	/* balance the initial ref set in pnfs_insert_deviceid */
@@ -269,6 +272,11 @@ EXPORT_SYMBOL_GPL(nfs4_init_deviceid_node);
 bool
 nfs4_put_deviceid_node(struct nfs4_deviceid_node *d)
 {
+	if (test_bit(NFS_DEVICEID_NOCACHE, &d->flags)) {
+		if (atomic_add_unless(&d->ref, -1, 2))
+			return false;
+		nfs4_delete_deviceid(d->ld, d->nfs_client, &d->deviceid);
+	}
 	if (!atomic_dec_and_test(&d->ref))
 		return false;
 	d->ld->free_deviceid_node(d);
@@ -312,6 +320,7 @@ _deviceid_purge_client(const struct nfs_client *clp, long hash)
 		if (d->nfs_client == clp && atomic_read(&d->ref)) {
 			hlist_del_init_rcu(&d->node);
 			hlist_add_head(&d->tmpnode, &tmp);
+			clear_bit(NFS_DEVICEID_NOCACHE, &d->flags);
 		}
 	rcu_read_unlock();
 	spin_unlock(&nfs4_deviceid_lock);
-- 
2.1.0

Re: [PATCH 4/4] NFSv4.1: Don't cache deviceids that have no notifications

[Christoph Hellwig]

On Mon, Mar 09, 2015 at 04:05:03PM -0400, Trond Myklebust wrote:
> The spec says that once all layouts that reference a given deviceid
> have been returned, then we are only allowed to continue to cache
> the deviceid if the metadata server supports notifications.

This causes massive performance issues for object and block layout
servers where a GETDEVICEINFO (or rather the client processing of it)
is expensive.  Also it increases the deadlock potential as the
GETDEVICEINFO generally isn't safe for writeback under memory pressure
(and yes, we'll need more fixes in that area).

I've also filed an errata a while ago to update the language in the spec
in this area to be consistent and not enforce this behavior:

http://www.rfc-editor.org/errata_search.php?rfc=5661&eid=4119

I think the right fix is to have a shrinker that allows the nfs client
to retire unused devices on a lru basis under memory pressure.

Re: [PATCH 4/4] NFSv4.1: Don't cache deviceids that have no notifications

[Trond Myklebust]

On Tue, Mar 17, 2015 at 6:37 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Mar 09, 2015 at 04:05:03PM -0400, Trond Myklebust wrote:
>> The spec says that once all layouts that reference a given deviceid
>> have been returned, then we are only allowed to continue to cache
>> the deviceid if the metadata server supports notifications.
>
> This causes massive performance issues for object and block layout
> servers where a GETDEVICEINFO (or rather the client processing of it)
> is expensive.  Also it increases the deadlock potential as the
> GETDEVICEINFO generally isn't safe for writeback under memory pressure
> (and yes, we'll need more fixes in that area).
>
> I've also filed an errata a while ago to update the language in the spec
> in this area to be consistent and not enforce this behavior:
>
> http://www.rfc-editor.org/errata_search.php?rfc=5661&eid=4119
>
> I think the right fix is to have a shrinker that allows the nfs client
> to retire unused devices on a lru basis under memory pressure.

No. The right fix is to make the server support device notifications.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [PATCH 4/4] NFSv4.1: Don't cache deviceids that have no notifications

[Christoph Hellwig]

On Sun, Mar 22, 2015 at 03:27:32PM -0400, Trond Myklebust wrote:
> No. The right fix is to make the server support device notifications.

And you instantly thrash performane on any server that doesn't, which
is why I sent out the errata that no one objected out.  I'm happy to
open this again on the WG list, but the notification scheme is such a
big hammer that it absolutely isn't worth implementing.  Given how
GETDEVICELIST is specified in 4.1 the original RFC language also is
inconsistent with itself, so that's not an argument for the old
behavior.