[RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Al Viro]

IMO 93faf426e3cc "vfs: shave work on failed file open" had gone overboard -
avoiding an RCU delay in that particular case is fine, but it's done on
the wrong level.  A file that has never gotten FMODE_OPENED will never
have RCU-accessed references, its final fput() is equivalent to file_free()
and if it doesn't have FMODE_BACKING either, it can be done from any context
and won't need task_work treatment.

However, all of that can be achieved easier - all it takes is fput()
recognizing that case and calling file_free() directly.
No need to introduce a special primitive for that - and things like
failing dentry_open() could benefit from that as well.

Objections?

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/file_table.c b/fs/file_table.c
index de4a2915bfd4..7bcfa169dd45 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -75,18 +75,6 @@ static inline void file_free(struct file *f)
 	}
 }
 
-void release_empty_file(struct file *f)
-{
-	WARN_ON_ONCE(f->f_mode & (FMODE_BACKING | FMODE_OPENED));
-	if (atomic_long_dec_and_test(&f->f_count)) {
-		security_file_free(f);
-		put_cred(f->f_cred);
-		if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
-			percpu_counter_dec(&nr_files);
-		kmem_cache_free(filp_cachep, f);
-	}
-}
-
 /*
  * Return the total number of open files in the system
  */
@@ -445,6 +433,10 @@ void fput(struct file *file)
 	if (atomic_long_dec_and_test(&file->f_count)) {
 		struct task_struct *task = current;
 
+		if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
+			file_free(f);
+			return;
+		}
 		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))
diff --git a/fs/internal.h b/fs/internal.h
index 58e43341aebf..273e6fd40d1b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -94,7 +94,6 @@ extern void chroot_fs_refs(const struct path *, const struct path *);
 struct file *alloc_empty_file(int flags, const struct cred *cred);
 struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred);
 struct file *alloc_empty_backing_file(int flags, const struct cred *cred);
