On Thu, 2020-10-01 at 14:25 -0400, Olga Kornievskaia wrote:
> On Thu, Oct 1, 2020 at 1:14 PM Trond Myklebust <
> trondmy@xxxxxxxxxxxxxxx> wrote:
> > Hi Olga,
> >
> > On Mon, 2020-09-14 at 17:05 -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > >
> > > Client uses static bitmask for GETATTR on CLOSE/WRITE/DELEGRETURN
> > > and ignores the fact that it might have some attributes marked
> > > invalid in its cache. Compared to v3 where all attributes are
> > > retrieved in postop attributes, v4's cache is frequently out of
> > > sync and leads to standalone GETATTRs being sent to the server.
> > >
> > > Instead, in addition to the minimum cache consistency attributes
> > > also check cache_validity and adjust the GETATTR request
> > > accordingly.
> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > > ---
> > > fs/nfs/nfs4proc.c | 45
> > > ++++++++++++++++++++++++++++++++++++++-
> > > --
> > > include/linux/nfs_xdr.h | 6 +++---
> > > 2 files changed, 45 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index 6e95c85fe395..d7434a3697d9 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -107,6 +107,9 @@ static int nfs41_test_stateid(struct
> > > nfs_server
> > > *, nfs4_stateid *,
> > > static int nfs41_free_stateid(struct nfs_server *, const
> > > nfs4_stateid *,
> > > const struct cred *, bool);
> > > #endif
> > > +static void nfs4_bitmask_adjust(__u32 *bitmask, struct inode
> > > *inode,
> > > + struct nfs_server *server,
> > > + struct nfs4_label *label);
> > >
> > > #ifdef CONFIG_NFS_V4_SECURITY_LABEL
> > > static inline struct nfs4_label *
> > > @@ -3632,9 +3635,10 @@ static void nfs4_close_prepare(struct
> > > rpc_task
> > > *task, void *data)
> > >
> > > if (calldata->arg.fmode == 0 || calldata->arg.fmode ==
> > > FMODE_READ) {
> > > /* Close-to-open cache consistency revalidation */
> > > - if (!nfs4_have_delegation(inode, FMODE_READ))
> > > + if (!nfs4_have_delegation(inode, FMODE_READ)) {
> > > calldata->arg.bitmask = NFS_SERVER(inode)-
> > > > cache_consistency_bitmask;
> > > - else
> > > + nfs4_bitmask_adjust(calldata->arg.bitmask,
> > > inode, NFS_SERVER(inode), NULL);
> > > + } else
> > > calldata->arg.bitmask = NULL;
> > > }
> > >
> > > @@ -5360,6 +5364,38 @@ bool
> > > nfs4_write_need_cache_consistency_data(struct nfs_pgio_header
> > > *hdr)
> > > return nfs4_have_delegation(hdr->inode, FMODE_READ) == 0;
> > > }
> > >
> > > +static void nfs4_bitmask_adjust(__u32 *bitmask, struct inode
> > > *inode,
> > > + struct nfs_server *server,
> > > + struct nfs4_label *label)
> > > +{
> > > +
> > > + unsigned long cache_validity = READ_ONCE(NFS_I(inode)-
> > > > cache_validity);
> > > +
> > > + if ((cache_validity & NFS_INO_INVALID_DATA) ||
> > > + (cache_validity & NFS_INO_REVAL_PAGECACHE) ||
> > > + (cache_validity & NFS_INO_REVAL_FORCED) ||
> > > + (cache_validity & NFS_INO_INVALID_OTHER))
> > > + nfs4_bitmap_copy_adjust(bitmask,
> > > nfs4_bitmask(server,
> > > label), inode);
> > > +
> > > + if (cache_validity & NFS_INO_INVALID_ATIME)
> > > + bitmask[1] |= FATTR4_WORD1_TIME_ACCESS;
> > > + if (cache_validity & NFS_INO_INVALID_ACCESS)
> > > + bitmask[0] |= FATTR4_WORD1_MODE |
> > > FATTR4_WORD1_OWNER |
> > > + FATTR4_WORD1_OWNER_GROUP;
> > > + if (cache_validity & NFS_INO_INVALID_ACL)
> > > + bitmask[0] |= FATTR4_WORD0_ACL;
> > > + if (cache_validity & NFS_INO_INVALID_LABEL)
> > > + bitmask[2] |= FATTR4_WORD2_SECURITY_LABEL;
> > > + if (cache_validity & NFS_INO_INVALID_CTIME)
> > > + bitmask[0] |= FATTR4_WORD0_CHANGE;
> > > + if (cache_validity & NFS_INO_INVALID_MTIME)
> > > + bitmask[1] |= FATTR4_WORD1_TIME_MODIFY;
> > > + if (cache_validity & NFS_INO_INVALID_SIZE)
> > > + bitmask[0] |= FATTR4_WORD0_SIZE;
> > > + if (cache_validity & NFS_INO_INVALID_BLOCKS)
> > > + bitmask[1] |= FATTR4_WORD1_SPACE_USED;
> >
> > If we hold a delegation (which we could do when called
> > from nfs4_proc_write_setup()) then we only want to get extra
> > attributes
> > if the NFS_INO_REVAL_FORCED flag is also set.
>
> If we hold a delegation then nfs4_write_need_cache_consistency_data()
> would be true and no getattr would be added (or need to be adjusted).
> Am I mis-reading your comment?
No, you are absolutely right, and I did miss that. In that case I
believe this should be ready to merge as is.
>
> > > +}
> > > +
> > > static void nfs4_proc_write_setup(struct nfs_pgio_header *hdr,
> > > struct rpc_message *msg,
> > > struct rpc_clnt **clnt)
> > > @@ -5369,8 +5405,10 @@ static void nfs4_proc_write_setup(struct
> > > nfs_pgio_header *hdr,
> > > if (!nfs4_write_need_cache_consistency_data(hdr)) {
> > > hdr->args.bitmask = NULL;
> > > hdr->res.fattr = NULL;
> > > - } else
> > > + } else {
> > > hdr->args.bitmask = server-
> > > >cache_consistency_bitmask;
> > > + nfs4_bitmask_adjust(hdr->args.bitmask, hdr->inode,
> > > server, NULL);
> > > + }
> > >
> > > if (!hdr->pgio_done_cb)
> > > hdr->pgio_done_cb = nfs4_write_done_cb;
> > > @@ -6406,6 +6444,7 @@ static int _nfs4_proc_delegreturn(struct
> > > inode
> > > *inode, const struct cred *cred,
> > > data->args.fhandle = &data->fh;
> > > data->args.stateid = &data->stateid;
> > > data->args.bitmask = server->cache_consistency_bitmask;
> > > + nfs4_bitmask_adjust(data->args.bitmask, inode, server,
> > > NULL);
> > > nfs_copy_fh(&data->fh, NFS_FH(inode));
> > > nfs4_stateid_copy(&data->stateid, stateid);
> > > data->res.fattr = &data->fattr;
> > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > > index 9408f3252c8e..bafbf6695796 100644
> > > --- a/include/linux/nfs_xdr.h
> > > +++ b/include/linux/nfs_xdr.h
> > > @@ -525,7 +525,7 @@ struct nfs_closeargs {
> > > struct nfs_seqid * seqid;
> > > fmode_t fmode;
> > > u32 share_access;
> > > - const u32 * bitmask;
> > > + u32 * bitmask;
> > > struct nfs4_layoutreturn_args *lr_args;
> > > };
> > >
> > > @@ -608,7 +608,7 @@ struct nfs4_delegreturnargs {
> > > struct nfs4_sequence_args seq_args;
> > > const struct nfs_fh *fhandle;
> > > const nfs4_stateid *stateid;
> > > - const u32 * bitmask;
> > > + u32 * bitmask;
> > > struct nfs4_layoutreturn_args *lr_args;
> > > };
> > >
> > > @@ -648,7 +648,7 @@ struct nfs_pgio_args {
> > > union {
> > > unsigned int replen; /*
> > > used by read */
> > > struct {
> > > - const u32 * bitmask; /*
> > > used by write */
> > > + u32 * bitmask; /*
> > > used by write */
> > > enum nfs3_stable_how stable; /*
> > > used by write */
> > > };
> > > };
> >
> > Otherwise this looks good. Thanks!
> >
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
