| Message ID | 20240806172846.886570-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfs: dodge smp_mb in break_lease and break_deleg in the common case | expand |
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>
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
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; }
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(-)