Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 20 Nov 2020 20:01:28 -0500
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
Message-ID: <20201121010128.GK136478@pick.fieldses.org>
References: <20201120223831.GB7705@fieldses.org>
 <1605911960-12516-1-git-send-email-bfields@redhat.com>
 <1605911960-12516-6-git-send-email-bfields@redhat.com>
 <6c2bb6ae1ad586d6bf369afe561b77b271cd2ac8.camel@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6c2bb6ae1ad586d6bf369afe561b77b271cd2ac8.camel@kernel.org>
Precedence: bulk

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>
>