Support the query flags AT_STATX_FORCE_SYNC by forcing an attribute
revalidation, and AT_STATX_DONT_SYNC by returning cached attributes
only.
Use the mask to optimise away server revalidation for attributes
that are not being requested by the user.
Signed-off-by: Trond Myklebust <[email protected]>
---
v2:
- Respect the attribute cache timeout.
- Unless AT_STATX_DONT_SYNC is set, we must only return attributes
that have been duly revalidated.
fs/nfs/inode.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index b112002dbdb2..c2c8bb7755ad 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -735,12 +735,20 @@ int nfs_getattr(const struct path *path, struct kstat *stat,
u32 request_mask, unsigned int query_flags)
{
struct inode *inode = d_inode(path->dentry);
- int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
+ struct nfs_server *server = NFS_SERVER(inode);
+ unsigned long cache_validity;
int err = 0;
+ bool force_sync = query_flags & AT_STATX_FORCE_SYNC;
+ bool do_update = false;
trace_nfs_getattr_enter(inode);
+
+ if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync)
+ goto out_no_update;
+
/* Flush out writes to the server in order to update c/mtime. */
- if (S_ISREG(inode->i_mode)) {
+ if ((request_mask & (STATX_CTIME|STATX_MTIME)) &&
+ S_ISREG(inode->i_mode)) {
err = filemap_write_and_wait(inode->i_mapping);
if (err)
goto out;
@@ -757,24 +765,41 @@ int nfs_getattr(const struct path *path, struct kstat *stat,
*/
if ((path->mnt->mnt_flags & MNT_NOATIME) ||
((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
- need_atime = 0;
-
- if (need_atime || nfs_need_revalidate_inode(inode)) {
- struct nfs_server *server = NFS_SERVER(inode);
-
+ request_mask &= ~STATX_ATIME;
+
+ /* Is the user requesting attributes that might need revalidation? */
+ if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
+ STATX_MTIME|STATX_UID|STATX_GID|
+ STATX_SIZE|STATX_BLOCKS)))
+ goto out_no_revalidate;
+
+ /* Check whether the cached attributes are stale */
+ do_update |= force_sync || nfs_attribute_cache_expired(inode);
+ cache_validity = READ_ONCE(NFS_I(inode)->cache_validity);
+ do_update |= cache_validity & NFS_INO_INVALID_ATTR;
+ if (request_mask & STATX_ATIME)
+ do_update |= cache_validity & NFS_INO_INVALID_ATIME;
+ if (request_mask & (STATX_CTIME|STATX_MTIME))
+ do_update |= cache_validity & NFS_INO_REVAL_PAGECACHE;
+ if (do_update) {
+ /* Update the attribute cache */
if (!(server->flags & NFS_MOUNT_NOAC))
nfs_readdirplus_parent_cache_miss(path->dentry);
else
nfs_readdirplus_parent_cache_hit(path->dentry);
err = __nfs_revalidate_inode(server, inode);
+ if (err)
+ goto out;
} else
nfs_readdirplus_parent_cache_hit(path->dentry);
- if (!err) {
- generic_fillattr(inode, stat);
- stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
- if (S_ISDIR(inode->i_mode))
- stat->blksize = NFS_SERVER(inode)->dtsize;
- }
+out_no_revalidate:
+ /* Only return attributes that were revalidated. */
+ stat->result_mask &= request_mask;
+out_no_update:
+ generic_fillattr(inode, stat);
+ stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
+ if (S_ISDIR(inode->i_mode))
+ stat->blksize = NFS_SERVER(inode)->dtsize;
out:
trace_nfs_getattr_exit(inode, err);
return err;
--
2.14.3
On Tue, Jan 09 2018, Trond Myklebust wrote:
> Support the query flags AT_STATX_FORCE_SYNC by forcing an attribute
> revalidation, and AT_STATX_DONT_SYNC by returning cached attributes
> only.
>
> Use the mask to optimise away server revalidation for attributes
> that are not being requested by the user.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> v2:
> - Respect the attribute cache timeout.
> - Unless AT_STATX_DONT_SYNC is set, we must only return attributes
> that have been duly revalidated.
This looks good, thanks.
It handles _FORCE_SYNC and _DONT_SYNC and avoids the flush if
CTIME/MTIME aren't requested.
I note that you no longer call nfs_need_revalidate_inode().
Most of the checks in there you have included in a different form,
but I don't see a test for NFS_INO_INVALID_LABEL. Is there a reason
that we don't need that test any more? Was it redundant previously?
Thanks,
NeilBrown
>
> fs/nfs/inode.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index b112002dbdb2..c2c8bb7755ad 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -735,12 +735,20 @@ int nfs_getattr(const struct path *path, struct kstat *stat,
> u32 request_mask, unsigned int query_flags)
> {
> struct inode *inode = d_inode(path->dentry);
> - int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
> + struct nfs_server *server = NFS_SERVER(inode);
> + unsigned long cache_validity;
> int err = 0;
> + bool force_sync = query_flags & AT_STATX_FORCE_SYNC;
> + bool do_update = false;
>
> trace_nfs_getattr_enter(inode);
> +
> + if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync)
> + goto out_no_update;
> +
> /* Flush out writes to the server in order to update c/mtime. */
> - if (S_ISREG(inode->i_mode)) {
> + if ((request_mask & (STATX_CTIME|STATX_MTIME)) &&
> + S_ISREG(inode->i_mode)) {
> err = filemap_write_and_wait(inode->i_mapping);
> if (err)
> goto out;
> @@ -757,24 +765,41 @@ int nfs_getattr(const struct path *path, struct kstat *stat,
> */
> if ((path->mnt->mnt_flags & MNT_NOATIME) ||
> ((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
> - need_atime = 0;
> -
> - if (need_atime || nfs_need_revalidate_inode(inode)) {
> - struct nfs_server *server = NFS_SERVER(inode);
> -
> + request_mask &= ~STATX_ATIME;
> +
> + /* Is the user requesting attributes that might need revalidation? */
> + if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> + STATX_MTIME|STATX_UID|STATX_GID|
> + STATX_SIZE|STATX_BLOCKS)))
> + goto out_no_revalidate;
> +
> + /* Check whether the cached attributes are stale */
> + do_update |= force_sync || nfs_attribute_cache_expired(inode);
> + cache_validity = READ_ONCE(NFS_I(inode)->cache_validity);
> + do_update |= cache_validity & NFS_INO_INVALID_ATTR;
> + if (request_mask & STATX_ATIME)
> + do_update |= cache_validity & NFS_INO_INVALID_ATIME;
> + if (request_mask & (STATX_CTIME|STATX_MTIME))
> + do_update |= cache_validity & NFS_INO_REVAL_PAGECACHE;
> + if (do_update) {
> + /* Update the attribute cache */
> if (!(server->flags & NFS_MOUNT_NOAC))
> nfs_readdirplus_parent_cache_miss(path->dentry);
> else
> nfs_readdirplus_parent_cache_hit(path->dentry);
> err = __nfs_revalidate_inode(server, inode);
> + if (err)
> + goto out;
> } else
> nfs_readdirplus_parent_cache_hit(path->dentry);
> - if (!err) {
> - generic_fillattr(inode, stat);
> - stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> - if (S_ISDIR(inode->i_mode))
> - stat->blksize = NFS_SERVER(inode)->dtsize;
> - }
> +out_no_revalidate:
> + /* Only return attributes that were revalidated. */
> + stat->result_mask &= request_mask;
> +out_no_update:
> + generic_fillattr(inode, stat);
> + stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> + if (S_ISDIR(inode->i_mode))
> + stat->blksize = NFS_SERVER(inode)->dtsize;
> out:
> trace_nfs_getattr_exit(inode, err);
> return err;
> --
> 2.14.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-01-10 at 13:33 +1100, NeilBrown wrote:
> On Tue, Jan 09 2018, Trond Myklebust wrote:
>
> > Support the query flags AT_STATX_FORCE_SYNC by forcing an attribute
> > revalidation, and AT_STATX_DONT_SYNC by returning cached attributes
> > only.
> >
> > Use the mask to optimise away server revalidation for attributes
> > that are not being requested by the user.
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > v2:
> > - Respect the attribute cache timeout.
> > - Unless AT_STATX_DONT_SYNC is set, we must only return attributes
> > that have been duly revalidated.
>
> This looks good, thanks.
> It handles _FORCE_SYNC and _DONT_SYNC and avoids the flush if
> CTIME/MTIME aren't requested.
>
> I note that you no longer call nfs_need_revalidate_inode().
> Most of the checks in there you have included in a different form,
> but I don't see a test for NFS_INO_INVALID_LABEL. Is there a reason
> that we don't need that test any more? Was it redundant previously?
It's a good question. NFS_INO_INVALID_LABEL is definitely not needed in
order to satisfy a stat() or statx() request. We wanted to be able to
revalidate the security label somewhere, but it doesn't really make
much sense to peg that to stat(), and I've never really been happy with
that decision.
We can add it back in for now.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]