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. -- 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