Re: [PATCH] fs: use __fput_sync in close(2)

[Mateusz Guzik]

On 8/8/23, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>> I don't think perf tax on something becomes more sensible the longer
>> it is there.
>
> One does need to answer the question why it does suddenly become
> relevant after all these years though.
>

There is some work I'm considering doing, but before that happens I'm
sanity checking performance of various syscalls and I keep finding
problems, some of which are trivially avoidable.

I'm genuinely confused with the strong opposition to the very notion
of making close(2) a special case (which I consider conceptually
trivial), but as you noted below I'm not ultimately the person on the
hook for any problems.

> The original discussion was triggered by fifo ordering in task work
> which led to a noticable regression and why it was ultimately reverted.
> The sync proposal for fput() was an orthogonal proposal and the
> conclusion was that it wasn't safe generally
> https://lore.kernel.org/all/20150905051915.GC22011@xxxxxxxxxxxxxxxxxx
> even though it wasn't a direct response to the patch you linked.
>

Ok, I missed this e-mail. It further discourages patching filp_close,
but does not make an argument against *just* close(2) rolling with
sync which is what I'm proposing.

> If you care about it enough send a patch that just makes close(2) go
> sync.

But this is precisely what the submitted patch is doing. It adds
file_fput_sync, then adds close_fd_sync which is the only consumer and
only makes close(2) use it. *nobody* else has sync added.

One can argue the way this is sorted out is crap and I'm not going to
defend it. I am saying making *just* close(2) roll with sync is very
easy, there are numerous ways to do it and anyone involved with
maintaining vfs can write their own variant in minutes. Basically I
don't see *technical* problems here.

> We'll stuff it in a branch and we'll see what LKP has to say about
> it or whether this gets lost in noise. I really don't think letting
> micro-benchmarks become a decisive factor for code churn is a good
> idea.
>

That would be nice. Given the patch is already doing what you asked,
can you just take it as is?

I'll note though what I mentioned elsewhere
(https://lore.kernel.org/all/CAGudoHEG7vtCRWjn0yR5LMUsaw3KJANfa+Hkke9gy0imXQz6tg@xxxxxxxxxxxxxx/):
can they make sure to whack CONFIG_RANDOMIZE_KSTACK_OFFSET=y from
their kernel config? It is an *optional* measure and it comes at a
massive premium, so single-threaded changes are easily diminished.

-- 
Mateusz Guzik <mjguzik gmail.com>