On 04/09/2018 07:53 PM, David Sterba wrote:
On Mon, Apr 09, 2018 at 04:39:03PM +0800, Anand Jain wrote:
On 04/04/2018 02:34 AM, David Sterba wrote:
The volume mutex does not protect against anything in this case, the
comment about scrub is right but not related to locking and looks
confusing. The comment in btrfs_find_device_missing_or_by_path is wrong
and confusing too.
The device_list_mutex is not held here to protect device lookup, but in
this case device replace cannot run in parallel with device removal (due
to exclusive op protection), so we don't need further locking here.
Agreed, usage of device_list_mutex and volume_mutex is kind of redundant.
There are unfinished features in btrfs volume, such as device offline
while it was mounted (patches in the ML).
It's better to replace volume_mutex with device_list_mutex instead of
removing it, as we might need it in the context mentioned above.
The device_list_mutex will not do anything useful here. It's taken in
contexts where we need to make sure the device list is locked for writes
or we don't want to let the device disappear (superblock write).
As dev-replace requires the device to exist throughout the whole
operation (src_device), the EXCL_OP bit is the correct protection and we
can't hold device_list_mutex throughout the whole
btrfs_dev_replace_start.
>
Or it's not a good idea to clean up until all the features are in place
otherwise we end up adding the locks again at some point.
The lock is redundant and no new code should depend on it. If you're
going to add a more fine grained locking of devices, then we can revisit
what's the best way to do it and I'm quite sure this will not require
re-adding the volume_mutex as it is used now.
Agreed.
Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
