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

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

[Trond Myklebust]

On Wed, Mar 25, 2015 at 4:55 AM, Christoph Hellwig <hch@infradead.org> wrote:
> 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.

The reason for the GETDEVICEINFO description in RFC5661 is that you
need some mechanism to allow clients to know when a deviceid is
retired. There is no reasonable alternative to notification that
doesn't cause problems on either the client or the server.

That said, do note that one valid solution here is to have deviceids
that never expire (i.e. if you need to retire them, then you allocate
a new one). Given that the deviceid is a 128-bit field, such a
solution is 100% practical. That would allow you to say you support
notification, but not have to implement it.

Trond

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

[Christoph Hellwig]

On Wed, Mar 25, 2015 at 08:49:19AM -0400, Trond Myklebust wrote:
> The reason for the GETDEVICEINFO description in RFC5661 is that you
> need some mechanism to allow clients to know when a deviceid is
> retired. There is no reasonable alternative to notification that
> doesn't cause problems on either the client or the server.
> 
> That said, do note that one valid solution here is to have deviceids
> that never expire (i.e. if you need to retire them, then you allocate
> a new one). Given that the deviceid is a 128-bit field, such a
> solution is 100% practical. That would allow you to say you support
> notification, but not have to implement it.

Deviceid must never be retired and reused, to quote from 12.2.10.:

Once a device ID is
deleted by the server, the server MUST NOT reuse the device ID for
the same layout type and client ID again.  This requirement is
feasible because the device ID is 16 bytes long, leaving sufficient
room to store a generation number if the server's implementation
requires most of the rest of the device ID's content to	be reused.
This requirement is necessary because otherwise the race conditions
between asynchronous notification of device ID addition and deletion
would be too difficult to sort out.

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

[Trond Myklebust]

On Wed, Mar 25, 2015 at 9:45 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Mar 25, 2015 at 08:49:19AM -0400, Trond Myklebust wrote:
>> The reason for the GETDEVICEINFO description in RFC5661 is that you
>> need some mechanism to allow clients to know when a deviceid is
>> retired. There is no reasonable alternative to notification that
>> doesn't cause problems on either the client or the server.
>>
>> That said, do note that one valid solution here is to have deviceids
>> that never expire (i.e. if you need to retire them, then you allocate
>> a new one). Given that the deviceid is a 128-bit field, such a
>> solution is 100% practical. That would allow you to say you support
>> notification, but not have to implement it.
>
> Deviceid must never be retired and reused, to quote from 12.2.10.:
>
> Once a device ID is
> deleted by the server, the server MUST NOT reuse the device ID for
> the same layout type and client ID again.  This requirement is
> feasible because the device ID is 16 bytes long, leaving sufficient
> room to store a generation number if the server's implementation
> requires most of the rest of the device ID's content to be reused.
> This requirement is necessary because otherwise the race conditions
> between asynchronous notification of device ID addition and deletion
> would be too difficult to sort out.

That doesn't change what I said about the feasibility of working
around the notification requirement.

-- 
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

[Mkrtchyan, Tigran]

----- Original Message -----
> From: "Trond Myklebust" <trond.myklebust@primarydata.com>
> To: "Christoph Hellwig" <hch@infradead.org>
> Cc: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>, "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
> Sent: Wednesday, March 25, 2015 1:49:19 PM
> Subject: Re: [PATCH 4/4] NFSv4.1: Don't cache deviceids that have no notifications

> On Wed, Mar 25, 2015 at 4:55 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> 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.
> 
> The reason for the GETDEVICEINFO description in RFC5661 is that you
> need some mechanism to allow clients to know when a deviceid is
> retired. There is no reasonable alternative to notification that
> doesn't cause problems on either the client or the server.
> 
> That said, do note that one valid solution here is to have deviceids
> that never expire (i.e. if you need to retire them, then you allocate
> a new one). Given that the deviceid is a 128-bit field, such a
> solution is 100% practical. That would allow you to say you support
> notification, but not have to implement it.


I wasn't happy with that change either. But it was easy to add
a simple (single bit) hack and lie to client, that server supports
notifications. Eventually, if one-time device IDs will be required,
client already will do the right thing, which is good.

Tigran.

> 
> Trond

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.

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 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.

[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