On Thu, Sep 02, 2021 at 10:10:04AM -0400, Josef Bacik wrote:

quoted

This is a bit hand wavy but the critical part of the correctness proof,
and it's not explaining it enough IMO. The important piece happens in
device_list_add, the fs_devices lookup and EBUSY, but all that is now
excluded completely by the uuid_mutex from running in parallel with any
part of rm_device.

This means that the state of the device is seen complete by each (scan,
rm device). Without the uuid mutex the scaning can find the signature,
then try to lookup the device in the list, while in parallel the rm
device changes the signature or manipulates the list. But not everything
is covered by the device list mutex so there are combinations of both
tasks with some in-progress state.  Also count in the RCU protection.

 From high level it is what you say about ordering scan/scratch, but
otherwise I'm not convinced that the change is not subtly breaking
something.

Yeah this is far from ideal, we really need to rework our entire device 
liftetime handling and locking, however this isn't going to break 
anything.  We are worried about rm and scan racing with each other, 
before this change we'll zero the device out under the UUID mutex so 
when scan does run it'll make sure that it can go through the whole 
device scan thing without rm messing with us.

We aren't worried if the scratch happens first, because the result is we 
don't think this is a btrfs device and we bail out.

The only case we are concerned with is we scratch _after_ scan is able 
to read the superblock and gets a seemingly valid super block, so lets 
consider this case.

Scan will call device_list_add() with the device we're removing.  We'll 
call find_fsid_with_metadata_uuid() and get our fs_devices for this 
UUID.  At this point we lock the fs_devices->device_list_mutex.  This is 
what protects us in this case, but we have two cases here.

1. We aren't to the device removal part of the RM.  We found our device, 
and device name matches our path, we go down and we set total_devices to 
our super number of devices, which doesn't affect anything because we 
haven't done the remove yet.

2. We are past the device removal part, which is protected by the 
device_list_mutex.  Scan doesn't find the device, it goes down and does the

if (fs_devices->opened)
	return -EBUSY;

check and we bail out.

Nothing about this situation is ideal, but the lockdep splat is real, 
and the fix is safe, tho admittedly a bit scary looking.  Thanks,

Thanks, reading the code a few more times I tend to agree, I've added
this another explanation to the changelog.