[PATCH] vfs: dodge smp_mb in break_lease and break_deleg in the common case

[PATCH] vfs: dodge smp_mb in break_lease and break_deleg in the common case

[Mateusz Guzik]

These inlines show up in the fast path (e.g., in do_dentry_open()) and
induce said full barrier regarding i_flctx access when in most cases the
pointer is NULL.

The pointer can be safely checked before issuing the barrier, dodging it
in most cases as a result.

It is plausible the consume fence would be sufficient, but I don't want
to go audit all callers regarding what they before calling here.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

the header file has locks_inode_context and i even found users like
this (lease_get_mtime):

ctx = locks_inode_context(inode);
if (ctx && !list_empty_careful(&ctx->flc_lease)) {

however, without looking further at the code I'm not confident this
would be sufficient here -- for all I know one consumer needs all stores
to be visible before looking further after derefing the pointer

keeping the full fence in place makes this reasonably easy to reason
about the change i think, but someone(tm) willing to sort this out is
most welcome to do so

 include/linux/filelock.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/filelock.h b/include/linux/filelock.h
index daee999d05f3..bb44224c6676 100644
--- a/include/linux/filelock.h
+++ b/include/linux/filelock.h
@@ -420,28 +420,38 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
 #ifdef CONFIG_FILE_LOCKING
 static inline int break_lease(struct inode *inode, unsigned int mode)
 {
+	struct file_lock_context *flctx;
+
 	/*
 	 * Since this check is lockless, we must ensure that any refcounts
 	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
 	 * could end up racing with tasks trying to set a new lease on this
 	 * file.
 	 */
+	flctx = READ_ONCE(inode->i_flctx);
+	if (!flctx)
+		return 0;
 	smp_mb();
-	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
+	if (!list_empty_careful(&flctx->flc_lease))
 		return __break_lease(inode, mode, FL_LEASE);
 	return 0;
 }
 
 static inline int break_deleg(struct inode *inode, unsigned int mode)
 {
+	struct file_lock_context *flctx;
+
 	/*
 	 * Since this check is lockless, we must ensure that any refcounts
 	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
 	 * could end up racing with tasks trying to set a new lease on this
 	 * file.
 	 */
+	flctx = READ_ONCE(inode->i_flctx);
+	if (!flctx)
+		return 0;
 	smp_mb();
-	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
+	if (!list_empty_careful(&flctx->flc_lease))
 		return __break_lease(inode, mode, FL_DELEG);
 	return 0;
 }
-- 
2.43.0

Re: [PATCH] vfs: dodge smp_mb in break_lease and break_deleg in the common case

[Jeff Layton]

On Tue, 2024-08-06 at 19:28 +0200, Mateusz Guzik wrote:
> These inlines show up in the fast path (e.g., in do_dentry_open()) and
> induce said full barrier regarding i_flctx access when in most cases the
> pointer is NULL.
> 
> The pointer can be safely checked before issuing the barrier, dodging it
> in most cases as a result.
> 
> It is plausible the consume fence would be sufficient, but I don't want
> to go audit all callers regarding what they before calling here.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> the header file has locks_inode_context and i even found users like
> this (lease_get_mtime):
> 
> ctx = locks_inode_context(inode);
> if (ctx && !list_empty_careful(&ctx->flc_lease)) {
> 
> however, without looking further at the code I'm not confident this
> would be sufficient here -- for all I know one consumer needs all stores
> to be visible before looking further after derefing the pointer
> 
> keeping the full fence in place makes this reasonably easy to reason
> about the change i think, but someone(tm) willing to sort this out is
> most welcome to do so
> 

Nod. It would be nice to get rid of that barrier. I'm not sure how to
do that in a provably correct way. I'll need to think about that.

>  include/linux/filelock.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index daee999d05f3..bb44224c6676 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -420,28 +420,38 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  #ifdef CONFIG_FILE_LOCKING
>  static inline int break_lease(struct inode *inode, unsigned int mode)
>  {
> +	struct file_lock_context *flctx;
> +
>  	/*
>  	 * Since this check is lockless, we must ensure that any refcounts
>  	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
>  	 * could end up racing with tasks trying to set a new lease on this
>  	 * file.
>  	 */
> +	flctx = READ_ONCE(inode->i_flctx);
> +	if (!flctx)
> +		return 0;
>  	smp_mb();
> -	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
> +	if (!list_empty_careful(&flctx->flc_lease))
>  		return __break_lease(inode, mode, FL_LEASE);
>  	return 0;
>  }
>  
>  static inline int break_deleg(struct inode *inode, unsigned int mode)
>  {
> +	struct file_lock_context *flctx;
> +
>  	/*
>  	 * Since this check is lockless, we must ensure that any refcounts
>  	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
>  	 * could end up racing with tasks trying to set a new lease on this
>  	 * file.
>  	 */
> +	flctx = READ_ONCE(inode->i_flctx);
> +	if (!flctx)
> +		return 0;
>  	smp_mb();
> -	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
> +	if (!list_empty_careful(&flctx->flc_lease))
>  		return __break_lease(inode, mode, FL_DELEG);
>  	return 0;
>  }

This change looks good to me:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] vfs: dodge smp_mb in break_lease and break_deleg in the common case

[Christian Brauner]

On Tue, 06 Aug 2024 19:28:46 +0200, Mateusz Guzik wrote:
> These inlines show up in the fast path (e.g., in do_dentry_open()) and
> induce said full barrier regarding i_flctx access when in most cases the
> pointer is NULL.
> 
> The pointer can be safely checked before issuing the barrier, dodging it
> in most cases as a result.
> 
> [...]

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] vfs: dodge smp_mb in break_lease and break_deleg in the common case
      https://git.kernel.org/vfs/vfs/c/e23ee5db10d3