[PATCH] fs: dodge an atomic in putname if ref == 1

[PATCH] fs: dodge an atomic in putname if ref == 1

[Mateusz Guzik]

While the structure is refcounted, the only consumer incrementing it is
audit and even then the atomic operation is only needed when it
interacts with io_uring.

If putname spots a count of 1, there is no legitimate way for anyone to
bump it.

If audit is disabled, the count is guaranteed to be 1, which
consistently elides the atomic for all path lookups. If audit is
enabled, it still manages to elide the last decrement.

Note the patch does not do anything to prevent audit from suffering
atomics. See [1] and [2] for a different approach.

Benchmarked on Sapphire Rapids issuing access() (ops/s):
before: 5106246
after:  5269678 (+3%)

Link 1:	https://lore.kernel.org/linux-fsdevel/20250307161155.760949-1-mjguzik@gmail.com/
Link 2: https://lore.kernel.org/linux-fsdevel/20250307164216.GI2023217@ZenIV/
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

This is an alternative to the patch I linked above.

I think the improved commit message should also cover the feedback
Christian previously shared concerning it.

This is a trivial win until the atomic issue gets resolved, I don't see
any reason to NOT include it. At the same time I don't have that much
interest arguing about it either.

That is to say, here is a different take, if you don't like it, I'm
dropping the subject.

cheers

 fs/namei.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 06765d320e7e..add90981cfcd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -275,14 +275,19 @@ EXPORT_SYMBOL(getname_kernel);
 
 void putname(struct filename *name)
 {
+	int refcnt;
+
 	if (IS_ERR_OR_NULL(name))
 		return;
 
-	if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
-		return;
+	refcnt = atomic_read(&name->refcnt);
+	if (refcnt != 1) {
+		if (WARN_ON_ONCE(!refcnt))
+			return;
 
-	if (!atomic_dec_and_test(&name->refcnt))
-		return;
+		if (!atomic_dec_and_test(&name->refcnt))
+			return;
+	}
 
 	if (name->name != name->iname) {
 		__putname(name->name);
-- 
2.43.0

Re: [PATCH] fs: dodge an atomic in putname if ref == 1

[Christian Brauner]

On Tue, 11 Mar 2025 19:18:04 +0100, Mateusz Guzik wrote:
> While the structure is refcounted, the only consumer incrementing it is
> audit and even then the atomic operation is only needed when it
> interacts with io_uring.
> 
> If putname spots a count of 1, there is no legitimate way for anyone to
> bump it.
> 
> [...]

Applied to the vfs-6.15.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.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-6.15.misc

[1/1] fs: dodge an atomic in putname if ref == 1
      https://git.kernel.org/vfs/vfs/c/c93617c0f22c

Re: [PATCH] fs: dodge an atomic in putname if ref == 1

[Jan Kara]

On Tue 11-03-25 19:18:04, Mateusz Guzik wrote:
> While the structure is refcounted, the only consumer incrementing it is
> audit and even then the atomic operation is only needed when it
> interacts with io_uring.
> 
> If putname spots a count of 1, there is no legitimate way for anyone to
> bump it.
> 
> If audit is disabled, the count is guaranteed to be 1, which
> consistently elides the atomic for all path lookups. If audit is
> enabled, it still manages to elide the last decrement.
> 
> Note the patch does not do anything to prevent audit from suffering
> atomics. See [1] and [2] for a different approach.
> 
> Benchmarked on Sapphire Rapids issuing access() (ops/s):
> before: 5106246
> after:  5269678 (+3%)
> 
> Link 1:	https://lore.kernel.org/linux-fsdevel/20250307161155.760949-1-mjguzik@gmail.com/
> Link 2: https://lore.kernel.org/linux-fsdevel/20250307164216.GI2023217@ZenIV/
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Yeah, I like this much more than the original. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> This is an alternative to the patch I linked above.
> 
> I think the improved commit message should also cover the feedback
> Christian previously shared concerning it.
> 
> This is a trivial win until the atomic issue gets resolved, I don't see
> any reason to NOT include it. At the same time I don't have that much
> interest arguing about it either.
> 
> That is to say, here is a different take, if you don't like it, I'm
> dropping the subject.
> 
> cheers
> 
>  fs/namei.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 06765d320e7e..add90981cfcd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -275,14 +275,19 @@ EXPORT_SYMBOL(getname_kernel);
>  
>  void putname(struct filename *name)
>  {
> +	int refcnt;
> +
>  	if (IS_ERR_OR_NULL(name))
>  		return;
>  
> -	if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
> -		return;
> +	refcnt = atomic_read(&name->refcnt);
> +	if (refcnt != 1) {
> +		if (WARN_ON_ONCE(!refcnt))
> +			return;
>  
> -	if (!atomic_dec_and_test(&name->refcnt))
> -		return;
> +		if (!atomic_dec_and_test(&name->refcnt))
> +			return;
> +	}
>  
>  	if (name->name != name->iname) {
>  		__putname(name->name);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: dodge an atomic in putname if ref == 1

[Jens Axboe]

On 3/11/25 12:18 PM, Mateusz Guzik wrote:
> diff --git a/fs/namei.c b/fs/namei.c
> index 06765d320e7e..add90981cfcd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -275,14 +275,19 @@ EXPORT_SYMBOL(getname_kernel);
>  
>  void putname(struct filename *name)
>  {
> +	int refcnt;
> +
>  	if (IS_ERR_OR_NULL(name))
>  		return;
>  
> -	if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
> -		return;
> +	refcnt = atomic_read(&name->refcnt);
> +	if (refcnt != 1) {
> +		if (WARN_ON_ONCE(!refcnt))
> +			return;
>  
> -	if (!atomic_dec_and_test(&name->refcnt))
> -		return;
> +		if (!atomic_dec_and_test(&name->refcnt))
> +			return;
> +	}

Looks good to me, I use this trick with bitops too all the time, to
avoid a RMW when possible.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe