The following patchset makes the largest difference w.r.t. NFSv4, where
we don't always ask for the full set of attributes from the server, however
there are also some tidbits for NFSv3.
With this patchset, running xfstests over NFSv4.1 shows the following
improvement in execution times:
Generic/074 now 264s, was 344s
Generic/075 now 15s was 26s
Generic/089 now 178s was 236s
Generic/112 now 14s was 25s
Generic/113 now 14s was 20s
Generic/127 now 206s was 223s
Generic/133 now 85s was 118s
Generic/247 now 27s was 43s
I saw no statistically significant performance regressions (and no functional
regressions).
For nfsstats, the client shows a significant decrease in % GETATTR and
% LOOKUP calls on the same workloads.
For 2 runs of xfstests, the numbers are:
GETATTR: now 1456511, was 2168740
LOOKUP: now 301073, was 545571
ACCESS: now 137211 was 139447
Trond Myklebust (9):
NFSv4: Don't invalidate the directory twice
NFSv4: Update the attribute cache info in update_changeattr
NFSv4: Don't discard the attributes returned by asynchronous
DELEGRETURN
NFS: Don't revalidate the file on close if we hold a delegation
NFS: Clean up cache validity checking
NFS: Only look at the change attribute cache state in
nfs_weak_revalidate()
NFS: Fix and clean up the access cache validity checking
NFS: Remove unused function nfs_revalidate_inode_rcu()
NFS: Clean up nfs_attribute_timeout()
fs/nfs/dir.c | 23 +++++++++--------
fs/nfs/file.c | 12 +--------
fs/nfs/inode.c | 68 ++++++++++++++++++++++++++++++--------------------
fs/nfs/internal.h | 1 +
fs/nfs/nfs4proc.c | 17 +++++++++----
include/linux/nfs_fs.h | 2 --
6 files changed, 67 insertions(+), 56 deletions(-)
--
2.9.3
If an operation that modified the directory raced with a GETATTR, then we
don't need to invalidate the directory cache more than once.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d33242c8d95d..713932440e07 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1088,12 +1088,15 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo)
struct nfs_inode *nfsi = NFS_I(dir);
spin_lock(&dir->i_lock);
+ if (dir->i_version == cinfo->after)
+ goto out;
nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
if (!cinfo->atomic || cinfo->before != dir->i_version)
nfs_force_lookup_revalidate(dir);
dir->i_version = cinfo->after;
nfsi->attr_gencount = nfs_inc_attr_generation_counter();
nfs_fscache_invalidate(dir);
+out:
spin_unlock(&dir->i_lock);
}
--
2.9.3
If we're holding a delegation, we can skip sending the close-to-open
GETATTR until we're returning that delegation.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/inode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7de345fd8e1e..2fc237cd338e 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -795,6 +795,8 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
if (!is_sync)
return;
inode = d_inode(ctx->dentry);
+ if (NFS_PROTO(inode)->have_delegation(inode, FMODE_READ))
+ return;
nfsi = NFS_I(inode);
if (inode->i_mapping->nrpages == 0)
return;
--
2.9.3
Consolidate the open-coded checking of NFS_I(inode)->cache_validity
into a couple of helper functions.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/file.c | 12 +-----------
fs/nfs/inode.c | 43 ++++++++++++++++++++++++++++++++-----------
fs/nfs/internal.h | 1 +
3 files changed, 34 insertions(+), 22 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 64c11f399b3d..599a602cb2fd 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -101,21 +101,11 @@ EXPORT_SYMBOL_GPL(nfs_file_release);
static int nfs_revalidate_file_size(struct inode *inode, struct file *filp)
{
struct nfs_server *server = NFS_SERVER(inode);
- struct nfs_inode *nfsi = NFS_I(inode);
- const unsigned long force_reval = NFS_INO_REVAL_PAGECACHE|NFS_INO_REVAL_FORCED;
- unsigned long cache_validity = nfsi->cache_validity;
-
- if (NFS_PROTO(inode)->have_delegation(inode, FMODE_READ) &&
- (cache_validity & force_reval) != force_reval)
- goto out_noreval;
if (filp->f_flags & O_DIRECT)
goto force_reval;
- if (nfsi->cache_validity & NFS_INO_REVAL_PAGECACHE)
- goto force_reval;
- if (nfs_attribute_timeout(inode))
+ if (nfs_check_cache_invalid(inode, NFS_INO_REVAL_PAGECACHE))
goto force_reval;
-out_noreval:
return 0;
force_reval:
return __nfs_revalidate_inode(server, inode);
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 2fc237cd338e..e3194aa797f7 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -160,6 +160,36 @@ int nfs_sync_mapping(struct address_space *mapping)
return ret;
}
+static bool nfs_check_cache_invalid_delegated(struct inode *inode, unsigned long flags)
+{
+ unsigned long cache_validity = READ_ONCE(NFS_I(inode)->cache_validity);
+
+ /* Special case for the pagecache or access cache */
+ if (flags == NFS_INO_REVAL_PAGECACHE &&
+ !(cache_validity & NFS_INO_REVAL_FORCED))
+ return false;
+ return (cache_validity & flags) != 0;
+}
+
+static bool nfs_check_cache_invalid_not_delegated(struct inode *inode, unsigned long flags)
+{
+ unsigned long cache_validity = READ_ONCE(NFS_I(inode)->cache_validity);
+
+ if ((cache_validity & flags) != 0)
+ return true;
+ if (nfs_attribute_timeout(inode))
+ return true;
+ return false;
+}
+
+bool nfs_check_cache_invalid(struct inode *inode, unsigned long flags)
+{
+ if (NFS_PROTO(inode)->have_delegation(inode, FMODE_READ))
+ return nfs_check_cache_invalid_delegated(inode, flags);
+
+ return nfs_check_cache_invalid_not_delegated(inode, flags);
+}
+
static void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
{
struct nfs_inode *nfsi = NFS_I(inode);
@@ -1116,17 +1146,8 @@ static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map
bool nfs_mapping_need_revalidate_inode(struct inode *inode)
{
- unsigned long cache_validity = NFS_I(inode)->cache_validity;
-
- if (NFS_PROTO(inode)->have_delegation(inode, FMODE_READ)) {
- const unsigned long force_reval =
- NFS_INO_REVAL_PAGECACHE|NFS_INO_REVAL_FORCED;
- return (cache_validity & force_reval) == force_reval;
- }
-
- return (cache_validity & NFS_INO_REVAL_PAGECACHE)
- || nfs_attribute_timeout(inode)
- || NFS_STALE(inode);
+ return nfs_check_cache_invalid(inode, NFS_INO_REVAL_PAGECACHE) ||
+ NFS_STALE(inode);
}
int nfs_revalidate_mapping_rcu(struct inode *inode)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 6b79c2ca9b9a..09ca5095c04e 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -381,6 +381,7 @@ extern int nfs_drop_inode(struct inode *);
extern void nfs_clear_inode(struct inode *);
extern void nfs_evict_inode(struct inode *);
void nfs_zap_acl_cache(struct inode *inode);
+extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
extern int nfs_wait_atomic_killable(atomic_t *p);
--
2.9.3
Just like in nfs_check_verifier(), we want to use
nfs_mapping_need_revalidate_inode() to check our knowledge of the
change attribute is up to date.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index cb22a9f9ae7e..8f706f3e5c05 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1273,8 +1273,8 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
*/
static int nfs_weak_revalidate(struct dentry *dentry, unsigned int flags)
{
- int error;
struct inode *inode = d_inode(dentry);
+ int error = 0;
/*
* I believe we can only get a negative dentry here in the case of a
@@ -1293,7 +1293,8 @@ static int nfs_weak_revalidate(struct dentry *dentry, unsigned int flags)
return 0;
}
- error = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+ if (nfs_mapping_need_revalidate_inode(inode))
+ error = __nfs_revalidate_inode(NFS_SERVER(inode), inode);
dfprintk(LOOKUPCACHE, "NFS: %s: inode %lu is %s\n",
__func__, inode->i_ino, error ? "invalid" : "valid");
return !error;
--
2.9.3
The access cache needs to check whether or not the mode bits, ownership,
or ACL has changed or the cache has timed out.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8f706f3e5c05..fad81041f5ab 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2286,8 +2286,7 @@ static int nfs_access_get_cached(struct inode *inode, struct rpc_cred *cred, str
if (cache == NULL)
goto out;
/* Found an entry, is our attribute cache valid? */
- if (!nfs_attribute_cache_expired(inode) &&
- !(nfsi->cache_validity & NFS_INO_INVALID_ATTR))
+ if (!nfs_check_cache_invalid(inode, NFS_INO_INVALID_ACCESS))
break;
err = -ECHILD;
if (!may_block)
@@ -2335,12 +2334,12 @@ static int nfs_access_get_cached_rcu(struct inode *inode, struct rpc_cred *cred,
cache = NULL;
if (cache == NULL)
goto out;
- err = nfs_revalidate_inode_rcu(NFS_SERVER(inode), inode);
- if (err)
+ if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_ACCESS))
goto out;
res->jiffies = cache->jiffies;
res->cred = cache->cred;
res->mask = cache->mask;
+ err = 0;
out:
rcu_read_unlock();
return err;
@@ -2492,12 +2491,13 @@ EXPORT_SYMBOL_GPL(nfs_may_open);
static int nfs_execute_ok(struct inode *inode, int mask)
{
struct nfs_server *server = NFS_SERVER(inode);
- int ret;
+ int ret = 0;
- if (mask & MAY_NOT_BLOCK)
- ret = nfs_revalidate_inode_rcu(server, inode);
- else
- ret = nfs_revalidate_inode(server, inode);
+ if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_ACCESS)) {
+ if (mask & MAY_NOT_BLOCK)
+ return -ECHILD;
+ ret = __nfs_revalidate_inode(server, inode);
+ }
if (ret == 0 && !execute_ok(inode))
ret = -EACCES;
return ret;
--
2.9.3
It can be made static.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/inode.c | 14 +++++++-------
include/linux/nfs_fs.h | 1 -
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 8224e96b5808..a0cd79646c1b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -160,6 +160,13 @@ int nfs_sync_mapping(struct address_space *mapping)
return ret;
}
+static int nfs_attribute_timeout(struct inode *inode)
+{
+ struct nfs_inode *nfsi = NFS_I(inode);
+
+ return !time_in_range_open(jiffies, nfsi->read_cache_jiffies, nfsi->read_cache_jiffies + nfsi->attrtimeo);
+}
+
static bool nfs_check_cache_invalid_delegated(struct inode *inode, unsigned long flags)
{
unsigned long cache_validity = READ_ONCE(NFS_I(inode)->cache_validity);
@@ -1076,13 +1083,6 @@ __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
return status;
}
-int nfs_attribute_timeout(struct inode *inode)
-{
- struct nfs_inode *nfsi = NFS_I(inode);
-
- return !time_in_range_open(jiffies, nfsi->read_cache_jiffies, nfsi->read_cache_jiffies + nfsi->attrtimeo);
-}
-
int nfs_attribute_cache_expired(struct inode *inode)
{
if (nfs_have_delegated_attributes(inode))
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index fe2e7810ae80..f1da8c8dd473 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -340,7 +340,6 @@ extern void nfs_access_add_cache(struct inode *, struct nfs_access_entry *);
extern void nfs_access_set_mask(struct nfs_access_entry *, u32);
extern int nfs_permission(struct inode *, int);
extern int nfs_open(struct inode *, struct file *);
-extern int nfs_attribute_timeout(struct inode *inode);
extern int nfs_attribute_cache_expired(struct inode *inode);
extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);
extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
--
2.9.3
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/inode.c | 9 ---------
include/linux/nfs_fs.h | 1 -
2 files changed, 10 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e3194aa797f7..8224e96b5808 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1105,15 +1105,6 @@ int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
}
EXPORT_SYMBOL_GPL(nfs_revalidate_inode);
-int nfs_revalidate_inode_rcu(struct nfs_server *server, struct inode *inode)
-{
- if (!(NFS_I(inode)->cache_validity &
- (NFS_INO_INVALID_ATTR|NFS_INO_INVALID_LABEL))
- && !nfs_attribute_cache_expired(inode))
- return NFS_STALE(inode) ? -ESTALE : 0;
- return -ECHILD;
-}
-
static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
{
struct nfs_inode *nfsi = NFS_I(inode);
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index cb631973839a..fe2e7810ae80 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -343,7 +343,6 @@ extern int nfs_open(struct inode *, struct file *);
extern int nfs_attribute_timeout(struct inode *inode);
extern int nfs_attribute_cache_expired(struct inode *inode);
extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);
-extern int nfs_revalidate_inode_rcu(struct nfs_server *server, struct inode *inode);
extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
extern bool nfs_mapping_need_revalidate_inode(struct inode *inode);
extern int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping);
--
2.9.3
If we successfully updated the change attribute, we should timestamp the
cache. While we do know that the other attributes are not completely up
to date, we have the NFS_INO_INVALID_ATTR flag that let us know that,
so it is valid to say that the cache has not timed out.
We can also clear NFS_INO_REVAL_PAGECACHE, since our change attribute
is now known to be valid.
Conversely, if the change attribute did not match, we should make sure to
also revalidate the access and ACL caches.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 713932440e07..2e0816cb7136 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1091,8 +1091,15 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo)
if (dir->i_version == cinfo->after)
goto out;
nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
- if (!cinfo->atomic || cinfo->before != dir->i_version)
+ if (cinfo->atomic && cinfo->before == dir->i_version) {
+ nfsi->cache_validity &= ~NFS_INO_REVAL_PAGECACHE;
+ nfsi->attrtimeo_timestamp = jiffies;
+ } else {
nfs_force_lookup_revalidate(dir);
+ if (cinfo->before != dir->i_version)
+ nfsi->cache_validity |= NFS_INO_INVALID_ACCESS |
+ NFS_INO_INVALID_ACL;
+ }
dir->i_version = cinfo->after;
nfsi->attr_gencount = nfs_inc_attr_generation_counter();
nfs_fscache_invalidate(dir);
--
2.9.3
DELEGRETURN will always carry a reference to the inode except when
the latter is being freed, so let's ensure that we always use that
inode information to ensure close-to-open cache consistency, even
when the DELEGRETURN call is asynchronous.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2e0816cb7136..58e3d5f627f2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5702,6 +5702,7 @@ static void nfs4_delegreturn_release(void *calldata)
if (data->lr.roc)
pnfs_roc_release(&data->lr.arg, &data->lr.res,
data->res.lr_ret);
+ nfs_post_op_update_inode_force_wcc(inode, &data->fattr);
nfs_iput_and_deactive(inode);
}
kfree(calldata);
@@ -5790,10 +5791,6 @@ static int _nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, co
if (status != 0)
goto out;
status = data->rpc_status;
- if (status == 0)
- nfs_post_op_update_inode_force_wcc(inode, &data->fattr);
- else
- nfs_refresh_inode(inode, &data->fattr);
out:
rpc_put_task(task);
return status;
--
2.9.3
Hi Trond,
On 12/17/2016 01:27 PM, Trond Myklebust wrote:
> If an operation that modified the directory raced with a GETATTR, then we
> don't need to invalidate the directory cache more than once.
This patch causes cthon basic tests to fail with:
./test6: readdir
./test6: (/nfs/all) unlinked 'file.0' dir entry read pass 1
./test6: (/nfs/all) Test failed with 1 errors
basic tests failed
Thanks,
Anna
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d33242c8d95d..713932440e07 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1088,12 +1088,15 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo)
> struct nfs_inode *nfsi = NFS_I(dir);
>
> spin_lock(&dir->i_lock);
> + if (dir->i_version == cinfo->after)
> + goto out;
> nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
> if (!cinfo->atomic || cinfo->before != dir->i_version)
> nfs_force_lookup_revalidate(dir);
> dir->i_version = cinfo->after;
> nfsi->attr_gencount = nfs_inc_attr_generation_counter();
> nfs_fscache_invalidate(dir);
> +out:
> spin_unlock(&dir->i_lock);
> }
>
>
V2hhdOKAmXMgdGhlIGZpbGVzeXN0ZW0geW914oCZcmUgdGVzdGluZyBvbj8NCg0KPiBPbiBEZWMg
MTksIDIwMTYsIGF0IDExOjU0LCBBbm5hIFNjaHVtYWtlciA8c2NodW1ha2VyLmFubmFAZ21haWwu
Y29tPiB3cm90ZToNCj4gDQo+IEhpIFRyb25kLA0KPiANCj4gT24gMTIvMTcvMjAxNiAwMToyNyBQ
TSwgVHJvbmQgTXlrbGVidXN0IHdyb3RlOg0KPj4gSWYgYW4gb3BlcmF0aW9uIHRoYXQgbW9kaWZp
ZWQgdGhlIGRpcmVjdG9yeSByYWNlZCB3aXRoIGEgR0VUQVRUUiwgdGhlbiB3ZQ0KPj4gZG9uJ3Qg
bmVlZCB0byBpbnZhbGlkYXRlIHRoZSBkaXJlY3RvcnkgY2FjaGUgbW9yZSB0aGFuIG9uY2UuDQo+
IA0KPiBUaGlzIHBhdGNoIGNhdXNlcyBjdGhvbiBiYXNpYyB0ZXN0cyB0byBmYWlsIHdpdGg6DQo+
IA0KPiAuL3Rlc3Q2OiByZWFkZGlyDQo+ICAgICAgICAuL3Rlc3Q2OiAoL25mcy9hbGwpIHVubGlu
a2VkICdmaWxlLjAnIGRpciBlbnRyeSByZWFkIHBhc3MgMQ0KPiAgICAgICAgLi90ZXN0NjogKC9u
ZnMvYWxsKSBUZXN0IGZhaWxlZCB3aXRoIDEgZXJyb3JzDQo+IGJhc2ljIHRlc3RzIGZhaWxlZA0K
PiANCj4gVGhhbmtzLA0KPiBBbm5hDQo+PiANCj4+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xl
YnVzdCA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4NCj4+IC0tLQ0KPj4gZnMvbmZz
L25mczRwcm9jLmMgfCAzICsrKw0KPj4gMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygrKQ0K
Pj4gDQo+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMvbmZzNHByb2Mu
Yw0KPj4gaW5kZXggZDMzMjQyYzhkOTVkLi43MTM5MzI0NDBlMDcgMTAwNjQ0DQo+PiAtLS0gYS9m
cy9uZnMvbmZzNHByb2MuYw0KPj4gKysrIGIvZnMvbmZzL25mczRwcm9jLmMNCj4+IEBAIC0xMDg4
LDEyICsxMDg4LDE1IEBAIHN0YXRpYyB2b2lkIHVwZGF0ZV9jaGFuZ2VhdHRyKHN0cnVjdCBpbm9k
ZSAqZGlyLCBzdHJ1Y3QgbmZzNF9jaGFuZ2VfaW5mbyAqY2luZm8pDQo+PiAJc3RydWN0IG5mc19p
bm9kZSAqbmZzaSA9IE5GU19JKGRpcik7DQo+PiANCj4+IAlzcGluX2xvY2soJmRpci0+aV9sb2Nr
KTsNCj4+ICsJaWYgKGRpci0+aV92ZXJzaW9uID09IGNpbmZvLT5hZnRlcikNCj4+ICsJCWdvdG8g
b3V0Ow0KPj4gCW5mc2ktPmNhY2hlX3ZhbGlkaXR5IHw9IE5GU19JTk9fSU5WQUxJRF9BVFRSfE5G
U19JTk9fSU5WQUxJRF9EQVRBOw0KPj4gCWlmICghY2luZm8tPmF0b21pYyB8fCBjaW5mby0+YmVm
b3JlICE9IGRpci0+aV92ZXJzaW9uKQ0KPj4gCQluZnNfZm9yY2VfbG9va3VwX3JldmFsaWRhdGUo
ZGlyKTsNCj4+IAlkaXItPmlfdmVyc2lvbiA9IGNpbmZvLT5hZnRlcjsNCj4+IAluZnNpLT5hdHRy
X2dlbmNvdW50ID0gbmZzX2luY19hdHRyX2dlbmVyYXRpb25fY291bnRlcigpOw0KPj4gCW5mc19m
c2NhY2hlX2ludmFsaWRhdGUoZGlyKTsNCj4+ICtvdXQ6DQo+PiAJc3Bpbl91bmxvY2soJmRpci0+
aV9sb2NrKTsNCj4+IH0NCj4+IA0KPj4gDQo+IA0KDQo=
DQo+IE9uIERlYyAxOSwgMjAxNiwgYXQgMTE6NTYsIFRyb25kIE15a2xlYnVzdCA8dHJvbmRteUBw
cmltYXJ5ZGF0YS5jb20+IHdyb3RlOg0KPiANCj4gV2hhdOKAmXMgdGhlIGZpbGVzeXN0ZW0geW91
4oCZcmUgdGVzdGluZyBvbj8NCg0K4oCmYW5kIHdoYXQgaXMgdGhlIHRpbWVzdGFtcCByZXNvbHV0
aW9uIHlvdeKAmXJlIHNlZWluZyBvbiBpdD8NCg0KPiANCj4+IE9uIERlYyAxOSwgMjAxNiwgYXQg
MTE6NTQsIEFubmEgU2NodW1ha2VyIDxzY2h1bWFrZXIuYW5uYUBnbWFpbC5jb20+IHdyb3RlOg0K
Pj4gDQo+PiBIaSBUcm9uZCwNCj4+IA0KPj4gT24gMTIvMTcvMjAxNiAwMToyNyBQTSwgVHJvbmQg
TXlrbGVidXN0IHdyb3RlOg0KPj4+IElmIGFuIG9wZXJhdGlvbiB0aGF0IG1vZGlmaWVkIHRoZSBk
aXJlY3RvcnkgcmFjZWQgd2l0aCBhIEdFVEFUVFIsIHRoZW4gd2UNCj4+PiBkb24ndCBuZWVkIHRv
IGludmFsaWRhdGUgdGhlIGRpcmVjdG9yeSBjYWNoZSBtb3JlIHRoYW4gb25jZS4NCj4+IA0KPj4g
VGhpcyBwYXRjaCBjYXVzZXMgY3Rob24gYmFzaWMgdGVzdHMgdG8gZmFpbCB3aXRoOg0KPj4gDQo+
PiAuL3Rlc3Q2OiByZWFkZGlyDQo+PiAgICAgICAuL3Rlc3Q2OiAoL25mcy9hbGwpIHVubGlua2Vk
ICdmaWxlLjAnIGRpciBlbnRyeSByZWFkIHBhc3MgMQ0KPj4gICAgICAgLi90ZXN0NjogKC9uZnMv
YWxsKSBUZXN0IGZhaWxlZCB3aXRoIDEgZXJyb3JzDQo+PiBiYXNpYyB0ZXN0cyBmYWlsZWQNCj4+
IA0KPj4gVGhhbmtzLA0KPj4gQW5uYQ0KPj4+IA0KPj4+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15
a2xlYnVzdCA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4NCj4+PiAtLS0NCj4+PiBm
cy9uZnMvbmZzNHByb2MuYyB8IDMgKysrDQo+Pj4gMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9u
cygrKQ0KPj4+IA0KPj4+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9u
ZnM0cHJvYy5jDQo+Pj4gaW5kZXggZDMzMjQyYzhkOTVkLi43MTM5MzI0NDBlMDcgMTAwNjQ0DQo+
Pj4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCj4+PiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0K
Pj4+IEBAIC0xMDg4LDEyICsxMDg4LDE1IEBAIHN0YXRpYyB2b2lkIHVwZGF0ZV9jaGFuZ2VhdHRy
KHN0cnVjdCBpbm9kZSAqZGlyLCBzdHJ1Y3QgbmZzNF9jaGFuZ2VfaW5mbyAqY2luZm8pDQo+Pj4g
CXN0cnVjdCBuZnNfaW5vZGUgKm5mc2kgPSBORlNfSShkaXIpOw0KPj4+IA0KPj4+IAlzcGluX2xv
Y2soJmRpci0+aV9sb2NrKTsNCj4+PiArCWlmIChkaXItPmlfdmVyc2lvbiA9PSBjaW5mby0+YWZ0
ZXIpDQo+Pj4gKwkJZ290byBvdXQ7DQo+Pj4gCW5mc2ktPmNhY2hlX3ZhbGlkaXR5IHw9IE5GU19J
Tk9fSU5WQUxJRF9BVFRSfE5GU19JTk9fSU5WQUxJRF9EQVRBOw0KPj4+IAlpZiAoIWNpbmZvLT5h
dG9taWMgfHwgY2luZm8tPmJlZm9yZSAhPSBkaXItPmlfdmVyc2lvbikNCj4+PiAJCW5mc19mb3Jj
ZV9sb29rdXBfcmV2YWxpZGF0ZShkaXIpOw0KPj4+IAlkaXItPmlfdmVyc2lvbiA9IGNpbmZvLT5h
ZnRlcjsNCj4+PiAJbmZzaS0+YXR0cl9nZW5jb3VudCA9IG5mc19pbmNfYXR0cl9nZW5lcmF0aW9u
X2NvdW50ZXIoKTsNCj4+PiAJbmZzX2ZzY2FjaGVfaW52YWxpZGF0ZShkaXIpOw0KPj4+ICtvdXQ6
DQo+Pj4gCXNwaW5fdW5sb2NrKCZkaXItPmlfbG9jayk7DQo+Pj4gfQ0KPj4+IA0KPj4+IA0KPj4g
DQo+IA0KPiBO77+977+977+977+977+9cu+/ve+/vXnvv73vv73vv71i77+9WO+/ve+/vcendu+/
vV7vv70p3rp7Lm7vv70r77+977+977+977+9e++/ve+/ve+/vSLvv73vv71ebu+/vXLvv73vv73v
v71677+9Gu+/ve+/vWjvv73vv73vv73vv70m77+977+9Hu+/vUfvv73vv73vv71o77+9Ayjvv73p
mo7vv73domoi77+977+9Gu+/vRtt77+977+977+977+977+9eu+/vd6W77+977+977+9Zu+/ve+/
ve+/vWjvv73vv73vv71+77+9be+/vQ0KDQo=
On 12/19/2016 11:56 AM, Trond Myklebust wrote:
> What’s the filesystem you’re testing on?
I see this on xfs and ext4 but btrfs seems to work. What do you mean by timestamp resolution? I can't find the file in question in `ls`, but for another file `stat` tells me:
File: file.10
Size: 0 Blocks: 0 IO Block: 4096 regular empty file
Device: fe31h/65073d Inode: 1319795 Links: 1
Access: (0666/-rw-rw-rw-) Uid: ( 1000/ anna) Gid: ( 100/ users)
Access: 2016-12-19 12:04:47.680033877 -0500
Modify: 2016-12-19 12:04:47.680033877 -0500
Change: 2016-12-19 12:04:47.680033877 -0500
I hope this helps!
Anna
>
>> On Dec 19, 2016, at 11:54, Anna Schumaker <[email protected]> wrote:
>>
>> Hi Trond,
>>
>> On 12/17/2016 01:27 PM, Trond Myklebust wrote:
>>> If an operation that modified the directory raced with a GETATTR, then we
>>> don't need to invalidate the directory cache more than once.
>>
>> This patch causes cthon basic tests to fail with:
>>
>> ./test6: readdir
>> ./test6: (/nfs/all) unlinked 'file.0' dir entry read pass 1
>> ./test6: (/nfs/all) Test failed with 1 errors
>> basic tests failed
>>
>> Thanks,
>> Anna
>>>
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>> fs/nfs/nfs4proc.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index d33242c8d95d..713932440e07 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -1088,12 +1088,15 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo)
>>> struct nfs_inode *nfsi = NFS_I(dir);
>>>
>>> spin_lock(&dir->i_lock);
>>> + if (dir->i_version == cinfo->after)
>>> + goto out;
>>> nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
>>> if (!cinfo->atomic || cinfo->before != dir->i_version)
>>> nfs_force_lookup_revalidate(dir);
>>> dir->i_version = cinfo->after;
>>> nfsi->attr_gencount = nfs_inc_attr_generation_counter();
>>> nfs_fscache_invalidate(dir);
>>> +out:
>>> spin_unlock(&dir->i_lock);
>>> }
>>>
>>>
>>
>
DQo+IE9uIERlYyAxOSwgMjAxNiwgYXQgMTI6MDcsIEFubmEgU2NodW1ha2VyIDxzY2h1bWFrZXIu
YW5uYUBnbWFpbC5jb20+IHdyb3RlOg0KPiANCj4gDQo+IA0KPiBPbiAxMi8xOS8yMDE2IDExOjU2
IEFNLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6DQo+PiBXaGF04oCZcyB0aGUgZmlsZXN5c3RlbSB5
b3XigJlyZSB0ZXN0aW5nIG9uPw0KPiANCj4gSSBzZWUgdGhpcyBvbiB4ZnMgYW5kIGV4dDQgYnV0
IGJ0cmZzIHNlZW1zIHRvIHdvcmsuICBXaGF0IGRvIHlvdSBtZWFuIGJ5IHRpbWVzdGFtcCByZXNv
bHV0aW9uPyAgSSBjYW4ndCBmaW5kIHRoZSBmaWxlIGluIHF1ZXN0aW9uIGluIGBsc2AsIGJ1dCBm
b3IgYW5vdGhlciBmaWxlIGBzdGF0YCB0ZWxscyBtZToNCj4gDQo+ICBGaWxlOiBmaWxlLjEwDQo+
ICBTaXplOiAwICAgICAgICAgICAgICAgQmxvY2tzOiAwICAgICAgICAgIElPIEJsb2NrOiA0MDk2
ICAgcmVndWxhciBlbXB0eSBmaWxlDQo+IERldmljZTogZmUzMWgvNjUwNzNkICAgIElub2RlOiAx
MzE5Nzk1ICAgICBMaW5rczogMQ0KPiBBY2Nlc3M6ICgwNjY2Ly1ydy1ydy1ydy0pICBVaWQ6ICgg
MTAwMC8gICAgYW5uYSkgICBHaWQ6ICggIDEwMC8gICB1c2VycykNCj4gQWNjZXNzOiAyMDE2LTEy
LTE5IDEyOjA0OjQ3LjY4MDAzMzg3NyAtMDUwMA0KPiBNb2RpZnk6IDIwMTYtMTItMTkgMTI6MDQ6
NDcuNjgwMDMzODc3IC0wNTAwDQo+IENoYW5nZTogMjAxNi0xMi0xOSAxMjowNDo0Ny42ODAwMzM4
NzcgLTA1MDANCg0KSSBtZWFuIHRoYXQgdGhlIHRpbWVzdGFtcHMgYXBwZWFyIHRvIGJlIHVuYWJs
ZSB0byBrZWVwIHVwIHdpdGggdGhlIHZvbHVtZSBvZiBjaGFuZ2VzIG9uIHRoZSBmaWxlc3lzdGVt
LiBJZiB0aGUgY3Rob24gdGVzdCBpcyBmYWlsaW5nIGhlcmUsIHRoZW4gaXTigJlzIHByb2JhYmx5
IGJlY2F1c2UgdGhlIGJlZm9yZS9hZnRlciBjaGFuZ2UgYXR0cmlidXRlcyBhcmUgdGhlIHNhbWXi
gKYgRG9lcyB3aXJlc2hhcmsgc2hvdyB0aGF0IHRoZSBiZWZvcmUvYWZ0ZXIgY2hhbmdlIGF0dHJp
YnV0ZXMgb24gdGhpcyBkaXJlY3RvcnkgYXJlIGRpZmZlcmVudD8NCg0KPiANCj4gSSBob3BlIHRo
aXMgaGVscHMhDQo+IEFubmENCj4+IA0KPj4+IE9uIERlYyAxOSwgMjAxNiwgYXQgMTE6NTQsIEFu
bmEgU2NodW1ha2VyIDxzY2h1bWFrZXIuYW5uYUBnbWFpbC5jb20+IHdyb3RlOg0KPj4+IA0KPj4+
IEhpIFRyb25kLA0KPj4+IA0KPj4+IE9uIDEyLzE3LzIwMTYgMDE6MjcgUE0sIFRyb25kIE15a2xl
YnVzdCB3cm90ZToNCj4+Pj4gSWYgYW4gb3BlcmF0aW9uIHRoYXQgbW9kaWZpZWQgdGhlIGRpcmVj
dG9yeSByYWNlZCB3aXRoIGEgR0VUQVRUUiwgdGhlbiB3ZQ0KPj4+PiBkb24ndCBuZWVkIHRvIGlu
dmFsaWRhdGUgdGhlIGRpcmVjdG9yeSBjYWNoZSBtb3JlIHRoYW4gb25jZS4NCj4+PiANCj4+PiBU
aGlzIHBhdGNoIGNhdXNlcyBjdGhvbiBiYXNpYyB0ZXN0cyB0byBmYWlsIHdpdGg6DQo+Pj4gDQo+
Pj4gLi90ZXN0NjogcmVhZGRpcg0KPj4+ICAgICAgIC4vdGVzdDY6ICgvbmZzL2FsbCkgdW5saW5r
ZWQgJ2ZpbGUuMCcgZGlyIGVudHJ5IHJlYWQgcGFzcyAxDQo+Pj4gICAgICAgLi90ZXN0NjogKC9u
ZnMvYWxsKSBUZXN0IGZhaWxlZCB3aXRoIDEgZXJyb3JzDQo+Pj4gYmFzaWMgdGVzdHMgZmFpbGVk
DQo+Pj4gDQo+Pj4gVGhhbmtzLA0KPj4+IEFubmENCj4+Pj4gDQo+Pj4+IFNpZ25lZC1vZmYtYnk6
IFRyb25kIE15a2xlYnVzdCA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4NCj4+Pj4g
LS0tDQo+Pj4+IGZzL25mcy9uZnM0cHJvYy5jIHwgMyArKysNCj4+Pj4gMSBmaWxlIGNoYW5nZWQs
IDMgaW5zZXJ0aW9ucygrKQ0KPj4+PiANCj4+Pj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0cHJv
Yy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4+Pj4gaW5kZXggZDMzMjQyYzhkOTVkLi43MTM5MzI0
NDBlMDcgMTAwNjQ0DQo+Pj4+IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+Pj4+ICsrKyBiL2Zz
L25mcy9uZnM0cHJvYy5jDQo+Pj4+IEBAIC0xMDg4LDEyICsxMDg4LDE1IEBAIHN0YXRpYyB2b2lk
IHVwZGF0ZV9jaGFuZ2VhdHRyKHN0cnVjdCBpbm9kZSAqZGlyLCBzdHJ1Y3QgbmZzNF9jaGFuZ2Vf
aW5mbyAqY2luZm8pDQo+Pj4+IAlzdHJ1Y3QgbmZzX2lub2RlICpuZnNpID0gTkZTX0koZGlyKTsN
Cj4+Pj4gDQo+Pj4+IAlzcGluX2xvY2soJmRpci0+aV9sb2NrKTsNCj4+Pj4gKwlpZiAoZGlyLT5p
X3ZlcnNpb24gPT0gY2luZm8tPmFmdGVyKQ0KPj4+PiArCQlnb3RvIG91dDsNCj4+Pj4gCW5mc2kt
PmNhY2hlX3ZhbGlkaXR5IHw9IE5GU19JTk9fSU5WQUxJRF9BVFRSfE5GU19JTk9fSU5WQUxJRF9E
QVRBOw0KPj4+PiAJaWYgKCFjaW5mby0+YXRvbWljIHx8IGNpbmZvLT5iZWZvcmUgIT0gZGlyLT5p
X3ZlcnNpb24pDQo+Pj4+IAkJbmZzX2ZvcmNlX2xvb2t1cF9yZXZhbGlkYXRlKGRpcik7DQo+Pj4+
IAlkaXItPmlfdmVyc2lvbiA9IGNpbmZvLT5hZnRlcjsNCj4+Pj4gCW5mc2ktPmF0dHJfZ2VuY291
bnQgPSBuZnNfaW5jX2F0dHJfZ2VuZXJhdGlvbl9jb3VudGVyKCk7DQo+Pj4+IAluZnNfZnNjYWNo
ZV9pbnZhbGlkYXRlKGRpcik7DQo+Pj4+ICtvdXQ6DQo+Pj4+IAlzcGluX3VubG9jaygmZGlyLT5p
X2xvY2spOw0KPj4+PiB9DQo+Pj4+IA0KPj4+PiANCj4+PiANCj4+IA0KPiANCg0K
On 12/19/2016 12:30 PM, Trond Myklebust wrote:
>
>> On Dec 19, 2016, at 12:07, Anna Schumaker <[email protected]> wrote:
>>
>>
>>
>> On 12/19/2016 11:56 AM, Trond Myklebust wrote:
>>> What’s the filesystem you’re testing on?
>>
>> I see this on xfs and ext4 but btrfs seems to work. What do you mean by timestamp resolution? I can't find the file in question in `ls`, but for another file `stat` tells me:
>>
>> File: file.10
>> Size: 0 Blocks: 0 IO Block: 4096 regular empty file
>> Device: fe31h/65073d Inode: 1319795 Links: 1
>> Access: (0666/-rw-rw-rw-) Uid: ( 1000/ anna) Gid: ( 100/ users)
>> Access: 2016-12-19 12:04:47.680033877 -0500
>> Modify: 2016-12-19 12:04:47.680033877 -0500
>> Change: 2016-12-19 12:04:47.680033877 -0500
>
> I mean that the timestamps appear to be unable to keep up with the volume of changes on the filesystem. If the cthon test is failing here, then it’s probably because the before/after change attributes are the same… Does wireshark show that the before/after change attributes on this directory are different?
Ah, that's what you mean. Yes, Wireshark does show the same values for before and after changeids.
Anna
>
>>
>> I hope this helps!
>> Anna
>>>
>>>> On Dec 19, 2016, at 11:54, Anna Schumaker <[email protected]> wrote:
>>>>
>>>> Hi Trond,
>>>>
>>>> On 12/17/2016 01:27 PM, Trond Myklebust wrote:
>>>>> If an operation that modified the directory raced with a GETATTR, then we
>>>>> don't need to invalidate the directory cache more than once.
>>>>
>>>> This patch causes cthon basic tests to fail with:
>>>>
>>>> ./test6: readdir
>>>> ./test6: (/nfs/all) unlinked 'file.0' dir entry read pass 1
>>>> ./test6: (/nfs/all) Test failed with 1 errors
>>>> basic tests failed
>>>>
>>>> Thanks,
>>>> Anna
>>>>>
>>>>> Signed-off-by: Trond Myklebust <[email protected]>
>>>>> ---
>>>>> fs/nfs/nfs4proc.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index d33242c8d95d..713932440e07 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -1088,12 +1088,15 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo)
>>>>> struct nfs_inode *nfsi = NFS_I(dir);
>>>>>
>>>>> spin_lock(&dir->i_lock);
>>>>> + if (dir->i_version == cinfo->after)
>>>>> + goto out;
>>>>> nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
>>>>> if (!cinfo->atomic || cinfo->before != dir->i_version)
>>>>> nfs_force_lookup_revalidate(dir);
>>>>> dir->i_version = cinfo->after;
>>>>> nfsi->attr_gencount = nfs_inc_attr_generation_counter();
>>>>> nfs_fscache_invalidate(dir);
>>>>> +out:
>>>>> spin_unlock(&dir->i_lock);
>>>>> }
>>>>>
>>>>>
>>>>
>>>
>>
>