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

[Mateusz Guzik]

On 8/8/23, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> It adds two new exports of filp_close_sync() and close_fd_sync() without
> any users. That's not something we do and we also shouldn't encourage
> random drivers to switch to sync behavior.
>

They don't need to be exported for the patch to work.

> That rseq thing is completely orthogonal and maybe that needs to be
> fixed and you can go and convince the glibc people to do it.
>

This is not a glibc problem, but rseq problem -- it should not be
lumped into any case which uses task_work_add.

> And making filp_close() fully sync again is also really not great.

The patch is not doing it.

> Simplicity wins out and if all codepaths share the same behavior we're
> better off then having parts use task work and other parts just not.
>

The difference is not particularly complicated.

> Yes, we just did re-added the f_pos optimization because it may have had
> an impact. And that makes more sense because that was something we had
> changed just a few days/weeks before.
>

I don't think perf tax on something becomes more sensible the longer
it is there.

> But this is over 10 year old behavior and this micro benchmarking isn't
> a strong selling point imho. We could make close(2) go sync, sure. But
> I'm skeptical even about this without real-world data or from a proper
> testsuite.
>

I responded to this in my mail to Eric.

> (There's also a stray sysctl_fput_sync there which is scary to think that
> we'd ever allow a sysctl that allows userspace to control how we close
> fds.)
>

This is a leftover from my tests -- I added a runtime switch so can I
flip back and forth, most definitely not something I would expect to
be shipped.

-- 
Mateusz Guzik <mjguzik gmail.com>