vfs: make evict() use smp_mb__after_spinlock instead of smp_mb

[PATCH] vfs: make evict() use smp_mb__after_spinlock instead of smp_mb

Posted by Mateusz Guzik 1 week, 2 days ago

It literally directly follows a spin_lock() call.

This whacks an explicit barrier on x86-64.

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

This plausibly can go away altogether, but I could not be arsed to
convince myself that's correct. Individuals willing to put in time are
welcome :)

 fs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index e5a60084a7a9..b3db1234737f 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -817,7 +817,7 @@ static void evict(struct inode *inode)
 	 * ___wait_var_event() either sees the bit cleared or
 	 * waitqueue_active() check in wake_up_var() sees the waiter.
 	 */
-	smp_mb();
+	smp_mb__after_spinlock();
 	inode_wake_up_bit(inode, __I_NEW);
 	BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
 	spin_unlock(&inode->i_lock);
-- 
2.43.0

Re: [PATCH] vfs: make evict() use smp_mb__after_spinlock instead of smp_mb

Posted by Christian Brauner 1 week, 1 day ago

On Wed, 13 Nov 2024 16:51:03 +0100, Mateusz Guzik wrote:
> It literally directly follows a spin_lock() call.
> 
> This whacks an explicit barrier on x86-64.
> 
> 

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: make evict() use smp_mb__after_spinlock instead of smp_mb
      https://git.kernel.org/vfs/vfs/c/45c9faf50665

Re: [PATCH] vfs: make evict() use smp_mb__after_spinlock instead of smp_mb

Posted by Jan Kara 1 week, 2 days ago

On Wed 13-11-24 16:51:03, Mateusz Guzik wrote:
> It literally directly follows a spin_lock() call.
> 
> This whacks an explicit barrier on x86-64.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good. Feel free to add:

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

> This plausibly can go away altogether, but I could not be arsed to
> convince myself that's correct. Individuals willing to put in time are
> welcome :)

AFAICS there's nothing else really guaranteeing the last store to
inode->i_state cannot be reordered up to after the wake up so I think the
barrier should be there.

								Honza
> 
>  fs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index e5a60084a7a9..b3db1234737f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -817,7 +817,7 @@ static void evict(struct inode *inode)
>  	 * ___wait_var_event() either sees the bit cleared or
>  	 * waitqueue_active() check in wake_up_var() sees the waiter.
>  	 */
> -	smp_mb();
> +	smp_mb__after_spinlock();
>  	inode_wake_up_bit(inode, __I_NEW);
>  	BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
>  	spin_unlock(&inode->i_lock);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] vfs: make evict() use smp_mb__after_spinlock instead of smp_mb

Posted by Mateusz Guzik 1 week, 2 days ago

On Wed, Nov 13, 2024 at 5:17 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 13-11-24 16:51:03, Mateusz Guzik wrote:
> > It literally directly follows a spin_lock() call.
> >
> > This whacks an explicit barrier on x86-64.
> >
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
>
> Looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>

thanks

> > This plausibly can go away altogether, but I could not be arsed to
> > convince myself that's correct. Individuals willing to put in time are
> > welcome :)
>
> AFAICS there's nothing else really guaranteeing the last store to
> inode->i_state cannot be reordered up to after the wake up so I think the
> barrier should be there.
>

There is a bunch of lock round trips in this routine alone, including
on i_lock itself, but that aside:

I *suspect* something like spin_wait_unlocked(&inode->i_state)
shipping with a full fence at the beginning of the routine would
correctly allow to check all the possible waiter et al flags without
acquiring the lock anymore, shaving off at least 2 lock trips in the
common case.

However, I don't see such a routine as is and I'm definitely not going
to flame about adding it for the time being.

>                                                                 Honza
> >
> >  fs/inode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index e5a60084a7a9..b3db1234737f 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -817,7 +817,7 @@ static void evict(struct inode *inode)
> >        * ___wait_var_event() either sees the bit cleared or
> >        * waitqueue_active() check in wake_up_var() sees the waiter.
> >        */
> > -     smp_mb();
> > +     smp_mb__after_spinlock();
> >       inode_wake_up_bit(inode, __I_NEW);
> >       BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
> >       spin_unlock(&inode->i_lock);
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR



-- 
Mateusz Guzik <mjguzik gmail.com>