On Fri, Mar 01, 2024 at 08:31:21AM -0500, Jeff Layton wrote: > On Thu, 2024-02-29 at 15:27 -0800, Darrick J. Wong wrote: > > On Tue, Feb 27, 2024 at 08:52:58PM +0200, Amir Goldstein wrote: > > > On Tue, Feb 27, 2024 at 7:46 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > > > > > > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > > > > > To head off bikeshedding about the fields in xfs_commit_range, let's > > > > make it an opaque u64 array and require the userspace program to call > > > > a third ioctl to sample the freshness data for us. If we ever converge > > > > on a definition for i_version then we can use that; for now we'll just > > > > use mtime/ctime like the old swapext ioctl. > > > > > > This addresses my concerns about using mtime/ctime. > > > > Oh good! :) > > > > > I have to say, Darrick, that I think that referring to this concern as > > > bikeshedding is not being honest. > > > > > > I do hate nit picking reviews and I do hate "maybe also fix the world" > > > review comments, but I think the question about using mtime/ctime in > > > this new API was not out of place > > > > I agree, your question about mtime/ctime: > > > > "Maybe a stupid question, but under which circumstances would mtime > > change and ctime not change? Why are both needed?" > > > > was a very good question. But perhaps that statement referred to the > > other part of that thread. > > > > > and I think that making the freshness > > > data opaque is better for everyone in the long run and hopefully, this will > > > help you move to the things you care about faster. > > > > I wish you'd suggested an opaque blob that the fs can lay out however it > > wants instead of suggesting specifically the change cookie. I'm very > > much ok with an opaque freshness blob that allows future flexibility in > > how we define the blob's contents. > > > > I was however very upset about the Jeff's suggestion of using i_version. > > I apologize for using all caps in that reply, and snarling about it in > > the commit message here. The final version of this patch will not have > > that. > > > > That said, I don't think it is at all helpful to suggest using a file > > attribute whose behavior is as yet unresolved. Multigrain timestamps > > were a clever idea, regrettably reverted. As far as I could tell when I > > wrote my reply, neither had NFS implemented a better behavior and > > quietly merged it; nor have Jeff and Dave produced any sort of candidate > > patchset to fix all the resulting issues in XFS. > > > > Reading "I realize that STATX_CHANGE_COOKIE is currently kernel > > internal" made me think "OH $deity, they wants me to do that work > > too???" > > > > A better way to have woreded that might've been "How about switching > > this to a fs-determined structure so that we can switch the freshness > > check to i_version when that's fully working on XFS?" > > > > The problem I have with reading patch review emails is that I can't > > easily tell whether an author's suggestion is being made in a casual > > offhand manner? Or if it reflects something they feel strongly needs > > change before merging. > > > > In fairness to you, Amir, I don't know how much you've kept on top of > > that i_version vs. XFS discussion. So I have no idea if you were aware > > of the status of that work. > > > > Sorry, I didn't mean to trigger anyone, but I do have real concerns > about any API that attempts to use timestamps to detect whether > something has changed. > > We learned that lesson in NFS in the 90's. VFS timestamp resolution is > just not enough to show whether there was a change to a file -- full > stop. > > I get the hand-wringing over i_version definitions and I don't care to > rehash that discussion here, but I'll point out that this is a > (proposed) XFS-private interface: > > What you could do is expose the XFS change counter (the one that gets > bumped for everything, even atime updates, possibly via different > ioctl), and use that for your "freshness" check. > > You'd unfortunately get false negative freshness checks after read > operations, but you shouldn't get any false positives (which is real > danger with timestamps). I don't see how would that work for this usecase? You have to sample file2 before reflinking file2's contents to file1, writing the changes to file1, and executing COMMIT_RANGE. Setting the xfs-private REFLINK inode flag on file2 will trigger an iversion update even though it won't change mtime or ctime. The COMMIT then fails due to the inode flags change. Worse yet, applications aren't going to know if a particular access is actually the one that will trigger an atime update. So this will just fail unpredictably. If iversion was purely a write counter then I would switch the freshness implementation to use it. But it's not, and I know this to be true because I tried that and could not get COMMIT_RANGE to work reliably. I suppose the advantage of the blob thing is that we actually /can/ switch over whenever it's ready. --D > -- > Jeff Layton <jlayton@xxxxxxxxxx> >
