Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Wed, 27 Sep 2023 10:48:20 -0700 | Subject | Re: [PATCH v2] vfs: shave work on failed file open |
| |
On Wed, 27 Sept 2023 at 07:10, Christian Brauner <brauner@kernel.org> wrote: > > No need to resend I can massage this well enough in-tree.
Hmm. Please do, but here's some more food for thought for at least the commit message.
Because there's more than the "__fput_sync()" issue at hand, we have another delayed thing that this patch ends up short-circuiting, which wasn't obvious from the original description.
I'm talking about the fact that our existing "file_free()" ends up doing the actual release with
call_rcu(&f->f_rcuhead, file_free_rcu);
and the patch under discussion avoids that part too.
And I actually like that it avoids it, I just think it should be mentioned explicitly, because it wasn't obvious to me until I actually looked at the old __fput() path. Particularly since it means that the f_creds are free'd synchronously now.
I do think that's fine, although I forget what path it was that required that rcu-delayed cred freeing. Worth mentioning, and maybe worth thinking about.
However, when I *did* look at it, it strikes me that we could do this differently.
Something like this (ENTIRELY UNTESTED) patch, which just moves this logic into fput() itself.
Again: ENTIRELY UNTESTED, and I might easily have screwed up. But it looks simpler and more straightforward to me. But again: that may be because I missed something.
Linus fs/file_table.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/fs/file_table.c b/fs/file_table.c index ee21b3da9d08..4fb87a0382d9 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -430,11 +430,33 @@ EXPORT_SYMBOL_GPL(flush_delayed_fput); static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); +/* + * Called for files that were never fully opened, and + * don't need the RCU-delayed freeing: they have never + * been accessed in any other context. + */ +static void fput_immediate(struct file *f) +{ + security_file_free(f); + put_cred(f->f_cred); + if (likely(!(f->f_mode & FMODE_NOACCOUNT))) + percpu_counter_dec(&nr_files); + if (unlikely(f->f_mode & FMODE_BACKING)) { + path_put(backing_file_real_path(f)); + kfree(backing_file(f)); + } else { + kmem_cache_free(filp_cachep, f); + } +} + void fput(struct file *file) { if (atomic_long_dec_and_test(&file->f_count)) { struct task_struct *task = current; + if (unlikely(!(file->f_mode & FMODE_OPENED))) + return fput_immediate(file); + if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { init_task_work(&file->f_rcuhead, ____fput); if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME)) | |