[PATCH 0/1] cleanup btrfs_free_extra_devids() drop arg step

[PATCH 0/1] cleanup btrfs_free_extra_devids() drop arg step

[Anand Jain]

Requires on:
 btrfs: dev-replace: fail mount if we don't have replace item with target device
in the mailing list.

Anand Jain (1):
  btrfs: cleanup btrfs_free_extra_devids() drop arg step

 fs/btrfs/disk-io.c | 12 ++++++------
 fs/btrfs/volumes.c |  8 ++++----
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/1] btrfs: cleanup btrfs_free_extra_devids() drop arg step

[Anand Jain]

Following patch
 btrfs: dev-replace: fail mount if we don't have replace item with target device
dropped the multi ops of the function btrfs_free_extra_devids(), where now
it doesn't check the replace target device. So btrfs_free_extra_devid()
doesn't need the 2nd argument %step anymore. Perpetuate the related
changes back to the functions in the stack.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 12 ++++++------
 fs/btrfs/volumes.c |  8 ++++----
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 212806d59012..3b07cce1937e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3156,11 +3156,13 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	}
 
 	/*
-	 * Keep the devid that is marked to be the target device for the
-	 * device replace procedure
+	 * At this point we know all the devices that make this FS, including
+	 * the seed devices but we don't know yet if the replace target is
+	 * required. So free devices not part of this FS but skip the replace
+	 * traget device if any. And the replace target is checked further
+	 * below in btrfs_init_dev_replace().
 	 */
-	btrfs_free_extra_devids(fs_devices, 0);
-
+	btrfs_free_extra_devids(fs_devices);
 	if (!fs_devices->latest_bdev) {
 		btrfs_err(fs_info, "failed to read devices");
 		goto fail_tree_roots;
@@ -3207,8 +3209,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 		goto fail_block_groups;
 	}
 
-	btrfs_free_extra_devids(fs_devices, 1);
-
 	ret = btrfs_sysfs_add_fsid(fs_devices);
 	if (ret) {
 		btrfs_err(fs_info, "failed to init sysfs fsid interface: %d",
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9dd55a6f38de..b08f232170ee 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1038,7 +1038,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 }
 
 static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
-				      int step, struct btrfs_device **latest_dev)
+				      struct btrfs_device **latest_dev)
 {
 	struct btrfs_device *device, *next;
 
@@ -1083,16 +1083,16 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
  * After we have read the system tree and know devids belonging to this
  * filesystem, remove the device which does not belong there.
  */
-void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
+void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *latest_dev = NULL;
 	struct btrfs_fs_devices *seed_dev;
 
 	mutex_lock(&uuid_mutex);
-	__btrfs_free_extra_devids(fs_devices, step, &latest_dev);
+	__btrfs_free_extra_devids(fs_devices, &latest_dev);
 
 	list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list)
-		__btrfs_free_extra_devids(seed_dev, step, &latest_dev);
+		__btrfs_free_extra_devids(seed_dev, &latest_dev);
 
 	fs_devices->latest_bdev = latest_dev->bdev;
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index bd95cea85a65..5e08274d1252 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -449,7 +449,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   fmode_t flags, void *holder);
 int btrfs_forget_devices(const char *path);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
-void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
+void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
 void btrfs_assign_next_active_device(struct btrfs_device *device,
 				     struct btrfs_device *this_dev);
 struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info,
-- 
2.25.1

Re: [PATCH 1/1] btrfs: cleanup btrfs_free_extra_devids() drop arg step

[Josef Bacik]

On 11/6/20 3:06 AM, Anand Jain wrote:
> Following patch
>   btrfs: dev-replace: fail mount if we don't have replace item with target device
> dropped the multi ops of the function btrfs_free_extra_devids(), where now

This is jumbled together so I was confused, write it like

The following patch
	
	btrfs: dev-replace: fail mount if we don't have replace item with target device

dropped the usage of the step argument in btrfs_free_extra_devids()

Other than that the code is fine, you can fix that up and add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 1/1] btrfs: cleanup btrfs_free_extra_devids() drop arg step

[Josef Bacik]

