Re: [PATCH v2 2/2] nfsd: store stat times in fill_pre_wcc() instead of inode times

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jan 3, 2018 at 5:41 PM, Jeff Layton <jlayton@kernel.org> 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@gmail.com>
>> ---
>>  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.


>
> For v4, it's a little more complicated. Scraping it out of the inode
> might be necessary for the cases where we need a change_info4.
>
> Either way, it'd be good to know that these getattrs are failing if this
> occurs. Maybe a KERN_WARNING printk or something might be good there?
>

As you wish, but fill_post_wcc() does not emit a warning, so...

I'll wait for feedback from Bruce as well.
Let me know the error handing of your choice.

Thanks,
Amir.