Re: [PATCH v3 0/5] btrfs: fix issues due to alien device

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

On 10/8/19 1:36 AM, David Sterba wrote:
> On Mon, Oct 07, 2019 at 05:45:10PM +0800, Anand Jain wrote:
>> v3: Fix alien device is due to wipefs in Patch4.
>>      Fix a nit in Patch3.
>>      Patches are reordered.
>>
>> Alien device is a device in fs_devices list having a different fsid than
>> the expected fsid or no btrfs_magic. This patch set fixes issues found due
>> to the same.
> 
> The definition of alien device should be in some of the patches, I see
> it only in the cover letter.

  Ok will include.

> So the sequence of actions
> 
> 	mkfs A
> 	mount A
> 	mkfs B C
> 	add B to A
> 	mount C

  Right (fixed in patch 3/5). B contains alien btrfs superblock.

  Another scenario (fixed in Patch 4/5) as below.

  	mkfs A B
  	wipe A OR mkfs.ext4 A
  	mount B

  'A' contains alien or no superblock.

> leaves the scanned devices in a state that does not match the reality.
> At the time when B is scanned again, the ownership in the in-memory
> structures should be transferred to A (ie. removing B from BC). So far I
> understand the problem.

> The fix I'd expect is to fix up the devices at the first occasion, like
> when the device is scanned or attempted for mount.

  Right. btrfs_free_stale_devices() does it. Checks for duplicate entries
  for a device, and deletes it as its new structure has already been
  created. But we didn't call this in the context of device add, now
  fixed in patch 5/5.

>> Patch1: is a cleanup patch, not related.
>> Patch2: fixes failing to mount a degraded RAIDs (RAID1/5/6/10), by
>> 	hardening the function btrfs_free_extra_devids().
>> Patch3: fixes the missing device (due to alien btrfs-device) not missing in
>> 	the userland, by hardening the function btrfs_open_one_device().
>> Patch4: fixes the missing device (due to alien device) not missing in
>> 	the userland, by returning EUCLEAN in btrfs_read_dev_one_super().
>> Patch5: eliminates the source of the alien device in the fs_devices.
> 
> I'm a bit lost in the way it's being fixed.

  We weren't checking if there is any stale device structure when a
  device comes into btrfs through btrfs device add. This patch fixes
  it. Similar to what we do during scan. These are the only two ways
  device can entry into the btrfs kernel.

> The userspace part is IMO
> irrelevant, the change must happen on the kernel side using the
> information provided (scan, mount).
> 
> The error conditions should be propagated in a more fine grained way,
> similar to what you propose with EUCLEAN, but not with EUCLEAN. That has
> a very specific meaning, as has been pointed out.
> 
> The distinctions should be like:
> 
>   < 0 - error
>     0 - all ok, take the device
>   > 0 - device ok, but not ours

  When device is scanned its SB is already been read and updated in
  fs_devices.
  Now when we try to mount the SB is found to be corrupted or wiped or
  alienated. IMO it should be < 0. Its not about device its about the
  SB (on-disk struct) in the device, so I wonder which one is better
  -EUCLEAN ? Or -ESTALE ? or any suggestion?

> And the callers will decide what to do (remove or ignore).
> 
>> PS: Fundamentally its wrong approach that btrfs-progs deduces the device
>> missing state in the userland instead of obtaining it from the kernel.
>> I remember objecting on the btrfs-progs patch which did that, but still
>> it got merged, bugs in p3 and p4 are its side effects. I wrote
>> patches to read device_state from the kernel using ioctl, procfs and
>> sysfs but it didn't get the due attention till a merger.
> 
> The state has to be ultimately decided by kernel, userspace can read the
> information from anything but at the time this gets processed,

  Right.

> it might
> be stale again.

  That's ok. The user will run the command again.

> It's been probably very long ago when the above was
> discussed and I don't recall the details, 

> it may be a good idea to
> revisit if there are still things to address.

  Ok thanks. The correct thread to respond on this is the readmirror
  thread. As we need to read the btrfs_device::dev_state from
  the kernel to support readmirror.

Thanks, Anand