[v3,0/5] btrfs: fix issues due to alien device
mbox series

Message ID 20191007094515.925-1-anand.jain@oracle.com
Headers show
Series
  • btrfs: fix issues due to alien device
Related show

Message

Anand Jain Oct. 7, 2019, 9:45 a.m. UTC
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.

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.

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.

Anand Jain (5):
  btrfs: drop useless goto in open_fs_devices
  btrfs: include non-missing as a qualifier for the latest_bdev
  btrfs: remove identified alien btrfs device in open_fs_devices
  btrfs: remove identified alien device in open_fs_devices
  btrfs: free alien device due to device add

 fs/btrfs/disk-io.c |  2 +-
 fs/btrfs/volumes.c | 57 +++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 17 deletions(-)

Comments

David Sterba Oct. 7, 2019, 5:36 p.m. UTC | #1
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.

So the sequence of actions

	mkfs A
	mount A
	mkfs B C
	add B to A
	mount C

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.

> 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. 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

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, it might
be stale 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.
Anand Jain Oct. 8, 2019, 6:11 a.m. UTC | #2
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