| Message ID | 20180109162353.10698-1-trond.myklebust@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <trond.myklebust@primarydata.com> > --- > 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 majordomo@vger.kernel.org > 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 <trond.myklebust@primarydata.com> > > --- > > 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.
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;
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 <trond.myklebust@primarydata.com> --- 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(-)