On Thu, 03 Feb 2022, inoguchi.yuki@xxxxxxxxxxx wrote:
> Thank you for the review.
>
> > I would make 2 changes.
> > 1/ invalidate when opening a file, rather than when closing.
> > This fits better with the understanding of close-to-open consistency
> > that we flush writes when closing and verify the cache when opening
> > 2/ I would be more careful about determining exactly when the
> > invalidation might be needed.
>
> Yes, I'm willing to make these changes.
>
> > In nfs_post_op_updatE_inode() there is the code:
> >
> > if ((fattr->valid & NFS_ATTR_FATTR_CHANGE) != 0 &&
> > (fattr->valid & NFS_ATTR_FATTR_PRECHANGE) == 0) {
> > fattr->pre_change_attr = inode_peek_iversion_raw(inode);
> > fattr->valid |= NFS_ATTR_FATTR_PRECHANGE;
> > }
>
> You mean nfs_post_op_update_inode_force_wcc_locked(), not nfs_post_op_update_inode(), right?
> This is just make sure --- so I can set the new flag in appropriate place :)
Yes, you are correct.
>
> > I assume that code doesn't end up running when you write to a file for
> > which you have a delegation, but I'm not at all certain - we would have
> > to check.
>
> Maybe it is nfs_check_inode_attributes()?
> It returns without doing anything if you have a delegation.
> It is called from:
> nfs_post_op_update_inode_force_wcc_locked()
> -> nfs_post_op_update_inode_locked()
> -> nfs_refresh_inode_locked()
> -> nfs_check_inode_attributes()
>
> 1476 static int nfs_check_inode_attributes(struct inode *inode, struct nfs_fattr *fattr)
> 1477 {
> 1478 struct nfs_inode *nfsi = NFS_I(inode);
> 1479 loff_t cur_size, new_isize;
> 1480 unsigned long invalid = 0;
> 1481 struct timespec64 ts;
> 1482
> 1483 if (NFS_PROTO(inode)->have_delegation(inode, FMODE_READ))
> 1484 return 0;
>
I don't *think* that is relevant. If you set the new flag where I
suggested, that code will already have run before it gets to the
have_delegation test.
nfs4_write_need_cache_consistency_data() seems relevant.
If that returns false (which it does when there is a delegation) the
WRITE request doesn't even ask for attributes.
In that case if nfs_post_op_update_inode_force_wcc_locked() is called,
it will find that NFS_ATTR_FATTR_CHANGE is not set, so it won't try to
set ->pre_change_attr with a "fake" value, and so won't set the new flag.
NeilBrown
