[06/10] btrfs: scrub: use bool for flush_all_writes
diff mbox

Message ID 0d4dcd2dd113cdff9141336e583ab5afacfeee6f.1494954480.git.dsterba@suse.com
State New
Headers show

Commit Message

David Sterba May 16, 2017, 5:10 p.m. UTC
flush_all_writes is an atomic but does not use the semantics at all,
it's just on/off indicator, we can use bool.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/scrub.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Liu Bo May 19, 2017, 12:42 a.m. UTC | #1
On Tue, May 16, 2017 at 07:10:38PM +0200, David Sterba wrote:
> flush_all_writes is an atomic but does not use the semantics at all,
> it's just on/off indicator, we can use bool.
> 

It might use atomic to avoid reordering, but I'm not sure if
reordering could really happen here.

Thanks,

-liubo

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/scrub.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 2c1d344c8edd..8caf775fee0a 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -182,8 +182,8 @@ struct scrub_ctx {
>  
>  	struct scrub_bio        *wr_curr_bio;
>  	struct mutex            wr_lock;
> -	atomic_t                flush_all_writes;
>  	struct btrfs_device     *wr_tgtdev;
> +	bool                    flush_all_writes;
>  
>  	/*
>  	 * statistics
> @@ -718,7 +718,7 @@ struct scrub_ctx *scrub_setup_ctx(struct btrfs_device *dev, int is_dev_replace)
>  	if (is_dev_replace) {
>  		WARN_ON(!dev->bdev);
>  		sctx->wr_tgtdev = dev;
> -		atomic_set(&sctx->flush_all_writes, 0);
> +		sctx->flush_all_writes = false;
>  	}
>  
>  	return sctx;
> @@ -2410,8 +2410,7 @@ static void scrub_missing_raid56_worker(struct btrfs_work *work)
>  
>  	scrub_block_put(sblock);
>  
> -	if (sctx->is_dev_replace &&
> -	    atomic_read(&sctx->flush_all_writes)) {
> +	if (sctx->is_dev_replace && sctx->flush_all_writes) {
>  		mutex_lock(&sctx->wr_lock);
>  		scrub_wr_submit(sctx);
>  		mutex_unlock(&sctx->wr_lock);
> @@ -2618,8 +2617,7 @@ static void scrub_bio_end_io_worker(struct btrfs_work *work)
>  	sctx->first_free = sbio->index;
>  	spin_unlock(&sctx->list_lock);
>  
> -	if (sctx->is_dev_replace &&
> -	    atomic_read(&sctx->flush_all_writes)) {
> +	if (sctx->is_dev_replace && sctx->flush_all_writes) {
>  		mutex_lock(&sctx->wr_lock);
>  		scrub_wr_submit(sctx);
>  		mutex_unlock(&sctx->wr_lock);
> @@ -3454,14 +3452,14 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>  		 */
>  		if (atomic_read(&fs_info->scrub_pause_req)) {
>  			/* push queued extents */
> -			atomic_set(&sctx->flush_all_writes, 1);
> +			sctx->flush_all_writes = true;
>  			scrub_submit(sctx);
>  			mutex_lock(&sctx->wr_lock);
>  			scrub_wr_submit(sctx);
>  			mutex_unlock(&sctx->wr_lock);
>  			wait_event(sctx->list_wait,
>  				   atomic_read(&sctx->bios_in_flight) == 0);
> -			atomic_set(&sctx->flush_all_writes, 0);
> +			sctx->flush_all_writes = false;
>  			scrub_blocked_if_needed(fs_info);
>  		}
>  
> @@ -3907,7 +3905,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>  		 * write requests are really completed when bios_in_flight
>  		 * changes to 0.
>  		 */
> -		atomic_set(&sctx->flush_all_writes, 1);
> +		sctx->flush_all_writes = true;
>  		scrub_submit(sctx);
>  		mutex_lock(&sctx->wr_lock);
>  		scrub_wr_submit(sctx);
> @@ -3925,7 +3923,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>  		 */
>  		wait_event(sctx->list_wait,
>  			   atomic_read(&sctx->workers_pending) == 0);
> -		atomic_set(&sctx->flush_all_writes, 0);
> +		sctx->flush_all_writes = false;
>  
>  		scrub_pause_off(fs_info);
>  
> -- 
> 2.12.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 19, 2017, 1:08 p.m. UTC | #2
On Thu, May 18, 2017 at 05:42:24PM -0700, Liu Bo wrote:
> On Tue, May 16, 2017 at 07:10:38PM +0200, David Sterba wrote:
> > flush_all_writes is an atomic but does not use the semantics at all,
> > it's just on/off indicator, we can use bool.
> > 
> 
> It might use atomic to avoid reordering, but I'm not sure if
> reordering could really happen here.

Ok, I'll have a look. It does not seem to rely on atomics/ordering though.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 29, 2017, 2:59 p.m. UTC | #3
On Fri, May 19, 2017 at 03:08:22PM +0200, David Sterba wrote:
> On Thu, May 18, 2017 at 05:42:24PM -0700, Liu Bo wrote:
> > On Tue, May 16, 2017 at 07:10:38PM +0200, David Sterba wrote:
> > > flush_all_writes is an atomic but does not use the semantics at all,
> > > it's just on/off indicator, we can use bool.
> > > 
> > 
> > It might use atomic to avoid reordering, but I'm not sure if
> > reordering could really happen here.
> 
> Ok, I'll have a look. It does not seem to rely on atomics/ordering though.

The semantics of atomic_t to not lose concurrent increment/decrement is
not used here. The variable is always used within the following patterns:

	set to 1
	submit writes
	wait for completion
	set to 0

or

	if set: submit writes

The potential ordering issues, eg. when the worker thread that just
reads the value will not see the value 1 from other thread would need to
keep the state out of sync overal several mutex lock/unlock calls, that
imply a barrier anyway.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 2c1d344c8edd..8caf775fee0a 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -182,8 +182,8 @@  struct scrub_ctx {
 
 	struct scrub_bio        *wr_curr_bio;
 	struct mutex            wr_lock;
-	atomic_t                flush_all_writes;
 	struct btrfs_device     *wr_tgtdev;
+	bool                    flush_all_writes;
 
 	/*
 	 * statistics
@@ -718,7 +718,7 @@  struct scrub_ctx *scrub_setup_ctx(struct btrfs_device *dev, int is_dev_replace)
 	if (is_dev_replace) {
 		WARN_ON(!dev->bdev);
 		sctx->wr_tgtdev = dev;
-		atomic_set(&sctx->flush_all_writes, 0);
+		sctx->flush_all_writes = false;
 	}
 
 	return sctx;
@@ -2410,8 +2410,7 @@  static void scrub_missing_raid56_worker(struct btrfs_work *work)
 
 	scrub_block_put(sblock);
 
-	if (sctx->is_dev_replace &&
-	    atomic_read(&sctx->flush_all_writes)) {
+	if (sctx->is_dev_replace && sctx->flush_all_writes) {
 		mutex_lock(&sctx->wr_lock);
 		scrub_wr_submit(sctx);
 		mutex_unlock(&sctx->wr_lock);
@@ -2618,8 +2617,7 @@  static void scrub_bio_end_io_worker(struct btrfs_work *work)
 	sctx->first_free = sbio->index;
 	spin_unlock(&sctx->list_lock);
 
-	if (sctx->is_dev_replace &&
-	    atomic_read(&sctx->flush_all_writes)) {
+	if (sctx->is_dev_replace && sctx->flush_all_writes) {
 		mutex_lock(&sctx->wr_lock);
 		scrub_wr_submit(sctx);
 		mutex_unlock(&sctx->wr_lock);
@@ -3454,14 +3452,14 @@  static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 		 */
 		if (atomic_read(&fs_info->scrub_pause_req)) {
 			/* push queued extents */
-			atomic_set(&sctx->flush_all_writes, 1);
+			sctx->flush_all_writes = true;
 			scrub_submit(sctx);
 			mutex_lock(&sctx->wr_lock);
 			scrub_wr_submit(sctx);
 			mutex_unlock(&sctx->wr_lock);
 			wait_event(sctx->list_wait,
 				   atomic_read(&sctx->bios_in_flight) == 0);
-			atomic_set(&sctx->flush_all_writes, 0);
+			sctx->flush_all_writes = false;
 			scrub_blocked_if_needed(fs_info);
 		}
 
@@ -3907,7 +3905,7 @@  int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 		 * write requests are really completed when bios_in_flight
 		 * changes to 0.
 		 */
-		atomic_set(&sctx->flush_all_writes, 1);
+		sctx->flush_all_writes = true;
 		scrub_submit(sctx);
 		mutex_lock(&sctx->wr_lock);
 		scrub_wr_submit(sctx);
@@ -3925,7 +3923,7 @@  int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 		 */
 		wait_event(sctx->list_wait,
 			   atomic_read(&sctx->workers_pending) == 0);
-		atomic_set(&sctx->flush_all_writes, 0);
+		sctx->flush_all_writes = false;
 
 		scrub_pause_off(fs_info);