| Message ID | 0d4dcd2dd113cdff9141336e583ab5afacfeee6f.1494954480.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(-)