Re: [PATCH 14/13] xfs: make XFS_IOC_COMMIT_RANGE freshness data opaque

Darrick J. Wong
2024-02-29T23:27:25Z

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@kernel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > 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.
>