| Message ID | 1462143728-3000-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 02, 2016 at 07:02:08AM +0800, Anand Jain wrote: > --- a/fs/btrfs/dev-replace.c > +++ b/fs/btrfs/dev-replace.c > @@ -569,11 +569,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > ASSERT(list_empty(&src_device->resized_list)); > tgt_device->commit_total_bytes = src_device->commit_total_bytes; > tgt_device->commit_bytes_used = src_device->bytes_used; > - if (fs_info->sb->s_bdev && > - (fs_info->sb->s_bdev == src_device->bdev)) > - fs_info->sb->s_bdev = tgt_device->bdev; What's the base of this patch? The above code is not in my for-next so I could be missing some important bits. > - if (fs_info->fs_devices->latest_bdev == src_device->bdev) > - fs_info->fs_devices->latest_bdev = tgt_device->bdev; > + > + btrfs_assign_next_active_device(fs_info, src_device, tgt_device); > + > list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list); > fs_info->fs_devices->rw_devices++; > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 5f70c1235466..0bb15da2da40 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1742,10 +1742,49 @@ out: > return ret; > } > > +struct btrfs_device *btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs, > + struct btrfs_device *device) > +{ > + struct btrfs_device *next_device; > + > + list_for_each_entry(next_device, &fs_devs->devices, dev_list) { > + if (next_device != device && > + !next_device->missing && next_device->bdev) > + return next_device; > + } > + return NULL; > +} > + > +/* > + * Helper function to check if the given device is part of > + * s_bdev / latest_bdev and replace it with the provided or > + * the next active device, in the context where this function > + * called, there should be always be another device which is > + * active. > + */ > +void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info, > + struct btrfs_device *device, struct btrfs_device *this_dev) > +{ > + struct btrfs_device *next_device; > + > + if (this_dev) > + next_device = this_dev; > + else > + next_device = btrfs_find_next_active_device(fs_info->fs_devices, > + device); > + BUG_ON(!next_device); /* Logic error */ Please make it an ASSERT. -- 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 05/02/2016 11:26 PM, David Sterba wrote: > On Mon, May 02, 2016 at 07:02:08AM +0800, Anand Jain wrote: >> --- a/fs/btrfs/dev-replace.c >> +++ b/fs/btrfs/dev-replace.c >> @@ -569,11 +569,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, >> ASSERT(list_empty(&src_device->resized_list)); >> tgt_device->commit_total_bytes = src_device->commit_total_bytes; >> tgt_device->commit_bytes_used = src_device->bytes_used; >> - if (fs_info->sb->s_bdev && >> - (fs_info->sb->s_bdev == src_device->bdev)) >> - fs_info->sb->s_bdev = tgt_device->bdev; > > What's the base of this patch? It was on master. But now the V3 which is based on your for-next. > The above code is not in my for-next so > I could be missing some important bits. Yes. That was added by this patch. [PATCH] btrfs: s_bdev is not null after missing replace While here can also integrate this. [PATCH 1/1] btrfs: fix lock dep warning move scratch super outside of chunk_mutex ------ git status On branch for-next Your branch is ahead of 'origin/for-next' by 3 commits. (use "git push" to publish your local commits) nothing to commit, working directory clean git log --oneline | head -n 3 ac3fd7a65d23 btrfs: cleanup assigning next active device with a check 0fbd788ec3d9 btrfs: s_bdev is not null after missing replace 8362f084ffd6 btrfs: fix lock dep warning move scratch super outside of chunk_mutex ----- I have run xfstests on for-next I don't see any unusual failures. Thanks, Anand >> - if (fs_info->fs_devices->latest_bdev == src_device->bdev) >> - fs_info->fs_devices->latest_bdev = tgt_device->bdev; >> + >> + btrfs_assign_next_active_device(fs_info, src_device, tgt_device); >> + >> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list); >> fs_info->fs_devices->rw_devices++; >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 5f70c1235466..0bb15da2da40 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -1742,10 +1742,49 @@ out: >> return ret; >> } >> >> +struct btrfs_device *btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs, >> + struct btrfs_device *device) >> +{ >> + struct btrfs_device *next_device; >> + >> + list_for_each_entry(next_device, &fs_devs->devices, dev_list) { >> + if (next_device != device && >> + !next_device->missing && next_device->bdev) >> + return next_device; >> + } >> + return NULL; >> +} >> + >> +/* >> + * Helper function to check if the given device is part of >> + * s_bdev / latest_bdev and replace it with the provided or >> + * the next active device, in the context where this function >> + * called, there should be always be another device which is >> + * active. >> + */ >> +void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info, >> + struct btrfs_device *device, struct btrfs_device *this_dev) >> +{ >> + struct btrfs_device *next_device; >> + >> + if (this_dev) >> + next_device = this_dev; >> + else >> + next_device = btrfs_find_next_active_device(fs_info->fs_devices, >> + device); >> + BUG_ON(!next_device); /* Logic error */ > > Please make it an ASSERT. > -- > 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 Tue, May 03, 2016 at 06:01:31PM +0800, Anand Jain wrote: > > > On 05/02/2016 11:26 PM, David Sterba wrote: > > On Mon, May 02, 2016 at 07:02:08AM +0800, Anand Jain wrote: > >> --- a/fs/btrfs/dev-replace.c > >> +++ b/fs/btrfs/dev-replace.c > >> @@ -569,11 +569,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > >> ASSERT(list_empty(&src_device->resized_list)); > >> tgt_device->commit_total_bytes = src_device->commit_total_bytes; > >> tgt_device->commit_bytes_used = src_device->bytes_used; > >> - if (fs_info->sb->s_bdev && > >> - (fs_info->sb->s_bdev == src_device->bdev)) > >> - fs_info->sb->s_bdev = tgt_device->bdev; > > > > What's the base of this patch? > > It was on master. But now the V3 which is based on your for-next. > > > The above code is not in my for-next so > > I could be missing some important bits. > > Yes. That was added by this patch. > > [PATCH] btrfs: s_bdev is not null after missing replace Ah, that's the one. Added to next together with v3, though a testcase is still desirable. > While here can also integrate this. > > [PATCH 1/1] btrfs: fix lock dep warning move scratch super outside of > chunk_mutex That one has been in for-next for some time. -- 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
>> While here can also integrate this. >> >> [PATCH 1/1] btrfs: fix lock dep warning move scratch super outside of >> chunk_mutex > > That one has been in for-next for some time. There is another patch which is similarly named (sorry about that), I see that one but not this. --------- git log -i --author=anand.jain@oracle.com --oneline | egrep "btrfs: fix lock dep" 58681ad85b9f btrfs: fix lock dep warning, move scratch dev out of device_list_mutex and uuid_mutex git status On branch for-next Your branch is up-to-date with 'origin/for-next'. nothing to commit, working directory clean ---------- Thanks, Anand -- 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 Tue, May 03, 2016 at 07:47:28PM +0200, David Sterba wrote: > On Tue, May 03, 2016 at 06:01:31PM +0800, Anand Jain wrote: > > > > > > On 05/02/2016 11:26 PM, David Sterba wrote: > > > On Mon, May 02, 2016 at 07:02:08AM +0800, Anand Jain wrote: > > >> --- a/fs/btrfs/dev-replace.c > > >> +++ b/fs/btrfs/dev-replace.c > > >> @@ -569,11 +569,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, > > >> ASSERT(list_empty(&src_device->resized_list)); > > >> tgt_device->commit_total_bytes = src_device->commit_total_bytes; > > >> tgt_device->commit_bytes_used = src_device->bytes_used; > > >> - if (fs_info->sb->s_bdev && > > >> - (fs_info->sb->s_bdev == src_device->bdev)) > > >> - fs_info->sb->s_bdev = tgt_device->bdev; > > > > > > What's the base of this patch? > > > > It was on master. But now the V3 which is based on your for-next. > > > > > The above code is not in my for-next so > > > I could be missing some important bits. > > > > Yes. That was added by this patch. > > > > [PATCH] btrfs: s_bdev is not null after missing replace > > Ah, that's the one. Added to next together with v3, though a testcase is > still desirable. There were some merge conflicts with the dev-del-by-id patchset so I've put the patches there. -- 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 Wed, May 04, 2016 at 07:31:55AM +0800, Anand Jain wrote: > > >> While here can also integrate this. > >> > >> [PATCH 1/1] btrfs: fix lock dep warning move scratch super outside of > >> chunk_mutex > > > > That one has been in for-next for some time. > > There is another patch which is similarly named (sorry about that), > I see that one but not this. > > --------- > git log -i --author=anand.jain@oracle.com --oneline | egrep "btrfs: fix > lock dep" > 58681ad85b9f btrfs: fix lock dep warning, move scratch dev out of > device_list_mutex and uuid_mutex I see, the subjects partially differ in the last word, that's easy to miss and I got te 'chunk_mutex' patch marked as dropped as it looked like the other patch superseded it. https://patchwork.kernel.org/patch/8868771/ -- in next https://patchwork.kernel.org/patch/8810591/ -- pending -- 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/dev-replace.c b/fs/btrfs/dev-replace.c index 6b37803a1a4a..50d0ed2fad2e 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -569,11 +569,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info, ASSERT(list_empty(&src_device->resized_list)); tgt_device->commit_total_bytes = src_device->commit_total_bytes; tgt_device->commit_bytes_used = src_device->bytes_used; - if (fs_info->sb->s_bdev && - (fs_info->sb->s_bdev == src_device->bdev)) - fs_info->sb->s_bdev = tgt_device->bdev; - if (fs_info->fs_devices->latest_bdev == src_device->bdev) - fs_info->fs_devices->latest_bdev = tgt_device->bdev; + + btrfs_assign_next_active_device(fs_info, src_device, tgt_device); + list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list); fs_info->fs_devices->rw_devices++; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 5f70c1235466..0bb15da2da40 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1742,10 +1742,49 @@ out: return ret; } +struct btrfs_device *btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs, + struct btrfs_device *device) +{ + struct btrfs_device *next_device; + + list_for_each_entry(next_device, &fs_devs->devices, dev_list) { + if (next_device != device && + !next_device->missing && next_device->bdev) + return next_device; + } + return NULL; +} + +/* + * Helper function to check if the given device is part of + * s_bdev / latest_bdev and replace it with the provided or + * the next active device, in the context where this function + * called, there should be always be another device which is + * active. + */ +void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info, + struct btrfs_device *device, struct btrfs_device *this_dev) +{ + struct btrfs_device *next_device; + + if (this_dev) + next_device = this_dev; + else + next_device = btrfs_find_next_active_device(fs_info->fs_devices, + device); + BUG_ON(!next_device); /* Logic error */ + + if (fs_info->sb->s_bdev && + (fs_info->sb->s_bdev == device->bdev)) + fs_info->sb->s_bdev = next_device->bdev; + + if (fs_info->fs_devices->latest_bdev == device->bdev) + fs_info->fs_devices->latest_bdev = next_device->bdev; +} + int btrfs_rm_device(struct btrfs_root *root, char *device_path) { struct btrfs_device *device; - struct btrfs_device *next_device; struct block_device *bdev; struct buffer_head *bh = NULL; struct btrfs_super_block *disk_super; @@ -1896,13 +1935,7 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path) if (device->missing) device->fs_devices->missing_devices--; - next_device = list_entry(root->fs_info->fs_devices->devices.next, - struct btrfs_device, dev_list); - if (root->fs_info->sb->s_bdev && - (root->fs_info->sb->s_bdev == device->bdev)) - root->fs_info->sb->s_bdev = next_device->bdev; - if (device->bdev == root->fs_info->fs_devices->latest_bdev) - root->fs_info->fs_devices->latest_bdev = next_device->bdev; + btrfs_assign_next_active_device(root->fs_info, device, NULL); if (device->bdev) { device->fs_devices->open_devices--; @@ -2069,8 +2102,6 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info, void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, struct btrfs_device *tgtdev) { - struct btrfs_device *next_device; - mutex_lock(&uuid_mutex); WARN_ON(!tgtdev); mutex_lock(&fs_info->fs_devices->device_list_mutex); @@ -2082,13 +2113,8 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, fs_info->fs_devices->num_devices--; - next_device = list_entry(fs_info->fs_devices->devices.next, - struct btrfs_device, dev_list); - if (fs_info->sb->s_bdev && - (tgtdev->bdev == fs_info->sb->s_bdev)) - fs_info->sb->s_bdev = next_device->bdev; - if (tgtdev->bdev == fs_info->fs_devices->latest_bdev) - fs_info->fs_devices->latest_bdev = next_device->bdev; + btrfs_assign_next_active_device(fs_info, tgtdev, NULL); + list_del_rcu(&tgtdev->dev_list); mutex_unlock(&fs_info->fs_devices->device_list_mutex); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 18c01739d46b..dbb018d82ec6 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -464,6 +464,8 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder, struct btrfs_fs_devices **fs_devices_ret); int btrfs_close_devices(struct btrfs_fs_devices *fs_devices); void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step); +void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info, + struct btrfs_device *device, struct btrfs_device *this_dev); int btrfs_find_device_missing_or_by_path(struct btrfs_root *root, char *device_path, struct btrfs_device **device);
Creates helper fucntion as needed by the device delete and replace operations. Also now it checks if the next device being assigned is an active device. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: added comments, and BUG_ON if we can't find next_device. fs/btrfs/dev-replace.c | 8 +++---- fs/btrfs/volumes.c | 60 ++++++++++++++++++++++++++++++++++++-------------- fs/btrfs/volumes.h | 2 ++ 3 files changed, 48 insertions(+), 22 deletions(-)