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

[Josef Bacik <josef@toxicpanda.com>]

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,

Josef