Re: [PATCH] btrfs: free device if we fail to open it

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

<snip>


>>   I got it. It shouldn't be difficult to reproduce and, I could reproduce.
>> Without this patch.
>>
>>
>>   Below is a device with two different paths. dm and its mapper.
>>
>> ----------
>>   $ ls -bli /dev/mapper/vg-scratch1  /dev/dm-1
>>   561 brw-rw---- 1 root disk 252, 1 Dec  3 12:13 /dev/dm-1
>>   565 lrwxrwxrwx 1 root root      7 Dec  3 12:13 /dev/mapper/vg-scratch1 ->
>> ../dm-1
>> ----------
>>
>>   Clean the fs_devices.
>>
>> ----------
>>   $ btrfs dev scan --forget
>> ----------
>>
>>   Use the mapper to do mkfs.btrfs.
>>
>> ----------
>>   $ mkfs.btrfs -fq /dev/mapper/vg-scratch0
>>   $ mount /dev/mapper/vg-scratch0 /btrfs
>> ----------
>>
>>   Crete raid1 again using mapper path.
>>
>> ----------
>>   $ mkfs.btrfs -U $uuid -fq -draid1 -mraid1 /dev/mapper/vg-scratch1
>> /dev/mapper/vg-scratch2
>> ----------
>>
>>   Use dm path to add the device which belongs to another btrfs filesystem.
>>
>> ----------
>>   $ btrfs dev add -f /dev/dm-1 /btrfs
>> ----------
>>
>>   Now mount the above raid1 in degraded mode.
>>
>> ----------
>>   $ mount -o degraded /dev/mapper/vg-scratch2 /btrfs1
>> ----------
>>
> 
> Ahhh nice, I couldn't figure out a way to trigger it manually.  I wonder if we
> can figure out a way to do this in xfstests without needing to have your
> SCRATCH_DEV on lvm already?

Yep. A dm linear on top of a raw device will help. I have a rough draft 
working. I could send it to xfstests if you want?

>>> Yeah I was a little fuzzy on this.  I think *any* failure should mean that we
>>> remove the device from the fs_devices tho right?  So that we show we're missing
>>> a device, since we can't actually access it?  I'm actually asking, because I
>>> think we can go either way, but to me I think any failure sure result in the
>>> removal of the device so we can re-scan the correct one.  Thanks,
>>>
>>
>> It is difficult to generalize, I guess. For example, consider the transient
>> errors during the boot-up and the errors due to slow to-ready devices or the
>> system-related errors such as ENOMEM/EACCES, all these does not call for
>> device-free. If we free the device for transient errors, any further attempt
>> to mount will fail unless it is device-scan again.
>>
>> Here the bug is about btrfs_free_stale_devices() which failed to identify
>> the same device when tricked by mixing the dm and mapper paths.
>> Can I check with you if there is another way to fix this by checking the
>> device major and min number or the serial number from the device inquiry
>> page?
> 

> I suppose I could just change it so that our verification proceses, like the
> MAGIC or FSID checks, return ENODATA and we only do it in those cases.  Does
> that seem reasonable?

The 'btrfs device add' calls btrfs_free_stale_devices(), however 
device_path_matched() fails to match the device by its path. So IMO, fix 
has to be in device_path_matched() but with a different parameter to 
match instead of device path.

Here is another manifestation of the same problem.

$ mkfs.btrfs -fq /dev/dm-0
$ cat /proc/fs/btrfs/devlist | grep device:
  device: /dev/dm-0

$ btrfs dev scan --forget /dev/dm-0
ERROR: cannot unregister device '/dev/mapper/tsdb': No such file or 
directory

Above, mkfs.btrfs does not sanitizes the input device path however, the 
forget command sanitizes the input device to /dev/mapper/tsdb and the 
device_path_matched() in btrfs_free_stale_devices() fails. So fix has to 
be in device_path_matched() and, why not replace strcmp() with compare 
major and minor device numbers?

Thanks, Anand