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, 22 Jan 2021 13:46:48 -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: <20210122184648.GD52753@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>
 <20210121202756.GA13298@pick.fieldses.org>
 <20210122082059.GA119852@infradead.org>
 <20210122144753.GA52753@pick.fieldses.org>
 <20210122173248.GB241302@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210122173248.GB241302@infradead.org>
Precedence: bulk

On Fri, Jan 22, 2021 at 05:32:48PM +0000, Christoph Hellwig wrote:
> 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.

Plain old nfs write also bumps the change attribute.

> More importanly ctime will
> also be lost as i_version and the ctime are commited together.

That's not the reason for using ctime.

The ctime is so that change attributes due to new changes that happen
after the boot will not collide with change attributes used before.

So:

	0. file has change attribute i.
	1. write bumps change attribute to i+1.
	2. client fetches new data and change attribute i+1.
	3. server crashes and comes back up.
	4. a new write with different data bumps change attribute to i+1.
	5. client fetches change attribute again, incorrectly concludes
	   its data cache is still correct.

Including the ctime ensures the change attributes at step 2 and 4 are no
longer the same.

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

Those matter too, of course.

--b.