From: Omar Sandoval <osandov@osandov.com>
To: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 06/11] btrfs: dev-replace: move replace members out of fs_info
Date: Fri, 7 Sep 2018 14:09:48 -0700 [thread overview]
Message-ID: <20180907210948.GI29245@vader> (raw)
In-Reply-To: <b990783799ffa2cdcccd262cacdf4ce23ba36502.1536331604.git.dsterba@suse.com>
On Fri, Sep 07, 2018 at 04:55:13PM +0200, David Sterba wrote:
> The replace_wait and bio_counter were mistakenly added to fs_info in
> commit c404e0dc2c843b154f ("Btrfs: fix use-after-free in the finishing
> procedure of the device replace"), but they logically belong to
> fs_info::dev_replace. Besides, bio_counter is a very generic name and is
> confusing in bare fs_info context.
Makes much more sense there.
Reviewed-by: Omar Sandoval <osandov@fb.com>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/ctree.h | 6 +++---
> fs/btrfs/dev-replace.c | 16 ++++++++--------
> fs/btrfs/disk-io.c | 9 +++++----
> 3 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 31f7a58add7e..89fc90eafb0a 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -371,6 +371,9 @@ struct btrfs_dev_replace {
> wait_queue_head_t read_lock_wq;
>
> struct btrfs_scrub_progress scrub_progress;
> +
> + struct percpu_counter bio_counter;
> + wait_queue_head_t replace_wait;
> };
>
> /* For raid type sysfs entries */
> @@ -1093,9 +1096,6 @@ struct btrfs_fs_info {
> /* device replace state */
> struct btrfs_dev_replace dev_replace;
>
> - struct percpu_counter bio_counter;
> - wait_queue_head_t replace_wait;
> -
> struct semaphore uuid_tree_rescan_sem;
>
> /* Used to reclaim the metadata space in the background. */
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 0a49843b2ee6..c0e9b2c229d5 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -540,8 +540,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> static void btrfs_rm_dev_replace_blocked(struct btrfs_fs_info *fs_info)
> {
> set_bit(BTRFS_FS_STATE_DEV_REPLACING, &fs_info->fs_state);
> - wait_event(fs_info->replace_wait, !percpu_counter_sum(
> - &fs_info->bio_counter));
> + wait_event(fs_info->dev_replace.replace_wait, !percpu_counter_sum(
> + &fs_info->dev_replace.bio_counter));
> }
>
> /*
> @@ -550,7 +550,7 @@ static void btrfs_rm_dev_replace_blocked(struct btrfs_fs_info *fs_info)
> static void btrfs_rm_dev_replace_unblocked(struct btrfs_fs_info *fs_info)
> {
> clear_bit(BTRFS_FS_STATE_DEV_REPLACING, &fs_info->fs_state);
> - wake_up(&fs_info->replace_wait);
> + wake_up(&fs_info->dev_replace.replace_wait);
> }
>
> static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> @@ -992,25 +992,25 @@ void btrfs_dev_replace_set_lock_blocking(
>
> void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info)
> {
> - percpu_counter_inc(&fs_info->bio_counter);
> + percpu_counter_inc(&fs_info->dev_replace.bio_counter);
> }
>
> void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount)
> {
> - percpu_counter_sub(&fs_info->bio_counter, amount);
> - cond_wake_up_nomb(&fs_info->replace_wait);
> + percpu_counter_sub(&fs_info->dev_replace.bio_counter, amount);
> + cond_wake_up_nomb(&fs_info->dev_replace.replace_wait);
> }
>
> void btrfs_bio_counter_inc_blocked(struct btrfs_fs_info *fs_info)
> {
> while (1) {
> - percpu_counter_inc(&fs_info->bio_counter);
> + percpu_counter_inc(&fs_info->dev_replace.bio_counter);
> if (likely(!test_bit(BTRFS_FS_STATE_DEV_REPLACING,
> &fs_info->fs_state)))
> break;
>
> btrfs_bio_counter_dec(fs_info);
> - wait_event(fs_info->replace_wait,
> + wait_event(fs_info->dev_replace.replace_wait,
> !test_bit(BTRFS_FS_STATE_DEV_REPLACING,
> &fs_info->fs_state));
> }
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index acd0cbc5a6b9..3b3906589d12 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2156,7 +2156,7 @@ static void btrfs_init_dev_replace_locks(struct btrfs_fs_info *fs_info)
> mutex_init(&fs_info->dev_replace.lock_finishing_cancel_unmount);
> rwlock_init(&fs_info->dev_replace.lock);
> atomic_set(&fs_info->dev_replace.blocking_readers, 0);
> - init_waitqueue_head(&fs_info->replace_wait);
> + init_waitqueue_head(&fs_info->dev_replace.replace_wait);
> init_waitqueue_head(&fs_info->dev_replace.read_lock_wq);
> }
>
> @@ -2646,7 +2646,8 @@ int open_ctree(struct super_block *sb,
> goto fail_dirty_metadata_bytes;
> }
>
> - ret = percpu_counter_init(&fs_info->bio_counter, 0, GFP_KERNEL);
> + ret = percpu_counter_init(&fs_info->dev_replace.bio_counter, 0,
> + GFP_KERNEL);
> if (ret) {
> err = ret;
> goto fail_delalloc_bytes;
> @@ -3307,7 +3308,7 @@ int open_ctree(struct super_block *sb,
>
> iput(fs_info->btree_inode);
> fail_bio_counter:
> - percpu_counter_destroy(&fs_info->bio_counter);
> + percpu_counter_destroy(&fs_info->dev_replace.bio_counter);
> fail_delalloc_bytes:
> percpu_counter_destroy(&fs_info->delalloc_bytes);
> fail_dirty_metadata_bytes:
> @@ -4016,7 +4017,7 @@ void close_ctree(struct btrfs_fs_info *fs_info)
>
> percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
> percpu_counter_destroy(&fs_info->delalloc_bytes);
> - percpu_counter_destroy(&fs_info->bio_counter);
> + percpu_counter_destroy(&fs_info->dev_replace.bio_counter);
> cleanup_srcu_struct(&fs_info->subvol_srcu);
>
> btrfs_free_stripe_hash_table(fs_info);
> --
> 2.18.0
>
next prev parent reply other threads:[~2018-09-08 1:52 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-07 14:54 [PATCH 00/11] Cleanup dev-replace locking David Sterba
2018-09-07 14:55 ` [PATCH 01/11] btrfs: remove btrfs_dev_replace::read_locks David Sterba
2018-09-07 14:55 ` [PATCH 02/11] btrfs: open code btrfs_dev_replace_clear_lock_blocking David Sterba
2018-09-07 14:55 ` [PATCH 03/11] btrfs: open code btrfs_dev_replace_stats_inc David Sterba
2018-09-07 21:02 ` Omar Sandoval
2018-09-07 14:55 ` [PATCH 04/11] btrfs: open code btrfs_after_dev_replace_commit David Sterba
2018-09-07 21:03 ` Omar Sandoval
2018-09-07 14:55 ` [PATCH 05/11] btrfs: dev-replace: avoid useless lock on error handling path David Sterba
2018-09-07 21:06 ` Omar Sandoval
2018-09-14 16:53 ` David Sterba
2018-09-07 14:55 ` [PATCH 06/11] btrfs: dev-replace: move replace members out of fs_info David Sterba
2018-09-07 21:09 ` Omar Sandoval [this message]
2018-09-07 14:55 ` [PATCH 07/11] btrfs: dev-replace: remove pointless assert in write unlock David Sterba
2018-09-07 14:55 ` [PATCH 08/11] btrfs: reada: reorder dev-replace locks before radix tree preload David Sterba
2018-09-07 14:55 ` [PATCH 09/11] btrfs: dev-replace: swich locking to rw semaphore David Sterba
2018-09-07 14:55 ` [PATCH 10/11] btrfs: dev-replace: remove custom read/write blocking scheme David Sterba
2018-09-07 14:55 ` [PATCH 11/11] btrfs: dev-replace: open code trivial locking helpers David Sterba
2018-09-27 11:06 ` [PATCH 00/11] Cleanup dev-replace locking David Sterba
2018-10-04 1:06 ` Anand Jain
2018-10-04 10:13 ` David Sterba
2018-10-05 8:58 ` Anand Jain
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=20180907210948.GI29245@vader \
--to=osandov@osandov.com \
--cc=dsterba@suse.com \
--cc=linux-btrfs@vger.kernel.org \
/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).