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
There really is no reason to do so.
Signed-off-by: Trond Myklebust <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
On Tue, Mar 17, 2015 at 6:37 AM, Christoph Hellwig <[email protected]> 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
[email protected]
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.
On Wed, Mar 25, 2015 at 4:55 AM, Christoph Hellwig <[email protected]> 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
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.
On Wed, Mar 25, 2015 at 9:45 AM, Christoph Hellwig <[email protected]> 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
[email protected]
----- Original Message -----
> From: "Trond Myklebust" <[email protected]>
> To: "Christoph Hellwig" <[email protected]>
> Cc: "Tigran Mkrtchyan" <[email protected]>, "Linux NFS Mailing List" <[email protected]>
> 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 <[email protected]> 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