[PATCH 1/3] btrfs: make code easy to read in btrfs_open_one_device()

[PATCH 1/3] btrfs: make code easy to read in btrfs_open_one_device()

[Anand Jain]

No functional change. First set the usual case, writeable then check
for any special config.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5a4c30451c7f..a81574dba124 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -676,14 +676,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 
 	device->generation = btrfs_super_generation(disk_super);
 
+	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING) {
 		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 		fs_devices->seeding = 1;
-	} else {
-		if (bdev_read_only(bdev))
-			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-		else
-			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
+	} else if (bdev_read_only(bdev)) {
+		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	}
 
 	q = bdev_get_queue(bdev);
-- 
2.7.0

[PATCH 2/3] btrfs: optimize move uuid_mutex closer to the critical section

[Anand Jain]

Move uuid_mutex closer to the exclusion section.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a81574dba124..a7eb9b42cd92 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1187,13 +1187,10 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	 */
 	bytenr = btrfs_sb_offset(0);
 	flags |= FMODE_EXCL;
-	mutex_lock(&uuid_mutex);
 
 	bdev = blkdev_get_by_path(path, flags, holder);
-	if (IS_ERR(bdev)) {
-		ret = PTR_ERR(bdev);
-		goto error;
-	}
+	if (IS_ERR(bdev))
+		return PTR_ERR(bdev);
 
 	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
 		goto error_bdev_put;
@@ -1202,7 +1199,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	transid = btrfs_super_generation(disk_super);
 	total_devices = btrfs_super_num_devices(disk_super);
 
+	mutex_lock(&uuid_mutex);
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
+	if (ret >= 0 && fs_devices_ret)
+		(*fs_devices_ret)->total_devices = total_devices;
+	mutex_unlock(&uuid_mutex);
+
 	if (ret > 0) {
 		if (disk_super->label[0]) {
 			pr_info("BTRFS: device label %s ", disk_super->label);
@@ -1213,15 +1215,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 		pr_cont("devid %llu transid %llu %s\n", devid, transid, path);
 		ret = 0;
 	}
-	if (!ret && fs_devices_ret)
-		(*fs_devices_ret)->total_devices = total_devices;
 
 	btrfs_release_disk_super(page);
 
 error_bdev_put:
 	blkdev_put(bdev, flags);
-error:
-	mutex_unlock(&uuid_mutex);
+
 	return ret;
 }
 
-- 
2.7.0

[PATCH 3/3] btrfs: misc cleanup btrfs_scan_one_device()

[Anand Jain]

Assign ret = -EINVAL where actually its required.
Remove { } around single line if else code.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a7eb9b42cd92..ba06a4ccd5a6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1173,7 +1173,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	struct btrfs_super_block *disk_super;
 	struct block_device *bdev;
 	struct page *page;
-	int ret = -EINVAL;
+	int ret;
 	u64 devid;
 	u64 transid;
 	u64 total_devices;
@@ -1192,8 +1192,10 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
+	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
+		ret = -EINVAL;
 		goto error_bdev_put;
+	}
 
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
 	transid = btrfs_super_generation(disk_super);
@@ -1206,11 +1208,10 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	mutex_unlock(&uuid_mutex);
 
 	if (ret > 0) {
-		if (disk_super->label[0]) {
+		if (disk_super->label[0])
 			pr_info("BTRFS: device label %s ", disk_super->label);
-		} else {
+		else
 			pr_info("BTRFS: device fsid %pU ", disk_super->fsid);
-		}
 
 		pr_cont("devid %llu transid %llu %s\n", devid, transid, path);
 		ret = 0;
-- 
2.7.0

Re: [PATCH 1/3] btrfs: make code easy to read in btrfs_open_one_device()

[David Sterba]

On Fri, Dec 15, 2017 at 03:40:14PM +0800, Anand Jain wrote:
> No functional change. First set the usual case, writeable then check
> for any special config.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5a4c30451c7f..a81574dba124 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -676,14 +676,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  
>  	device->generation = btrfs_super_generation(disk_super);
>  
> +	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);

I would not say there's no functional change. This line will
unconditionally set the writeable flag, but this was not the case
before.

Sure it's dropped a few lines below, but this would need some checking
that it's not a problem. btrfs_open_one_device is indirectly called from
mount so it should be safe (we can't use one device twice), but this
needs to be documented.

>  	if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING) {
>  		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  		fs_devices->seeding = 1;
> -	} else {
> -		if (bdev_read_only(bdev))
> -			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> -		else
> -			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> +	} else if (bdev_read_only(bdev)) {
> +		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	}
>  
>  	q = bdev_get_queue(bdev);
> -- 
> 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 2/3] btrfs: optimize move uuid_mutex closer to the critical section

[David Sterba]

On Fri, Dec 15, 2017 at 03:40:15PM +0800, Anand Jain wrote:
> Move uuid_mutex closer to the exclusion section.

Looks good, there's really something unrelated inside the critical
section so this could potentially speed up scanning devices.

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 3/3] btrfs: misc cleanup btrfs_scan_one_device()

[David Sterba]

On Fri, Dec 15, 2017 at 03:40:16PM +0800, Anand Jain wrote:
> Assign ret = -EINVAL where actually its required.
> Remove { } around single line if else code.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Added to next, thanks.

Re: [PATCH 1/3] btrfs: make code easy to read in btrfs_open_one_device()

[Anand Jain]

On 01/05/2018 10:09 PM, David Sterba wrote:
> On Fri, Dec 15, 2017 at 03:40:14PM +0800, Anand Jain wrote:
>> No functional change. First set the usual case, writeable then check
>> for any special config.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 5a4c30451c7f..a81574dba124 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -676,14 +676,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>>   
>>   	device->generation = btrfs_super_generation(disk_super);
>>   
>> +	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> 
> I would not say there's no functional change. This line will
> unconditionally set the writeable flag, but this was not the case
> before.

  (Sorry for the delay, got distracted by other patches).

  agreed here.

> Sure it's dropped a few lines below, but this would need some checking
> that it's not a problem. btrfs_open_one_device is indirectly called from
> mount so it should be safe (we can't use one device twice), but this
> needs to be documented.

  Sure will do.

Thanks, Anand


>>   	if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING) {
>>   		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>   		fs_devices->seeding = 1;
>> -	} else {
>> -		if (bdev_read_only(bdev))
>> -			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>> -		else
>> -			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>> +	} else if (bdev_read_only(bdev)) {
>> +		clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>   	}
>>   
>>   	q = bdev_get_queue(bdev);
>> -- 
>> 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