Re: [PATCH 2/3] nfsd: move change attribute generation to filesystem

From: Christoph Hellwig <hch@infradead.org>
To: "J. Bruce Fields" <bfields@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	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
Date: Fri, 22 Jan 2021 17:32:48 +0000	[thread overview]
Message-ID: <20210122173248.GB241302@infradead.org> (raw)
In-Reply-To: <20210122144753.GA52753@pick.fieldses.org>

On Fri, Jan 22, 2021 at 09:47:53AM -0500, J. Bruce Fields wrote:
> > > 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.)
> > 
> > Even without the ctime i_version never goes backward, what is the
> > problem here?
> 
> Suppose a modification bumps the change attribute, a client reads
> the new value of the change attribute before it's committed to disk,
> then the server crashes.  After the server comes back up, the client
> requests the change attribute again and sees an older value.

So all metadata operations kicked off by nfsd are synchronous due
to ->commit_metadata/sync_inode_metadata, so this could only happen
for operations not kicked off by nfsd.  More importanly ctime will
also be lost as i_version and the ctime are commited together.

> 
> That's actually not too bad.  What I'd mainly like to avoid is
> incrementing the change attribute further and risking reuse of an old
> value for a different new state of the file.

Ok but that is an issue if we need to deal with changes that did not
come in through NFSD.