On 11/6/20 3:06 AM, Anand Jain wrote:
> Following patch
>   btrfs: dev-replace: fail mount if we don't have replace item with target device
> dropped the multi ops of the function btrfs_free_extra_devids(), where now
> it doesn't check the replace target device. So btrfs_free_extra_devid()
> doesn't need the 2nd argument %step anymore. Perpetuate the related
> changes back to the functions in the stack.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Actually nm,  I forgot to build, this thing doesn't compile.  Thanks,

Josef

Re: [PATCH 1/1] btrfs: cleanup btrfs_free_extra_devids() drop arg step

[Anand Jain]

On 7/11/20 1:02 am, Josef Bacik wrote:
> On 11/6/20 3:06 AM, Anand Jain wrote:
>> Following patch
>>   btrfs: dev-replace: fail mount if we don't have replace item with 
>> target device
>> dropped the multi ops of the function btrfs_free_extra_devids(), where 
>> now
>> it doesn't check the replace target device. So btrfs_free_extra_devid()
>> doesn't need the 2nd argument %step anymore. Perpetuate the related
>> changes back to the functions in the stack.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Actually nm,  I forgot to build, this thing doesn't compile.  Thanks,

  The compile fail is not real to this patch. It happened due change in
  patch's order in the misc-next. In specific, the compile fail happens
  due to the dependent patch as below,

    btrfs: fix btrfs_find_device unused arg seed

  which I fixed locally as below. As David is reviewing the above patch,
  I didn't want to send another revision and confuse him more. To compile
  successfully, pls, after the above patch apply for the following
  changes,

-----------------------------------------------
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index f6e1a4b0ae84..dc3481014f16 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -96,7 +96,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
                  * a replace target, fail the mount.
                  */
                 if (btrfs_find_device(fs_info->fs_devices,
-                                     BTRFS_DEV_REPLACE_DEVID, NULL, 
NULL, false)) {
+                                     BTRFS_DEV_REPLACE_DEVID, NULL, 
NULL)) {
                         btrfs_err(fs_info,
                         "found replace target device without a valid 
replace item");
                         ret = -EUCLEAN;
@@ -159,7 +159,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info 
*fs_info)
                  * replace target, fail the mount.
                  */
                 if (btrfs_find_device(fs_info->fs_devices,
-                                     BTRFS_DEV_REPLACE_DEVID, NULL, 
NULL, false)) {
+                                     BTRFS_DEV_REPLACE_DEVID, NULL, 
NULL)) {
                         btrfs_err(fs_info,
                         "replace devid present without an active 
replace item");
                         ret = -EUCLEAN;
-------------------------

When this patch was sent, it was tested with fstests btrfs and finds no 
new regression.

Thanks, Anand

> 
> Josef

Re: [PATCH 1/1] btrfs: cleanup btrfs_free_extra_devids() drop arg step

[David Sterba]

On Sat, Nov 07, 2020 at 07:41:01AM +0800, Anand Jain wrote:
> 
> 
> On 7/11/20 1:02 am, Josef Bacik wrote:
> > On 11/6/20 3:06 AM, Anand Jain wrote:
> >> Following patch
> >>   btrfs: dev-replace: fail mount if we don't have replace item with 
> >> target device
> >> dropped the multi ops of the function btrfs_free_extra_devids(), where 
> >> now
> >> it doesn't check the replace target device. So btrfs_free_extra_devid()
> >> doesn't need the 2nd argument %step anymore. Perpetuate the related
> >> changes back to the functions in the stack.
> >>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > 
> > Actually nm,  I forgot to build, this thing doesn't compile.  Thanks,
> 
>   The compile fail is not real to this patch. It happened due change in
>   patch's order in the misc-next. In specific, the compile fail happens
>   due to the dependent patch as below,
> 
>     btrfs: fix btrfs_find_device unused arg seed
> 
>   which I fixed locally as below. As David is reviewing the above patch,
>   I didn't want to send another revision and confuse him more. To compile
>   successfully, pls, after the above patch apply for the following
>   changes,

With so many cleanup patches floating around compilation failure due to
some dependency is not a problem. I'll notice and if there's
something beyond trivial to fix it I'd let you know and ask for a
resend.

The patch "btrfs: dev-replace: fail mount if we don't have replace item
with" got merged to master meanwhile so I've updated the reference to
also include the commit id. With that the patch is now in misc-next,
thanks.