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

[Anand Jain <anand.jain@oracle.com>]

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