linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Davidlohr Bueso <dave@stgolabs.net>
Cc: dsterba@suse.com, nborisov@suse.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH] btrfs: optimize barrier usage for Rmw atomics
Date: Wed, 29 Jan 2020 20:14:40 +0100	[thread overview]
Message-ID: <20200129191439.GN3929@twin.jikos.cz> (raw)
In-Reply-To: <20200129180324.24099-1-dave@stgolabs.net>

On Wed, Jan 29, 2020 at 10:03:24AM -0800, Davidlohr Bueso wrote:
> Use smp_mb__after_atomic() instead of smp_mb() and avoid the
> unnecessary barrier for non LL/SC architectures, such as x86.

So that's a conflicting advice from what we got when discussing wich
barriers to use in 6282675e6708ec78518cc0e9ad1f1f73d7c5c53d and the
memory is still fresh. My first idea was to take the
smp_mb__after_atomic and __before_atomic variants and after discussion
with various people the plain smp_wmb/smp_rmb were suggested and used in
the end.

I can dig the email threads and excerpts from irc conversations, maybe
Nik has them at hand too. We do want to get rid of all unnecessary and
uncommented barriers in btrfs code, so I appreciate your patch.

> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  fs/btrfs/btrfs_inode.h | 2 +-
>  fs/btrfs/file.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 4e12a477d32e..54e0d2ae22cc 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -325,7 +325,7 @@ struct btrfs_dio_private {
>  static inline void btrfs_inode_block_unlocked_dio(struct btrfs_inode *inode)
>  {
>  	set_bit(BTRFS_INODE_READDIO_NEED_LOCK, &inode->runtime_flags);
> -	smp_mb();
> +	smp_mb__after_atomic();

In this case I think we should use the smp_wmb/smp_rmb pattern rather
than the full barrier.

>  }
>  
>  static inline void btrfs_inode_resume_unlocked_dio(struct btrfs_inode *inode)
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a16da274c9aa..ea79ab068079 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2143,7 +2143,7 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  	}
>  	atomic_inc(&root->log_batch);
>  
> -	smp_mb();
> +	smp_mb__after_atomic();

That's the problem with uncommented barriers that it's not clear what
are they related to. In this case it's not the atomic_inc above that
would justify __after_atomic. The patch that added it is years old so
any change to that barrier would require deeper analysis.

>  	if (btrfs_inode_in_log(BTRFS_I(inode), fs_info->generation) ||
>  	    BTRFS_I(inode)->last_trans <= fs_info->last_trans_committed) {
>  		/*

  parent reply	other threads:[~2020-01-29 19:15 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-29 18:03 [PATCH] btrfs: optimize barrier usage for Rmw atomics Davidlohr Bueso
2020-01-29 19:07 ` Nikolay Borisov
2020-01-29 19:14 ` David Sterba [this message]
2020-01-29 19:25   ` Davidlohr Bueso
2020-01-29 23:55     ` Qu Wenruo
2020-01-30  8:18       ` Nikolay Borisov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200129191439.GN3929@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=dave@stgolabs.net \
    --cc=dbueso@suse.de \
    --cc=dsterba@suse.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nborisov@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).