On Tue, Aug 08, 2023 at 09:57:04AM -0700, Linus Torvalds wrote:
> On Mon, 7 Aug 2023 at 22:57, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
> >
> > Taking a quick look at the history it appears that fput was always
> > synchronous [..]
>
> Indeed. Synchronous used to be the only case.
>
> The reason it's async now is because several drivers etc do the final
> close from nasty contexts, so 'fput()' needed to be async for the
> general case.
>
> > All 3 issues taken together says that a synchronous fput is a
> > loaded foot gun that must be used very carefully. That said
> > close(2) does seem to be a reliably safe place to be synchronous.
>
> Yes.
>
> That said, I detest Mateusz' patch. I hate these kinds of "do
> different things based on flags" interfaces. Particularly when it
> spreads out like this.
>
> So I do like having close() be synchronous, because we actually do
> have correctness issues wrt the close having completed properly by the
> time we return to user space, so we have that "task_work_add()" there
> that will force the synchronization anyway before we return.
>
> So the system call case is indeed a special case. Arguably
> close_range() could be too, but honestly, once you start doing ranges
> of file descriptors, you are (a) doint something fairly unusual, and
> (b) the "queue them up on the task work" might actually be a *good*
> thing.
>
> It's definitely not a good thing for the single-fd-close case, though.
>
> But even if we want to do this - and I have absolutely no objections
> to it conceptually as per above - we need to be a lot more surgical
> about it, and not pass stupid flags around.
>
> Here's a TOTALLY UNTESTED(!) patch that I think effectively does what
> Mateusz wants done, but does it all within just fs/open.c and only for
> the obvious context of the close() system call itself.
>
> All it needs is to just split out the "flush" part from filp_close(),
> and we already had all the other infrastructure for this operation.
>
> Mateusz, two questions:
>
> (a) does this patch work for you?
>
> (b) do you have numbers for this all?
I really would like to have good ways of testing the impact of such
things because I'm a little scared of endless optimization patches that
overall either complicate or uglify our code. Maybe I'm paranoid, maybe
that's dumb but it worries me.
>
> and if it all looks good I have no problems with this kind of much
> more targeted and obvious patch.
>
> Again: TOTALLY UNTESTED. It looks completely obvious, but mistakes happen.
I think you're at least missing the removal of the PF_KTHREAD check in
void __fput_sync(struct file *file)
{
if (atomic_long_dec_and_test(&file->f_count)) {
- struct task_struct *task = current;
- BUG_ON(!(task->flags & PF_KTHREAD));
__fput(file);
}
}
so right now we'd BUG_ON(). It'd be neat to leave that in so
__fput_sync() doesn't get proliferated to non PF_KTHREAD without us
noticing. So maybe we just need a tiny primitive.
