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. --D > Thanks, > Amir. >
