Re: [PATCH 6/8] nfsd: move change attribute generation to filesystem

From: "J. Bruce Fields" <bfields@redhat.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Daire Byrne <daire@dneg.com>,
	Trond Myklebust <trondmy@hammerspace.com>,
	linux-cachefs <linux-cachefs@redhat.com>,
	linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 6/8] nfsd: move change attribute generation to filesystem
Date: Fri, 20 Nov 2020 20:01:28 -0500	[thread overview]
Message-ID: <20201121010128.GK136478@pick.fieldses.org> (raw)
In-Reply-To: <6c2bb6ae1ad586d6bf369afe561b77b271cd2ac8.camel@kernel.org>

On Fri, Nov 20, 2020 at 07:58:38PM -0500, Jeff Layton wrote:
> On Fri, 2020-11-20 at 17:39 -0500, J. Bruce Fields wrote:
> > diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> > index 465fd9e048d4..b950fac3d7df 100644
> > --- a/fs/xfs/xfs_export.c
> > +++ b/fs/xfs/xfs_export.c
> > @@ -16,6 +16,7 @@
> >  #include "xfs_inode_item.h"
> >  #include "xfs_icache.h"
> >  #include "xfs_pnfs.h"
> > +#include <linux/iversion.h>
> >  
> > 
> > 
> > 
> >  /*
> >   * Note that we only accept fileids which are long enough rather than allow
> > @@ -234,4 +235,5 @@ const struct export_operations xfs_export_operations = {
> >  	.map_blocks		= xfs_fs_map_blocks,
> >  	.commit_blocks		= xfs_fs_commit_blocks,
> >  #endif
> > +	.fetch_iversion		= generic_fetch_iversion,
> >  };
> 
> It seems a little weird to call a static inline here. I imagine that
> means the compiler has to add a duplicate inline in every .o file that
> does this? It may be cleaner to move generic_fetch_iversion into
> fs/libfs.c so we only have one copy of it.

OK.

(To be honest, I was a little suprised this worked.)

--b.

> 
> > diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> > index 3bfebde5a1a6..ded74523c8a6 100644
> > --- a/include/linux/iversion.h
> > +++ b/include/linux/iversion.h
> > @@ -328,6 +328,32 @@ inode_query_iversion(struct inode *inode)
> >  	return cur >> I_VERSION_QUERIED_SHIFT;
> >  }
> >  
> > 
> > 
> > 
> > +/*
> > + * We could use i_version alone as the NFSv4 change attribute.  However,
> > + * i_version can go backwards after a reboot.  On its own that doesn't
> > + * necessarily cause a problem, but if i_version goes backwards and then
> > + * is incremented again it could reuse a value that was previously used
> > + * before boot, and a client who queried the two values might
> > + * incorrectly assume nothing changed.
> > + *
> > + * By using both ctime and the i_version counter we guarantee that as
> > + * long as time doesn't go backwards we never reuse an old value.
> > + *
> > + * A filesystem that has an on-disk boot counter or similar might prefer
> > + * to use that to avoid the risk of the change attribute going backwards
> > + * if system time is set backwards.
> > + */
> > +static inline u64 generic_fetch_iversion(struct inode *inode)
> > +{
> > +	u64 chattr;
> > +
> > +	chattr =  inode->i_ctime.tv_sec;
> > +	chattr <<= 30;
> > +	chattr += inode->i_ctime.tv_nsec;
> > +	chattr += inode_query_iversion(inode);
> > +	return chattr;
> > +}
> > +
> >  /*
> >   * For filesystems without any sort of change attribute, the best we can
> >   * do is fake one up from the ctime:
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 