[PATCH] btrfs: cleanup finding rotating device

[PATCH] btrfs: cleanup finding rotating device

[Anand Jain]

The pointer to struct request_queue is used only to get device type
rotating or the non-rotating. So use it directly.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7e92bf130137..68ea15e8e3cb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -609,7 +609,6 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 			struct btrfs_device *device, fmode_t flags,
 			void *holder)
 {
-	struct request_queue *q;
 	struct block_device *bdev;
 	struct btrfs_super_block *disk_super;
 	u64 devid;
@@ -651,8 +650,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	}
 
-	q = bdev_get_queue(bdev);
-	if (!blk_queue_nonrot(q))
+	if (!blk_queue_nonrot(bdev_get_queue(bdev)))
 		fs_devices->rotating = true;
 
 	device->bdev = bdev;
@@ -2600,7 +2598,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
 {
 	struct btrfs_root *root = fs_info->dev_root;
-	struct request_queue *q;
 	struct btrfs_trans_handle *trans;
 	struct btrfs_device *device;
 	struct block_device *bdev;
@@ -2676,7 +2673,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		goto error_free_zone;
 	}
 
-	q = bdev_get_queue(bdev);
 	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	device->generation = trans->transid;
 	device->io_width = fs_info->sectorsize;
@@ -2724,7 +2720,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	atomic64_add(device->total_bytes, &fs_info->free_chunk_space);
 
-	if (!blk_queue_nonrot(q))
+	if (!blk_queue_nonrot(bdev_get_queue(bdev)))
 		fs_devices->rotating = true;
 
 	orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
-- 
2.33.1

Re: [PATCH] btrfs: cleanup finding rotating device

[Nikolay Borisov]

On 13.01.22 г. 6:44, Anand Jain wrote:
> The pointer to struct request_queue is used only to get device type
> rotating or the non-rotating. So use it directly.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7e92bf130137..68ea15e8e3cb 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -609,7 +609,6 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  			struct btrfs_device *device, fmode_t flags,
>  			void *holder)
>  {
> -	struct request_queue *q;
>  	struct block_device *bdev;
>  	struct btrfs_super_block *disk_super;
>  	u64 devid;
> @@ -651,8 +650,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	}
>  
> -	q = bdev_get_queue(bdev);
> -	if (!blk_queue_nonrot(q))
> +	if (!blk_queue_nonrot(bdev_get_queue(bdev)))
>  		fs_devices->rotating = true;

Eliminate the altogether:

fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));

>  
>  	device->bdev = bdev;
> @@ -2600,7 +2598,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>  int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
>  {
>  	struct btrfs_root *root = fs_info->dev_root;
> -	struct request_queue *q;
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_device *device;
>  	struct block_device *bdev;
> @@ -2676,7 +2673,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  		goto error_free_zone;
>  	}
>  
> -	q = bdev_get_queue(bdev);
>  	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	device->generation = trans->transid;
>  	device->io_width = fs_info->sectorsize;
> @@ -2724,7 +2720,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  
>  	atomic64_add(device->total_bytes, &fs_info->free_chunk_space);
>  
> -	if (!blk_queue_nonrot(q))
> +	if (!blk_queue_nonrot(bdev_get_queue(bdev)))
>  		fs_devices->rotating = true;

Ditto

>  
>  	orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
> 

[PATCH v2] btrfs: cleanup finding rotating device

[Anand Jain]

The pointer to struct request_queue is used only to get device type
rotating or the non-rotating. So use it directly.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Eliminate the if statement.

 fs/btrfs/volumes.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 70b085dff500..34d08c4f7b6c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -601,7 +601,6 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 			struct btrfs_device *device, fmode_t flags,
 			void *holder)
 {
-	struct request_queue *q;
 	struct block_device *bdev;
 	struct btrfs_super_block *disk_super;
 	u64 devid;
@@ -643,9 +642,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	}
 
-	q = bdev_get_queue(bdev);
-	if (!blk_queue_nonrot(q))
-		fs_devices->rotating = true;
+	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));
 
 	device->bdev = bdev;
 	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
@@ -2590,7 +2587,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
 {
 	struct btrfs_root *root = fs_info->dev_root;
-	struct request_queue *q;
 	struct btrfs_trans_handle *trans;
 	struct btrfs_device *device;
 	struct block_device *bdev;
@@ -2666,7 +2662,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		goto error_free_zone;
 	}
 
-	q = bdev_get_queue(bdev);
 	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	device->generation = trans->transid;
 	device->io_width = fs_info->sectorsize;
@@ -2714,8 +2709,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	atomic64_add(device->total_bytes, &fs_info->free_chunk_space);
 
-	if (!blk_queue_nonrot(q))
-		fs_devices->rotating = true;
+	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));
 
 	orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
 	btrfs_set_super_total_bytes(fs_info->super_copy,
-- 
2.33.1

Re: [PATCH v2] btrfs: cleanup finding rotating device

[Nikolay Borisov]

On 13.01.22 г. 9:48, Anand Jain wrote:
> The pointer to struct request_queue is used only to get device type
> rotating or the non-rotating. So use it directly.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH v2] btrfs: cleanup finding rotating device

[David Sterba]

