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

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

On 07/12/2021 03:16, Josef Bacik wrote:
> On Mon, Dec 06, 2021 at 10:32:07PM +0800, Anand Jain wrote:
>>
>> <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?
>>
> 
> Yes please, that would be helpful.
> 
>>>>> 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?
> 
> I like that better actually, you mind wiring it up since it's your idea?
> Thanks,
> 

  Sure. For both xfstests and the fix.

Thanks, Anand