-void release_empty_file(struct file *f);
 
 static inline void file_put_write_access(struct file *file)
 {
diff --git a/fs/namei.c b/fs/namei.c
index 22915c40e2bd..e7f641d3115f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3787,10 +3787,7 @@ static struct file *path_openat(struct nameidata *nd,
 		WARN_ON(1);
 		error = -EINVAL;
 	}
-	if (unlikely(file->f_mode & FMODE_OPENED))
-		fput(file);
-	else
-		release_empty_file(file);
+	fput(file);
 	if (error == -EOPENSTALE) {
 		if (flags & LOOKUP_RCU)
 			error = -ECHILD;

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Linus Torvalds]

On Sat, 25 Nov 2023 at 18:08, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> However, all of that can be achieved easier - all it takes is fput()
> recognizing that case and calling file_free() directly.
> No need to introduce a special primitive for that - and things like
> failing dentry_open() could benefit from that as well.
>
> Objections?

Ack, looks fine.

In fact, I did suggest something along the lines at the time:

   https://lore.kernel.org/all/CAHk-=whLadznjNKZPYUjxVzAyCH-rRhb24_KaGegKT9E6A86Kg@mail.gmail.com/

although yours is simpler, because I for some reason (probably looking
at Mateusz' original patch too much) re-implemented file_free() as
fput_immediate()..

              Linus

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Al Viro]

On Sat, Nov 25, 2023 at 08:59:54PM -0800, Linus Torvalds wrote:
> On Sat, 25 Nov 2023 at 18:08, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > However, all of that can be achieved easier - all it takes is fput()
> > recognizing that case and calling file_free() directly.
> > No need to introduce a special primitive for that - and things like
> > failing dentry_open() could benefit from that as well.
> >
> > Objections?
> 
> Ack, looks fine.
> 
> In fact, I did suggest something along the lines at the time:
> 
>    https://lore.kernel.org/all/CAHk-=whLadznjNKZPYUjxVzAyCH-rRhb24_KaGegKT9E6A86Kg@mail.gmail.com/
> 
> although yours is simpler, because I for some reason (probably looking
> at Mateusz' original patch too much) re-implemented file_free() as
> fput_immediate()..

file_free() was with RCU delay at that time, IIRC.  I don't think that
cost of one test and (rarely) branch on each final fput() is going to
be measurable.

Mateusz, do you still have the setup you used for the original patch?
Could you profile and compare the current tree and current tree + the
patch upthread?

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Linus Torvalds]

On Sat, 25 Nov 2023 at 21:08, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Nov 25, 2023 at 08:59:54PM -0800, Linus Torvalds wrote:
> >
> >       because I for some reason (probably looking
> > at Mateusz' original patch too much) re-implemented file_free() as
> > fput_immediate()..
>
> file_free() was with RCU delay at that time, IIRC.

Ahh, indeed. So it was the SLAB_TYPESAFE_BY_RCU changes that basically
made my fput_immediate() and file_free() be the same, and thus it all
simplifies to your nice version.

>  I don't think that
> cost of one test and (rarely) branch on each final fput() is going to
> be measurable.

Nope. Me likey.

             Linus

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[kernel test robot]

Hi Al,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v6.7-rc2 next-20231124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Al-Viro/simpler-way-to-get-benefits-of-vfs-shave-work-on-failed-file-open/20231126-101216
base:   linux/master
patch link:    https://lore.kernel.org/r/20231126020834.GC38156%40ZenIV
patch subject: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"
config: i386-randconfig-141-20231126 (https://download.01.org/0day-ci/archive/20231126/202311261246.kHFXNLds-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231126/202311261246.kHFXNLds-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311261246.kHFXNLds-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/file_table.c:436:18: error: use of undeclared identifier 'f'
                   if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
                                  ^
   fs/file_table.c:437:14: error: use of undeclared identifier 'f'
                           file_free(f);
                                     ^
   2 errors generated.


vim +/f +436 fs/file_table.c

   430	
   431	void fput(struct file *file)
   432	{
   433		if (atomic_long_dec_and_test(&file->f_count)) {
   434			struct task_struct *task = current;
   435	
 > 436			if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
   437				file_free(f);
   438				return;
   439			}
   440			if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
   441				init_task_work(&file->f_rcuhead, ____fput);
   442				if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
   443					return;
   444				/*
   445				 * After this task has run exit_task_work(),
   446				 * task_work_add() will fail.  Fall through to delayed
   447				 * fput to avoid leaking *file.
   448				 */
   449			}
   450	
   451			if (llist_add(&file->f_llist, &delayed_fput_list))
   452				schedule_delayed_work(&delayed_fput_work, 1);
   453		}
   454	}
   455	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[kernel test robot]

Hi Al,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v6.7-rc2 next-20231124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Al-Viro/simpler-way-to-get-benefits-of-vfs-shave-work-on-failed-file-open/20231126-101216
base:   linux/master
patch link:    https://lore.kernel.org/r/20231126020834.GC38156%40ZenIV
patch subject: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231126/202311261322.8ip3vqTD-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231126/202311261322.8ip3vqTD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311261322.8ip3vqTD-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/array_size.h:5,
                    from include/linux/string.h:5,
                    from fs/file_table.c:9:
   fs/file_table.c: In function 'fput':
>> fs/file_table.c:436:32: error: 'f' undeclared (first use in this function); did you mean 'fd'?
     436 |                 if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
         |                                ^
   include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
      77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^
   fs/file_table.c:436:32: note: each undeclared identifier is reported only once for each function it appears in
     436 |                 if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
         |                                ^
   include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
      77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^


vim +436 fs/file_table.c

   430	
   431	void fput(struct file *file)
   432	{
   433		if (atomic_long_dec_and_test(&file->f_count)) {
   434			struct task_struct *task = current;
   435	
 > 436			if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
   437				file_free(f);
   438				return;
   439			}
   440			if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
   441				init_task_work(&file->f_rcuhead, ____fput);
   442				if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
   443					return;
   444				/*
   445				 * After this task has run exit_task_work(),
   446				 * task_work_add() will fail.  Fall through to delayed
   447				 * fput to avoid leaking *file.
   448				 */
   449			}
   450	
   451			if (llist_add(&file->f_llist, &delayed_fput_list))
   452				schedule_delayed_work(&delayed_fput_work, 1);
   453		}
   454	}
   455	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Christian Brauner]

On Sat, Nov 25, 2023 at 09:17:36PM -0800, Linus Torvalds wrote:
> On Sat, 25 Nov 2023 at 21:08, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sat, Nov 25, 2023 at 08:59:54PM -0800, Linus Torvalds wrote:
> > >
> > >       because I for some reason (probably looking
> > > at Mateusz' original patch too much) re-implemented file_free() as
> > > fput_immediate()..
> >
> > file_free() was with RCU delay at that time, IIRC.
> 
> Ahh, indeed. So it was the SLAB_TYPESAFE_BY_RCU changes that basically

Yes, special-casing this into file_free() wasn't looking very appealing.

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Christian Brauner]

On Sun, 26 Nov 2023 02:08:34 +0000, Al Viro wrote:
> IMO 93faf426e3cc "vfs: shave work on failed file open" had gone overboard -
> avoiding an RCU delay in that particular case is fine, but it's done on
> the wrong level.  A file that has never gotten FMODE_OPENED will never
> have RCU-accessed references, its final fput() is equivalent to file_free()
> and if it doesn't have FMODE_BACKING either, it can be done from any context
> and won't need task_work treatment.
> 
> [...]

Fwiw, you had a typo in their so I folded the fixup below into it and
tweaked the commit message. The cleanup is good.