On Thu, Jan 13, 2022 at 03:48:54PM +0800, Anand Jain wrote:
> The pointer to struct request_queue is used only to get device type
> rotating or the non-rotating. So use it directly.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Eliminate the if statement.
> 
>  fs/btrfs/volumes.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 70b085dff500..34d08c4f7b6c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -601,7 +601,6 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  			struct btrfs_device *device, fmode_t flags,
>  			void *holder)
>  {
> -	struct request_queue *q;
>  	struct block_device *bdev;
>  	struct btrfs_super_block *disk_super;
>  	u64 devid;
> @@ -643,9 +642,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	}
>  
> -	q = bdev_get_queue(bdev);
> -	if (!blk_queue_nonrot(q))
> -		fs_devices->rotating = true;
> +	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));

This is an equivalent change, in the new code fs_devices->rotating will
be always set according to the last device, while in the old code, any
rotational device will set it to true and never back.

>  
>  	device->bdev = bdev;
>  	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
> @@ -2590,7 +2587,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>  int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
>  {
>  	struct btrfs_root *root = fs_info->dev_root;
> -	struct request_queue *q;
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_device *device;
>  	struct block_device *bdev;
> @@ -2666,7 +2662,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  		goto error_free_zone;
>  	}
>  
> -	q = bdev_get_queue(bdev);
>  	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	device->generation = trans->transid;
>  	device->io_width = fs_info->sectorsize;
> @@ -2714,8 +2709,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  
>  	atomic64_add(device->total_bytes, &fs_info->free_chunk_space);
>  
> -	if (!blk_queue_nonrot(q))
> -		fs_devices->rotating = true;
> +	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));

Same here.

>  
>  	orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
>  	btrfs_set_super_total_bytes(fs_info->super_copy,
> -- 
> 2.33.1

Re: [PATCH v2] btrfs: cleanup finding rotating device

[Anand Jain]

On 18/01/2022 23:43, David Sterba wrote:
> On Thu, Jan 13, 2022 at 03:48:54PM +0800, Anand Jain wrote:
>> The pointer to struct request_queue is used only to get device type
>> rotating or the non-rotating. So use it directly.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2: Eliminate the if statement.
>>
>>   fs/btrfs/volumes.c | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 70b085dff500..34d08c4f7b6c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -601,7 +601,6 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>>   			struct btrfs_device *device, fmode_t flags,
>>   			void *holder)
>>   {
>> -	struct request_queue *q;
>>   	struct block_device *bdev;
>>   	struct btrfs_super_block *disk_super;
>>   	u64 devid;
>> @@ -643,9 +642,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>>   			set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>   	}
>>   
>> -	q = bdev_get_queue(bdev);
>> -	if (!blk_queue_nonrot(q))
>> -		fs_devices->rotating = true;
>> +	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));
> 
> This is an equivalent change, in the new code fs_devices->rotating will
> be always set according to the last device, while in the old code, any
> rotational device will set it to true and never back.
> 

Oh. The previous status gets overwritten based on the current device
non-rot check. This is not good. My bad. However, V1 is ok could you
pls consider that?

Also, there is a bug in the original code - If we have at least one
rotating device (HDD) we won't enable SSD benefit. However, if that
HDD is deleted later on, we won't still enable the SSD status. This
bug can be fixed easily on top of the patch

  '[PATCH 1/2] btrfs: keep device type in the struct btrfs_device'

  I will wait for some comments before fixing this.

Thanks, Anand

>>   
>>   	device->bdev = bdev;
>>   	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
>> @@ -2590,7 +2587,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>>   int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
>>   {
>>   	struct btrfs_root *root = fs_info->dev_root;
>> -	struct request_queue *q;
>>   	struct btrfs_trans_handle *trans;
>>   	struct btrfs_device *device;
>>   	struct block_device *bdev;
>> @@ -2666,7 +2662,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   		goto error_free_zone;
>>   	}
>>   
>> -	q = bdev_get_queue(bdev);
>>   	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>   	device->generation = trans->transid;
>>   	device->io_width = fs_info->sectorsize;
>> @@ -2714,8 +2709,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   
>>   	atomic64_add(device->total_bytes, &fs_info->free_chunk_space);
>>   
>> -	if (!blk_queue_nonrot(q))
>> -		fs_devices->rotating = true;
>> +	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));
> 
> Same here.
> 
>>   
>>   	orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
>>   	btrfs_set_super_total_bytes(fs_info->super_copy,
>> -- 
>> 2.33.1

Re: [PATCH v2] btrfs: cleanup finding rotating device

[David Sterba]

On Wed, Jan 19, 2022 at 03:43:42PM +0800, Anand Jain wrote:
> On 18/01/2022 23:43, David Sterba wrote:
> > On Thu, Jan 13, 2022 at 03:48:54PM +0800, Anand Jain wrote:
> >> -	q = bdev_get_queue(bdev);
> >> -	if (!blk_queue_nonrot(q))
> >> -		fs_devices->rotating = true;
> >> +	fs_devices->rotating = !blk_queue_nonrot(bdev_get_queue(bdev));
> > 
> > This is an equivalent change, in the new code fs_devices->rotating will
> > be always set according to the last device, while in the old code, any
> > rotational device will set it to true and never back.
> 
> Oh. The previous status gets overwritten based on the current device
> non-rot check. This is not good. My bad. However, V1 is ok could you
> pls consider that?

Yes, v1 applied to misc-next, thanks.

> Also, there is a bug in the original code - If we have at least one
> rotating device (HDD) we won't enable SSD benefit. However, if that
> HDD is deleted later on, we won't still enable the SSD status. This
> bug can be fixed easily on top of the patch
> 
>   '[PATCH 1/2] btrfs: keep device type in the struct btrfs_device'
> 
>   I will wait for some comments before fixing this.

Right, I think that mixed HDD/SSD devices were not considered and
keeping the status should be fixed.