On Fri, Mar 1, 2024 at 1:27 AM Darrick J. Wong <djwong@xxxxxxxxxx> 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 wish I had thought of it myself - it is a good idea - just did not occur to me. Using the language of i_changecounter, that is "the current xfs implementation of i_version", I still think that using it as the content of the opaque freshness blob makes more sense than mtime+ctime, but it is none of my concern what you decide to fill in the freshness blob for the first version. I was not aware of the way xfs_fsr is currently using mtime+ctime when I replied and I am not sure if and how it is relevant to the new API. > 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?" > Yeh, I should have chosen my words more carefully. I was perfectly aware of your lack of interest in doing extra work and wasn't trying to request any. > 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. > Can't speak for everyone else, but coming from the middle east, I have fewer politeness filters. When I write "wouldn't it be better to use change_cookie?" I am just asking that question. When I am asking something to be changed before merge, I try to be much more explicit about it and this is what I expect others to do when reviewing my patches. Thanks, Amir.
