On Wed, Jan 3, 2018 at 8:45 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 2018-01-03 at 17:48 +0200, Amir Goldstein wrote:
>> On Wed, Jan 3, 2018 at 5:41 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> > On Wed, 2018-01-03 at 17:14 +0200, Amir Goldstein wrote:
>> > > The time values in stat and inode may differ for overlayfs and stat time
>> > > values are the correct ones to use. This is also consistent with the fact
>> > > that fill_post_wcc() also stores stat time values.
>> > >
>> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>> > > ---
>> > > fs/nfsd/nfs3xdr.c | 31 ++++++++++++++++++++++++++++++-
>> > > fs/nfsd/nfs4xdr.c | 2 +-
>> > > fs/nfsd/nfsfh.h | 28 ++++++----------------------
>> > > 3 files changed, 37 insertions(+), 24 deletions(-)
>> > >
>> > > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
>> > > index 2758480555fa..1a70581e1cb2 100644
>> > > --- a/fs/nfsd/nfs3xdr.c
>> > > +++ b/fs/nfsd/nfs3xdr.c
>> > > @@ -251,6 +251,34 @@ encode_wcc_data(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
>> > > }
>> > >
>> > > /*
>> > > + * Fill in the pre_op attr for the wcc data
>> > > + */
>> > > +void fill_pre_wcc(struct svc_fh *fhp)
>> > > +{
>> > > + struct inode *inode;
>> > > + struct kstat stat;
>> > > + __be32 err;
>> > > +
>> > > + if (fhp->fh_pre_saved)
>> > > + return;
>> > > +
>> > > + inode = d_inode(fhp->fh_dentry);
>> > > + err = fh_getattr(fhp, &stat);
>> > > + if (err) {
>> > > + /* Grab the times from inode anyway */
>> > > + stat.mtime = inode->i_mtime;
>> > > + stat.ctime = inode->i_ctime;
>> > > + stat.size = inode->i_size;
>> > > + }
>> > > +
>> >
>> > Might be best to instead just not supply pre/post op attrs if the
>> > getattr fails? They are technically optional with v3 -- we can just set
>> > the attributes_follow bit to false there.
>>
>> I considered to set fh_pre_saved = false on error just like
>> fill_post_wcc() does, but wasn't sure of the consequences or how to test
>> for that matter, so I chose a more delicate fallback instead.
>>
>
> Take care with the BUG_ON in set_change_info if you do that.
>
> Note that all of this is really just to handle weak cache consistency in
> v3, and the change_info4 value in v4. When the info is not reliable, the
> client doesn't trust its cache, for the most part. Getting it wrong
> shouldn't be fatal in most cases.
>
> For v3 you can just clear the attributes_follow bit when you fill out
> the info, and leave it zeroed out. I had a patch a few years ago that
> did this on a per-export basis that you're welcome to take an run with:
>
> https://patchwork.kernel.org/patch/7159311/
>
> Obviously, the conditions for doing this here are different.
>
> For v4, I think we can just try to scrape what we have like you're doing
> here, and simply ensure that the "atomic" field being encoded in
> encode_cinfo is false when this occurs.
>
Honestly, Jeff, at this point I am so far out into the woods with overlay NFS
export, that I would like to remain focused on correctness and leave
performance for later time.
So if I understand you correctly, patch 2/2 is not needed for correctness?
Meaning that if overlay inode times are not uptodate, nothing fatal will
happen? Or did you mean that I must take care of signalling the client
that time values are not reliable for overlayfs?
If patch 2/2 is indeed not a must, then I would like to ask you to ACK
patch 1/2. It seems quite simple, trivial and harmless to me even without
diving deep into NFS protocols. I think patch 1/2 should be enough for
first implementation - it certainly is enough to fix the nfstest_posix failures.
Thanks!
Amir.