diff --git a/fs/file_table.c b/fs/file_table.c
index 7bcfa169dd45..6deac386486d 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -433,8 +433,8 @@ void fput(struct file *file)
        if (atomic_long_dec_and_test(&file->f_count)) {
                struct task_struct *task = current;

-               if (unlikely(!(f->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
-                       file_free(f);
+               if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
+                       file_free(file);
                        return;
                }
                if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] file: massage cleanup of files that failed to open
      https://git.kernel.org/vfs/vfs/c/4d6fdbf44ad8

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Mateusz Guzik]

On Sun, Nov 26, 2023 at 10:21:59AM +0100, Christian Brauner wrote:
> On Sat, Nov 25, 2023 at 09:17:36PM -0800, Linus Torvalds wrote:
> > On Sat, 25 Nov 2023 at 21:08, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Sat, Nov 25, 2023 at 08:59:54PM -0800, Linus Torvalds wrote:
> > > >
> > > >       because I for some reason (probably looking
> > > > at Mateusz' original patch too much) re-implemented file_free() as
> > > > fput_immediate()..
> > >
> > > file_free() was with RCU delay at that time, IIRC.
> > 
> > Ahh, indeed. So it was the SLAB_TYPESAFE_BY_RCU changes that basically
> 
> Yes, special-casing this into file_free() wasn't looking very appealing.

Right, if the SLAB_TYPESAFE_BY_RCU work was already there my v1 for
dodging task_work would have been much simpler (but would still have
fput_badopen).

While I support deduping code which comes with this patch I'm not fond
of special casing failed opens in fput.

A minor remark is that in the spot which ends up calling here on stock
kernel it is FMODE_OPENED which is the unlikely case, but with the patch
it ends up being handled with a branch marked the other way around.
I noted in my commit message failed opens are not some corner-case, they
are much common.

The thing which irks me on its principle is that special-casing for the
case which is guaranteed to not be true for majority of fput users is
avoidably rolled into the general routine.

For my taste the code below (untested) would be nicer, but I don't think
there are solid grounds for picking one approach over another. That is
to say if you insist on Al's variant then we are done here. :)

diff --git a/fs/file_table.c b/fs/file_table.c
index de4a2915bfd4..5e5613d80631 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -75,18 +75,6 @@ static inline void file_free(struct file *f)
 	}
 }
 
-void release_empty_file(struct file *f)
-{
-	WARN_ON_ONCE(f->f_mode & (FMODE_BACKING | FMODE_OPENED));
-	if (atomic_long_dec_and_test(&f->f_count)) {
-		security_file_free(f);
-		put_cred(f->f_cred);
-		if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
-			percpu_counter_dec(&nr_files);
-		kmem_cache_free(filp_cachep, f);
-	}
-}
-
 /*
  * Return the total number of open files in the system
  */
@@ -461,6 +449,18 @@ void fput(struct file *file)
 	}
 }
 
+void fput_badopen(struct file *f)
+{
+	if (unlikely(f->f_mode & (FMODE_BACKING | FMODE_OPENED))) {
+		fput(f);
+		return;
+	}
+
+	if (likely(atomic_long_dec_and_test(&f->f_count))) {
+		file_free(f);
+	}
+}
+
 /*
  * synchronous analog of fput(); for kernel threads that might be needed
  * in some umount() (and thus can't use flush_delayed_fput() without
diff --git a/fs/internal.h b/fs/internal.h
index 58e43341aebf..3afe774ff7c6 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -94,7 +94,7 @@ extern void chroot_fs_refs(const struct path *, const struct path *);
 struct file *alloc_empty_file(int flags, const struct cred *cred);
 struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred);
 struct file *alloc_empty_backing_file(int flags, const struct cred *cred);
-void release_empty_file(struct file *f);
+void fput_badopen(struct file *f);
 
 static inline void file_put_write_access(struct file *file)
 {
diff --git a/fs/namei.c b/fs/namei.c
index 71c13b2990b4..e42e2c237a4c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3785,10 +3785,7 @@ static struct file *path_openat(struct nameidata *nd,
 		WARN_ON(1);
 		error = -EINVAL;
 	}
-	if (unlikely(file->f_mode & FMODE_OPENED))
-		fput(file);
-	else
-		release_empty_file(file);
+	fput_badopen(file);
 	if (error == -EOPENSTALE) {
 		if (flags & LOOKUP_RCU)
 			error = -ECHILD;

Re: [RFC][PATCH] simpler way to get benefits of "vfs: shave work on failed file open"

[Christian Brauner]

> to say if you insist on Al's variant then we are done here. :)

I think it's just simpler and having it in a central place is actually
nicer in this case.

I mostly try to avoid arguing about minutiae unless they do actually
have provable impact so if a patch comes that adheres to someones taste
more than my own then I'm not going to argue (quite often). All three
version are fine and functional.