Re: [PATCH v3 2/4] btrfs: redeclare btrfs_stale_devices arg1 to dev_t

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

On 08/01/2022 00:16, David Sterba wrote:
> On Fri, Jan 07, 2022 at 09:04:14PM +0800, Anand Jain wrote:
>> After the commit cb57afa39796 ("btrfs: harden identification of the stale
> 
> Please drop the commit id when referencing patches that haven't been in
> released branch, the subject should be sufficient.

Got it.

>> @@ -2405,7 +2406,12 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>>   		mutex_unlock(&uuid_mutex);
>>   		break;
>>   	case BTRFS_IOC_FORGET_DEV:
>> -		ret = btrfs_forget_devices(vol->name);
>> +		if (strlen(vol->name)) {
> 
> The full strlen() is not necessary, just check the first byte.

Yep. Will update.


>> @@ -1427,8 +1422,12 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
>>   
>>   	device = device_list_add(path, disk_super, &new_device_added);
>>   	if (!IS_ERR(device)) {
>> -		if (new_device_added)
>> -			btrfs_free_stale_devices(path, device);
>> +		if (new_device_added) {
>> +			dev_t devt;
>> +
>> +			if (!lookup_bdev(path, &devt))
> 
> This reads like some negative condition, while what we expect is "if the
> device exists and can be looked up" and not "if we can't look up the
> device". Please write it like (lookup_bdev(...) == 0), and maybe add a
> comment why we can ignore errors.
> 

Ah. That's much easier to read. Changed in v4.


>> +				btrfs_free_stale_devices(devt, device);
>> +		}
>>   	}
>>   
>>   	btrfs_release_disk_super(disk_super);
>> @@ -2659,6 +2658,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   	int ret = 0;
>>   	bool seeding_dev = false;
>>   	bool locked = false;
>> +	dev_t devt;
>>   
>>   	if (sb_rdonly(sb) && !fs_devices->seeding)
>>   		return -EROFS;
>> @@ -2853,7 +2853,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   	 * We can ignore the return value as it typically returns -EINVAL and
>>   	 * only succeeds if the device was an alien.
>>   	 */
>> -	btrfs_forget_devices(device_path);
>> +	if (!lookup_bdev(device_path, &devt))
> 
> Same here.
> 
Ok, I fixed it in v4. However, patch 4/4 removes it.

Thanks, Anand