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:   Thu, 21 Jan 2021 15:27:56 -0500
From:   "J. Bruce Fields" <bfields@redhat.com>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Trond Myklebust <trondmy@hammerspace.com>,
        Anna Schumaker <schumakeranna@gmail.com>,
        Chuck Lever <chuck.lever@oracle.com>
Subject: Re: [PATCH 2/3] nfsd: move change attribute generation to filesystem
Message-ID: <20210121202756.GA13298@pick.fieldses.org>
References: <1611084297-27352-1-git-send-email-bfields@redhat.com>
 <1611084297-27352-3-git-send-email-bfields@redhat.com>
 <20210120084638.GA3678536@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210120084638.GA3678536@infradead.org>
Precedence: bulk

On Wed, Jan 20, 2021 at 08:46:38AM +0000, Christoph Hellwig wrote:
> On Tue, Jan 19, 2021 at 02:24:56PM -0500, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > After this, only filesystems lacking change attribute support will leave
> > the fetch_iversion export op NULL.
> > 
> > This seems cleaner to me, and will allow some minor optimizations in the
> > nfsd code.
> 
> Another indirect call just to fetch the change attribute (which happens
> a lot IIRC) does not seem very optimal to me.

In the next patch we're removing an fh_getattr (vfs_getattr) in the case
we call the new op, so that's typically a net decrease in indirect
calls.

Though maybe we could use a flag here and do without either.

> And the fact that we need three duplicate implementations also is not
> very nice.

Ext4 and xfs are identical, btrfs is a little different since it doesn't
consult I_VERSION.  (And then there's nfs, which uses the origin
server's i_version if it can.)

I also have a vague idea that some filesystem-specific improvements
might be possible.  (E.g., if a filesystem had some kind of boot count
in the superblock, maybe that would be a better way to prevent the
change attribute from going backwards on reboot than the thing
generic_fetch_iversion is currently doing with ctime.  But I have no
concrete plan there, maybe I'm dreaming.)

--b.