[PATCH 1/2] btrfs: code reorder in btrfs_prepare_sprout

[PATCH 1/2] btrfs: code reorder in btrfs_prepare_sprout

[Anand Jain]

No functional change, bring the clone of fs_devices and its
operations closer, so that it indicates its purpose.

Also add a comment to indicate why we clone the fs_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b63aae267ebe..0e92969b1adc 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2276,18 +2276,21 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	if (!fs_devices->seeding)
 		return -EINVAL;
 
-	seed_devices = alloc_fs_devices(NULL);
-	if (IS_ERR(seed_devices))
-		return PTR_ERR(seed_devices);
-
+	/*
+	 * This step ensures the scanned seed device remains scanned,
+	 * its not a must for the sprout operation though.
+	 */
 	old_devices = clone_fs_devices(fs_devices);
 	if (IS_ERR(old_devices)) {
 		kfree(seed_devices);
 		return PTR_ERR(old_devices);
 	}
-
 	list_add(&old_devices->fs_list, &fs_uuids);
 
+	seed_devices = alloc_fs_devices(NULL);
+	if (IS_ERR(seed_devices))
+		return PTR_ERR(seed_devices);
+
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
 	seed_devices->opened = 1;
 	INIT_LIST_HEAD(&seed_devices->devices);
-- 
2.7.0

[PATCH 2/2] btrfs: use fs_devices instead of the dereference

[Anand Jain]

We have assigned the %fs_info->fs_devices in %fs_devices as its not
modified just use it for the mutex_lock().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e92969b1adc..781c79a2ff7a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2297,7 +2297,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
 
-	mutex_lock(&fs_info->fs_devices->device_list_mutex);
+	mutex_lock(&fs_devices->device_list_mutex);
 	list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
 			      synchronize_rcu);
 	list_for_each_entry(device, &seed_devices->devices, dev_list)
@@ -2317,7 +2317,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
 	generate_random_uuid(fs_devices->fsid);
 	memcpy(fs_info->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
 	memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
-	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
+	mutex_unlock(&fs_devices->device_list_mutex);
 
 	super_flags = btrfs_super_flags(disk_super) &
 		      ~BTRFS_SUPER_FLAG_SEEDING;
-- 
2.7.0

Re: [PATCH 1/2] btrfs: code reorder in btrfs_prepare_sprout

[David Sterba]

On Mon, Jul 09, 2018 at 04:33:52PM +0800, Anand Jain wrote:
> No functional change, bring the clone of fs_devices and its
> operations closer, so that it indicates its purpose.
> 
> Also add a comment to indicate why we clone the fs_devices.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b63aae267ebe..0e92969b1adc 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2276,18 +2276,21 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>  	if (!fs_devices->seeding)
>  		return -EINVAL;
>  
> -	seed_devices = alloc_fs_devices(NULL);
> -	if (IS_ERR(seed_devices))
> -		return PTR_ERR(seed_devices);
> -
> +	/*
> +	 * This step ensures the scanned seed device remains scanned,
> +	 * its not a must for the sprout operation though.
> +	 */
>  	old_devices = clone_fs_devices(fs_devices);
>  	if (IS_ERR(old_devices)) {
>  		kfree(seed_devices);

seed_devices is uninitialized here.

>  		return PTR_ERR(old_devices);
>  	}
> -
>  	list_add(&old_devices->fs_list, &fs_uuids);
>  
> +	seed_devices = alloc_fs_devices(NULL);
> +	if (IS_ERR(seed_devices))
> +		return PTR_ERR(seed_devices);

If this fails, is it ok to leave the old_devices in the list? I think it
would be better to do all the allocations first and then do the rest so
the error handling is a bit simpler.

> +
>  	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>  	seed_devices->opened = 1;
>  	INIT_LIST_HEAD(&seed_devices->devices);
> -- 
> 2.7.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

Re: [PATCH 1/2] btrfs: code reorder in btrfs_prepare_sprout

[Anand Jain]

On 07/10/2018 06:41 PM, David Sterba wrote:
> On Mon, Jul 09, 2018 at 04:33:52PM +0800, Anand Jain wrote:
>> No functional change, bring the clone of fs_devices and its
>> operations closer, so that it indicates its purpose.
>>
>> Also add a comment to indicate why we clone the fs_devices.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index b63aae267ebe..0e92969b1adc 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2276,18 +2276,21 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>   	if (!fs_devices->seeding)
>>   		return -EINVAL;
>>   
>> -	seed_devices = alloc_fs_devices(NULL);
>> -	if (IS_ERR(seed_devices))
>> -		return PTR_ERR(seed_devices);
>> -
>> +	/*
>> +	 * This step ensures the scanned seed device remains scanned,
>> +	 * its not a must for the sprout operation though.
>> +	 */
>>   	old_devices = clone_fs_devices(fs_devices);
>>   	if (IS_ERR(old_devices)) {
>>   		kfree(seed_devices);
> 
> seed_devices is uninitialized here.

Right. It also provides compile warning. I have fixed it in my WS ready
  to send, its been tested with other patches.

> 
>>   		return PTR_ERR(old_devices);
>>   	}
>> -
>>   	list_add(&old_devices->fs_list, &fs_uuids);
>>   
>> +	seed_devices = alloc_fs_devices(NULL);
>> +	if (IS_ERR(seed_devices))
>> +		return PTR_ERR(seed_devices);
> 
> If this fails, is it ok to leave the old_devices in the list? I think it
> would be better to do all the allocations first and then do the rest so
> the error handling is a bit simpler.

  Err my bad. I have better way to fix this using the
  scan_one_device will send it with a new tile pls ignore this patch.

Thanks, Anand

>> +
>>   	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>>   	seed_devices->opened = 1;
>>   	INIT_LIST_HEAD(&seed_devices->devices);
>> -- 
>> 2.7.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
>