Re: [PATCH v5.1 24/27] Btrfs: free the stale device

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

Hi David, Thanks for comments, more below..

On 05/27/2015 07:34 PM, David Sterba wrote:
> On Fri, May 22, 2015 at 11:33:48PM +0800, Anand Jain wrote:
>> +void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>> +{
>> +	int del = 0;
>> +	struct btrfs_fs_devices *fs_devs;
>> +	struct btrfs_device *dev;
>> +
>> +	if (!rcu_str_deref(cur_dev->name))
>> +		return;
>
> This looks like rcu-unprotected access, though there's an outer mutext
> held in device_list_add that calls btrfs_free_stale_device. I'm not sure
> that the device name should be used to do any sorts of checks at all.

  you are right.  I got this corrected in v5.2 just sent out.


>> +	list_for_each_entry(fs_devs, &fs_uuids, list) {
>> +		if (fs_devs->opened)
>> +			continue;
>> +		if (fs_devs->seeding)
>> +			continue;
>> +		list_for_each_entry(dev, &fs_devs->devices, dev_list) {
>> +			if (dev == cur_dev)
>> +				continue;
>> +
>> +			/*
>> +			 * Todo: This won't be enough. What if same device
>> +			 * comes back with new uuid and with its mapper path?
>> +			 * But for now, this does helps as mostly an admin will
>> +			 * use either mapper or non mapper path throughout.
>> +			 */
>> +			if (!rcu_str_deref(dev->name))
>> +				continue;
>> +			if (!strcmp(rcu_str_deref(dev->name),
>> +						rcu_str_deref(cur_dev->name))) {
>> +				del = 1;
>> +				break;
>> +			}
>> +		}
>> +		if (del) {
>> +			/* delete the stale */
>> +			if (fs_devs->num_devices == 1) {
>> +				btrfs_sysfs_remove_fsid(fs_devs);
>> +				list_del(&fs_devs->list);
>> +				free_fs_devices(fs_devs);
>> +			} else {
>> +				fs_devs->num_devices--;
>> +				list_del(&dev->dev_list);
>> +				rcu_string_free(dev->name);
>> +				kfree(dev);
>
> Devices are normally freed by the rcu through free_device, this looks
> suspicious to mix both approaches.

  yes its bit of mixed up, also in other parts as well, for eg:
  free_fs_devices(). Here I am following free_fs_devices,
  since free_stale will check on the devices that are unmounted.

>> +			}
>> +			break;
>> +		}
>> +	}
>> +	return;
>
> Unnecessary return

right.

>> +}
>> +
>>   /*
>>    * Add new device to list of registered devices
>>    *
>> @@ -560,6 +609,8 @@ static noinline int device_list_add(const char *path,
>>   	if (!fs_devices->opened)
>>   		device->generation = found_transid;
>>
>> +	btrfs_free_stale_device(device);
>
> It might be safe to do that in the end, but it should be explained
> somewhere.

Added, Thanks, Anand

>> +
>>   	*fs_devices_ret = fs_devices;
>>
>>   	return ret;
> --
> 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
>