On Thu, 26 Jan 2023, Jeff Layton wrote:
> On Wed, 2023-01-25 at 13:03 +1100, NeilBrown wrote:
> > NFSv3 includes pre/post wcc attributes which allow the client to
> > determine if all changes to the file have been made by itself, or any
> > might have been made by some other client. IF there are gaps in the
> > pre/post ctime sequence it must be assumed that some other client caused
> > that change in that gap and the local cache must be suspect. The next
> > time the file is opened, the cache should be invalidated.
> >
> > Since Commit 1c341b777501 ("NFS: Add deferred cache invalidation for
> > close-to-open consistency violations") (I think) in linux 5.3 the Linux
> > client has been triggering this invalidation. The chunk in
> > nfs_update_inode() in particularly triggers.
> >
> > Unfortunately Linux NFS assumes that all replies will be processed in
> > the order sent, and will arrive in the order processed. This is not
> > true in general. Linux might ignore the wcc info in a WRITE reply -
> > even though the pre-ctime might match the current iversion - because the
> > reply is for a request that was sent before some other request for which
> > a reply has already been seen. This is detected by Linux using the
> > gencount tests in nfs_inode_attr_cmp().
> >
> > Also, when the gencount tests pass it is still possible that the
> > requests were processed on the service in a different order, and a gap
> > in the ctime sequence might be filled in by a subsequent reply. Linux
> > NFS does not try to detect this.
> >
> > The net result is that writing to a server and then reading the file
> > back can result in going to the server for the read rather than serving
> > it from cache - all because a couple of replies arrived out-of-order.
> > This is a performance regression over kernels before 5.3 (though that
> > change is a correctness improvement).
> >
> > This has been seen with Linux writing to a Netapp server which can
> > occasionally re-order requests. I have also demonstrated it with a
> > modified Linux server which adds pre/post attributes to V3 write
> > replies, and occasionally adds a delay to force reply reordering.
> >
> > This patch attempts to address the problem by replacing the
> > NFS_INO_DATA_INVAL_DEFER flag with a small array of segments in the
> > ctime sequence which have not yet been seen in replies. Rather than
> > setting the flag, we add the missing segment to the array. Rather
> > than testing the flag, we check if the array is empty.
> >
> > The array contains A,B pairs when the local iversion is advanced from A
> > to B, and B,A pairs when a reply tells us that the ctime (aka the
> > change-id) has advanced from A to B on the server.
> > Overlapping segments are merged and inverted segments cancel (which
> > effectively comes to the same thing). Empty segments are removed.
> >
> > An expected reply (probably the common case) will have the server say
> > that ctime went from A to B, and Linux will update the iversion from A
> > to B. Rather than adding A,B and the B,A which will cancel out, we
> > simply don't bother.
> >
>
> So your cache remains intact as long as you're not checking validity
> before all of the replies come in.
Exactly - and we primarily check validity at 'open' (or lock).
>
> Clever! I like this idea.
Thanks :-)
>
> > One difficulty in this is that in some circumstances,
> > NFS_ATTR_FATTR_PRECHANGE is cleared. This causes the gap tracking to
> > fail because we can only add pre/post attributes to the table if bother
> > NFS_ATTR_FATTR_PRECHANGE and NFS_ATTR_FATTR_CHANGE are set. If one is
> > cleared, we have a problem.
> >
> > This patch works around that by introducing a new
> > NFS_ATTR_FATTR_PRECHANGE_RAW whcih is set when the normal PRECHANGE is
> > cleared. If either PRECHANGE or PRECHANGE_RAW a set then we can use the
> > prechange value. Obviously this is a hack. I would be keen to
> > understand why PRECHANGE is being cleared so I can find a better
> > resolution.
> >
> > I haven't given much thought to directories. A number of places call
> > nfs_set_cache_invalid(inode, NFS_INO_INVALID_DATA);
> >
> > only on directories and I don't understand why. As directories still
> > have pre/post attributes even in NFSv4 it would be good to understand
> > how to get this right.
> >
> > Any help would be most appreciated.
> >
> > Thanks,
> > NeilBrown
> >
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index f594dac436a7..8535372861e9 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -84,7 +84,7 @@ alloc_nfs_open_dir_context(struct inode *dir)
> > ctx->dtsize = NFS_INIT_DTSIZE;
> > spin_lock(&dir->i_lock);
> > if (list_empty(&nfsi->open_files) &&
> > - (nfsi->cache_validity & NFS_INO_DATA_INVAL_DEFER))
> > + nfs_ooo_test(nfsi))
> > nfs_set_cache_invalid(dir,
> > NFS_INO_INVALID_DATA |
> > NFS_INO_REVAL_FORCED);
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 6b2cfa59a1a2..4c9ad2e6586e 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -208,11 +208,12 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
> >
> > nfsi->cache_validity |= flags;
> >
> > - if (inode->i_mapping->nrpages == 0)
> > - nfsi->cache_validity &= ~(NFS_INO_INVALID_DATA |
> > - NFS_INO_DATA_INVAL_DEFER);
> > - else if (nfsi->cache_validity & NFS_INO_INVALID_DATA)
> > - nfsi->cache_validity &= ~NFS_INO_DATA_INVAL_DEFER;
> > + if (inode->i_mapping->nrpages == 0) {
> > + nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
> > + nfs_ooo_clear(nfsi);
> > + } else if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
> > + nfs_ooo_clear(nfsi);
> > + }
> > trace_nfs_set_cache_invalid(inode, 0);
> > }
> > EXPORT_SYMBOL_GPL(nfs_set_cache_invalid);
> > @@ -677,9 +678,10 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
> > trace_nfs_size_truncate(inode, offset);
> > i_size_write(inode, offset);
> > /* Optimisation */
> > - if (offset == 0)
> > - NFS_I(inode)->cache_validity &= ~(NFS_INO_INVALID_DATA |
> > - NFS_INO_DATA_INVAL_DEFER);
> > + if (offset == 0) {
> > + NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_DATA;
> > + nfs_ooo_clear(NFS_I(inode));
> > + }
> > NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_SIZE;
> >
> > spin_unlock(&inode->i_lock);
> > @@ -1101,7 +1103,7 @@ void nfs_inode_attach_open_context(struct nfs_open_context *ctx)
> >
> > spin_lock(&inode->i_lock);
> > if (list_empty(&nfsi->open_files) &&
> > - (nfsi->cache_validity & NFS_INO_DATA_INVAL_DEFER))
> > + nfs_ooo_test(nfsi))
> > nfs_set_cache_invalid(inode, NFS_INO_INVALID_DATA |
> > NFS_INO_REVAL_FORCED);
> > list_add_tail_rcu(&ctx->list, &nfsi->open_files);
> > @@ -1344,8 +1346,8 @@ int nfs_clear_invalid_mapping(struct address_space *mapping)
> >
> > set_bit(NFS_INO_INVALIDATING, bitlock);
> > smp_wmb();
> > - nfsi->cache_validity &=
> > - ~(NFS_INO_INVALID_DATA | NFS_INO_DATA_INVAL_DEFER);
> > + nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
> > + nfs_ooo_clear(nfsi);
> > spin_unlock(&inode->i_lock);
> > trace_nfs_invalidate_mapping_enter(inode);
> > ret = nfs_invalidate_mapping(inode, mapping);
> > @@ -1807,6 +1809,38 @@ static int nfs_inode_finish_partial_attr_update(const struct nfs_fattr *fattr,
> > return 0;
> > }
> >
> > +static void nfs_ooo_merge(struct nfs_inode *nfsi,
> > + u64 start, u64 end)
> > +{
> > + int i, cnt;
> > +
> > + /* add this range, merging if possible */
> > + cnt = nfsi->ooo_cnt;
> > + for (i = 0; i < cnt; i++) {
> > + if (end == nfsi->ooo[i][0])
> > + end = nfsi->ooo[i][1];
> > + else if (start == nfsi->ooo[i][1])
> > + start = nfsi->ooo[i][0];
> > + else
> > + continue;
> > + /* Remove 'i' from table and loop to insert the new range */
> > + cnt -= 1;
> > + nfsi->ooo[i][0] = nfsi->ooo[cnt][0];
> > + nfsi->ooo[i][1] = nfsi->ooo[cnt][1];
> > + i = -1;
> > + }
> > + if (start != end && cnt < ARRAY_SIZE(nfsi->ooo)) {
> > + nfsi->ooo[cnt][0] = start;
> > + nfsi->ooo[cnt][1] = end;
> > + cnt += 1;
> > + }
> > + if (cnt > nfsi->ooo_max) {
> > + printk("ooo_max -> %d\n", cnt);
> > + nfsi->ooo_max = cnt;
> > + }
> > + nfsi->ooo_cnt = cnt;
> > +}
> > +
> > static int nfs_refresh_inode_locked(struct inode *inode,
> > struct nfs_fattr *fattr)
> > {
> > @@ -1817,8 +1851,17 @@ static int nfs_refresh_inode_locked(struct inode *inode,
> >
> > if (attr_cmp > 0 || nfs_inode_finish_partial_attr_update(fattr, inode))
> > ret = nfs_update_inode(inode, fattr);
> > - else if (attr_cmp == 0)
> > - ret = nfs_check_inode_attributes(inode, fattr);
> > + else {
> > + if ((fattr->valid & NFS_ATTR_FATTR_CHANGE) &&
> > + (fattr->valid & (NFS_ATTR_FATTR_PRECHANGE |
> > + NFS_ATTR_FATTR_PRECHANGE_RAW)))
> > + nfs_ooo_merge(NFS_I(inode),
> > + fattr->change_attr,
> > + fattr->pre_change_attr);
> > +
> > + if (attr_cmp == 0)
> > + ret = nfs_check_inode_attributes(inode, fattr);
> > + }
> >
> > trace_nfs_refresh_inode_exit(inode, ret);
> > return ret;
> > @@ -1909,6 +1952,8 @@ int nfs_post_op_update_inode_force_wcc_locked(struct inode *inode, struct nfs_fa
> > if (attr_cmp < 0)
> > return 0;
> > if ((fattr->valid & NFS_ATTR_FATTR) == 0 || !attr_cmp) {
> > + if (fattr->valid & NFS_ATTR_FATTR_PRECHANGE)
> > + fattr->valid |= NFS_ATTR_FATTR_PRECHANGE_RAW;
> > fattr->valid &= ~(NFS_ATTR_FATTR_PRECHANGE
> > | NFS_ATTR_FATTR_PRESIZE
> > | NFS_ATTR_FATTR_PREMTIME
> > @@ -2084,8 +2129,13 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
> > dprintk("NFS: change_attr change on server for file %s/%ld\n",
> > inode->i_sb->s_id,
> > inode->i_ino);
> > - } else if (!have_delegation)
> > - nfsi->cache_validity |= NFS_INO_DATA_INVAL_DEFER;
> > + } else if (!have_delegation) {
> > + if (fattr->valid & NFS_ATTR_FATTR_PRECHANGE)
> > + nfs_ooo_merge(nfsi, fattr->change_attr,
> > + fattr->pre_change_attr);
> > + nfs_ooo_merge(nfsi, inode_peek_iversion_raw(inode),
> > + fattr->change_attr);
> > + }
> > inode_set_iversion_raw(inode, fattr->change_attr);
> > }
> > } else {
> > @@ -2239,6 +2289,8 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
> > return NULL;
> > nfsi->flags = 0UL;
> > nfsi->cache_validity = 0UL;
> > + nfsi->ooo_cnt = 0;
> > + nfsi->ooo_max = 0;
> > #if IS_ENABLED(CONFIG_NFS_V4)
> > nfsi->nfs4_acl = NULL;
> > #endif /* CONFIG_NFS_V4 */
> > diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> > index 8c6cc58679ff..cdf77f49ef8f 100644
> > --- a/fs/nfs/nfstrace.h
> > +++ b/fs/nfs/nfstrace.h
> > @@ -28,7 +28,6 @@
> > { NFS_INO_INVALID_MTIME, "INVALID_MTIME" }, \
> > { NFS_INO_INVALID_SIZE, "INVALID_SIZE" }, \
> > { NFS_INO_INVALID_OTHER, "INVALID_OTHER" }, \
> > - { NFS_INO_DATA_INVAL_DEFER, "DATA_INVAL_DEFER" }, \
> > { NFS_INO_INVALID_BLOCKS, "INVALID_BLOCKS" }, \
> > { NFS_INO_INVALID_XATTR, "INVALID_XATTR" }, \
> > { NFS_INO_INVALID_NLINK, "INVALID_NLINK" }, \
> > diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> > index 7931fa472561..8a83d6d204ed 100644
> > --- a/include/linux/nfs_fs.h
> > +++ b/include/linux/nfs_fs.h
> > @@ -190,6 +190,33 @@ struct nfs_inode {
> > /* Open contexts for shared mmap writes */
> > struct list_head open_files;
> >
> > + /* Keep track of out-of-order replies.
> > + * The ooo array contains start/end pairs of
> > + * number from the changeid sequence when
> > + * the inodes iversion has been updated.
> > + * It also contains end/start pair (i.e. reverse order)
> > + * of sections of the changeid sequence that have
> > + * been seen in replies from the server.
> > + * Normally these should match and when both
> > + * A:B and B:A are found in ooo, they are both removed.
> > + * And if a reply with A:B causes an iversion update
> > + * of A:B, then neither are added.
> > + * When a reply has pre_change that doesn't match
> > + * iversion, then the changeid pair, and any consequent
> > + * change in iversion ARE added. Later replies
> > + * might fill in the gaps, or possibly a gap is caused
> > + * by a change from another client.
> > + * When a file or directory is opened, if the ooo table
> > + * is not empty, then we assume the gaps were due to
> > + * another client and we invalidate the cached data.
> > + *
> > + * We can only track a limited number of concurrent gaps.
> > + * Currently that limit is 16.
> > + */
> > + int ooo_cnt;
> > + int ooo_max; // TRACING
> > + unsigned long ooo[16][2];
>
> Why unsigned longs here? Shouldn't these be u64?
Yes, they should be u64. Thanks.
>
> I guess you could argue that when we have 32-bit longs that the most
> significant bits don't matter, but that's also the case with 64-bit
> longs, and 32 bits would halve the space requirements.
>
> Also, this grows each inode by 2k on a 64-bit arch! Maybe we should
> dynamically allocate these things instead? If the allocation fails, then
> we could just go back to marking the cache invalid and move on.
2K? 8*16*2+4*2 == 264, not 2048.
But I agree that allocating on demand would make sense. 16 is probably
more than needed. I don't have proper testing results from the customer
yet, but I wouldn't be surprised if a smaller number would suffice. But
if we are allocating only on demand, it wouldn't hurt to allocate 16.
Thanks,
NeilBrown
>
> > +
> > #if IS_ENABLED(CONFIG_NFS_V4)
> > struct nfs4_cached_acl *nfs4_acl;
> > /* NFSv4 state */
> > @@ -253,8 +280,6 @@ struct nfs4_copy_state {
> > #define NFS_INO_INVALID_MTIME BIT(10) /* cached mtime is invalid */
> > #define NFS_INO_INVALID_SIZE BIT(11) /* cached size is invalid */
> > #define NFS_INO_INVALID_OTHER BIT(12) /* other attrs are invalid */
> > -#define NFS_INO_DATA_INVAL_DEFER \
> > - BIT(13) /* Deferred cache invalidation */
> > #define NFS_INO_INVALID_BLOCKS BIT(14) /* cached blocks are invalid */
> > #define NFS_INO_INVALID_XATTR BIT(15) /* xattrs are invalid */
> > #define NFS_INO_INVALID_NLINK BIT(16) /* cached nlinks is invalid */
> > @@ -615,6 +640,19 @@ nfs_fileid_to_ino_t(u64 fileid)
> > return ino;
> > }
> >
> > +static inline void nfs_ooo_clear(struct nfs_inode *nfsi)
> > +{
> > + nfsi->ooo_cnt = 0;
> > +}
> > +
> > +static inline bool nfs_ooo_test(struct nfs_inode *nfsi)
> > +{
> > + if (nfsi->ooo_cnt == 0)
> > + return false;
> > + printk("nfs_ooo_test_and_clear() found %d\n", nfsi->ooo_cnt);
> > + return true;
> > +}
> > +
> > #define NFS_JUKEBOX_RETRY_TIME (5 * HZ)
> >
> > /* We need to block new opens while a file is being unlinked.
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index e86cf6642d21..b18a877b16eb 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -106,6 +106,7 @@ struct nfs_fattr {
> > #define NFS_ATTR_FATTR_OWNER_NAME (1U << 23)
> > #define NFS_ATTR_FATTR_GROUP_NAME (1U << 24)
> > #define NFS_ATTR_FATTR_V4_SECURITY_LABEL (1U << 25)
> > +#define NFS_ATTR_FATTR_PRECHANGE_RAW (1U << 26)
> >
> > #define NFS_ATTR_FATTR (NFS_ATTR_FATTR_TYPE \
> > | NFS_ATTR_FATTR_MODE \
> >
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
