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

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

[Anand Jain]

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(-)

-- 
1.8.3.1

[PATCH 1/5] btrfs: drop useless goto in open_fs_devices

[Anand Jain]

There is no need of goto out in open_fs_devices() as there is nothing
special done at %out:. So refactor it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bba74e3bd9d8..c223a8147bfd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1123,7 +1123,6 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 {
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
-	int ret = 0;
 
 	flags |= FMODE_EXCL;
 
@@ -1136,15 +1135,14 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 		    device->generation > latest_dev->generation)
 			latest_dev = device;
 	}
-	if (fs_devices->open_devices == 0) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (fs_devices->open_devices == 0)
+		return -EINVAL;
+
 	fs_devices->opened = 1;
 	fs_devices->latest_bdev = latest_dev->bdev;
 	fs_devices->total_rw_bytes = 0;
-out:
-	return ret;
+
+	return 0;
 }
 
 static int devid_cmp(void *priv, struct list_head *a, struct list_head *b)
-- 
1.8.3.1

[PATCH 2/5] btrfs: include non-missing as a qualifier for the latest_bdev

[Anand Jain]

btrfs_free_extra_devids() reorgs fs_devices::latest_bdev
to point to the bdev with greatest device::generation number.
For a typical-missing device the generation number is zero so
fs_devices::latest_bdev will never point to it.

But if the missing device is due to alienating [1], then
device::generation is not-zero and if it is >= to rest of
device::generation in the list, then fs_devices::latest_bdev
ends up pointing to the missing device and reports the error
like this [2]

[1]
mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
mkfs.btrfs -fq -draid1 -mraid1 /dev/sdb /dev/sdc
sleep 3 # avoid racing with udev's useless scans if needed
btrfs dev add -f /dev/sdb /btrfs

mount -o degraded /dev/sdc /btrfs1

[2]
mount: wrong fs type, bad option, bad superblock on /dev/sdc,
       missing codepage or helper program, or other error

       In some cases useful info is found in syslog - try
       dmesg | tail or so.

kernel: BTRFS warning (device sdc): devid 1 uuid 072a0192-675b-4d5a-8640-a5cf2b2c704d is missing
kernel: BTRFS error (device sdc): failed to read devices
kernel: BTRFS error (device sdc): open_ctree failed

Fix the root of the issue, by checking if the the device is not
missing before it can be a contender for the fs_devices::latest_bdev
title.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c223a8147bfd..0cc8d40b8bb9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -974,6 +974,8 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 							&device->dev_state)) {
 			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
 			     &device->dev_state) &&
+			    !test_bit(BTRFS_DEV_STATE_MISSING,
+			     &device->dev_state) &&
 			     (!latest_dev ||
 			      device->generation > latest_dev->generation)) {
 				latest_dev = device;
-- 
1.8.3.1

[PATCH v3 3/5] btrfs: remove identified alien btrfs device in open_fs_devices

[Anand Jain]

In open_fs_devices() we identify alien device but we don't reset its
the device::name. So progs device list does not show the device missing
as shown in the script below.

mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdb
sleep 3 # avoid racing with udev's useless scans if needed
btrfs dev add -f /dev/sdb /btrfs
mount -o degraded /dev/sdc /btrfs1

No missing device:
btrfs fi show -m /btrfs1
Label: none  uuid: 3eb7cd50-4594-458f-9d68-c243cc49954d
	Total devices 2 FS bytes used 128.00KiB
	devid    1 size 12.00GiB used 1.26GiB path /dev/sdc
	devid    2 size 12.00GiB used 1.26GiB path /dev/sdb

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: Fix nit, rearrange code to remove continue.
v2: Move free alien part to its parent function btrfs_open_one_device.
    Thanks Nikolay.

 fs/btrfs/volumes.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0cc8d40b8bb9..81097c80ac4a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -591,13 +591,18 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	if (ret)
 		return ret;
 
+	ret = -EINVAL;
 	disk_super = (struct btrfs_super_block *)bh->b_data;
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
-	if (devid != device->devid)
+	if (devid != device->devid) {
+		ret = -EUCLEAN;
 		goto error_brelse;
+	}
 
-	if (memcmp(device->uuid, disk_super->dev_item.uuid, BTRFS_UUID_SIZE))
+	if (memcmp(device->uuid, disk_super->dev_item.uuid, BTRFS_UUID_SIZE)) {
+		ret = -EUCLEAN;
 		goto error_brelse;
+	}
 
 	device->generation = btrfs_super_generation(disk_super);
 
@@ -640,7 +645,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	brelse(bh);
 	blkdev_put(bdev, flags);
 
-	return -EINVAL;
+	return ret;
 }
 
 /*
@@ -1123,18 +1128,25 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 				fmode_t flags, void *holder)
 {
+	int ret;
 	struct btrfs_device *device;
+	struct btrfs_device *tmp_device;
 	struct btrfs_device *latest_dev = NULL;
 
 	flags |= FMODE_EXCL;
 
-	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
+				 dev_list) {
 		/* Just open everything we can; ignore failures here */
-		if (btrfs_open_one_device(fs_devices, device, flags, holder))
-			continue;
-
-		if (!latest_dev ||
-		    device->generation > latest_dev->generation)
+		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
+		if (ret == -EUCLEAN) {
+			/* Its an alien device, remove it from the list */
+			fs_devices->num_devices--;
+			list_del(&device->dev_list);
+			btrfs_free_device(device);
+		}
+		if (ret == 0 && (!latest_dev ||
+		    device->generation > latest_dev->generation))
 			latest_dev = device;
 	}
 	if (fs_devices->open_devices == 0)
-- 
1.8.3.1

[PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Anand Jain]

Following test case explains it all, even though the degraded mount is
successful the btrfs-progs fails to report the missing device.

 mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
 wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
 btrfs fi show -m /btrfs

 Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
	Total devices 2 FS bytes used 128.00KiB
	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd

This is because btrfs-progs does it fundamentally wrong way that
it deduces the missing device status in the user land instead of
refuting from the kernel.

At the same time in the kernel when we know that there is device
with non-btrfs magic, then remove that device from the list so
that btrfs-progs or someother userland utility won't be confused.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 326d5281ad93..e05856432456 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
 	if (btrfs_super_bytenr(super) != bytenr ||
 		    btrfs_super_magic(super) != BTRFS_MAGIC) {
 		brelse(bh);
-		return -EINVAL;
+		return -EUCLEAN;
 	}
 
 	*bh_ret = bh;
-- 
1.8.3.1

[PATCH 5/5] btrfs: free alien device due to device add

[Anand Jain]

When the old device has new fsid through btrfs device add -f <dev> our
fs_devices list has an alien device in one of the fs_devices.

By having an alien device in fs_devices, we have two issues so far

1. missing device is not shows as missing in the userland

Which is due to cracks in the function btrfs_open_one_device() and
hardened by patch
 btrfs: delete identified alien device in open_fs_devices

2. mount of a degraded fs_devices fails

Which is due to cracks in the function btrfs_free_extra_devids() and
hardened by patch
 btrfs: include non-missing as a qualifier for the latest_bdev

Now the reason for both of this issue is that there is an alien (does not
contain the intended fsid) device in the fs_devices.

We know a device can be scanned/added through
btrfs-control::BTRFS_IOC_SCAN_DEV|BTRFS_IOC_DEVICES_READY
or by
ioctl::BTRFS_IOC_ADD_DEV

And device coming through btrfs-control is checked against the all other
devices in btrfs kernel but not coming through BTRFS_IOC_ADD_DEV.

This patch checks if the device add is alienating any other scanned
device and deletes it.

In fact, this patch fixes both the issues 1 and 2 (above) by eliminating
the source of the issue, but still they have their own patch as well
because its the right way to harden the functions and fill the cracks.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 81097c80ac4a..4c7e37cd2166 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2534,6 +2534,19 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	/* Update ctime/mtime for libblkid */
 	update_dev_time(device_path);
+
+	/*
+	 * Now that we have written a new sb into this device, check all other
+	 * fs_devices list if it alienates any scanned device.
+	 */
+	mutex_lock(&uuid_mutex);
+	/*
+	 * Ignore the return as we are successfull in the core task - to added
+	 * the device
+	 */
+	btrfs_free_stale_devices(device_path, NULL);
+	mutex_unlock(&uuid_mutex);
+
 	return ret;
 
 error_sysfs:
-- 
1.8.3.1

Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Nikolay Borisov]

On 7.10.19 г. 12:45 ч., Anand Jain wrote:
> Following test case explains it all, even though the degraded mount is
> successful the btrfs-progs fails to report the missing device.
> 
>  mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
>  wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
>  btrfs fi show -m /btrfs
> 
>  Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
> 	Total devices 2 FS bytes used 128.00KiB
> 	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
> 	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
> 
> This is because btrfs-progs does it fundamentally wrong way that
> it deduces the missing device status in the user land instead of
> refuting from the kernel.
> 
> At the same time in the kernel when we know that there is device
> with non-btrfs magic, then remove that device from the list so
> that btrfs-progs or someother userland utility won't be confused.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/disk-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 326d5281ad93..e05856432456 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>  	if (btrfs_super_bytenr(super) != bytenr ||
>  		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>  		brelse(bh);
> -		return -EINVAL;
> +		return -EUCLEAN;

This is really non-obvious and you are propagating the special-meaning
of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
patch does is make the following call chain return EUCLAN:

btrfs_open_one_device <-- finally removing the device in this function
 btrfs_get_bdev_and_sb <-- propagating it to here
  btrfs_read_dev_super
    btrfs_read_dev_one_super <-- you return the EUCLEAN


And your commit log doesn't mention anything about that. EUCLEAN
warrants a comment in this case since it changes behavior in
higher-level layers.

>  	}
>  
>  	*bh_ret = bh;
> 

Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Qu Wenruo]

On 2019/10/7 下午9:30, Nikolay Borisov wrote:
>
>
> On 7.10.19 г. 12:45 ч., Anand Jain wrote:
>> Following test case explains it all, even though the degraded mount is
>> successful the btrfs-progs fails to report the missing device.
>>
>>  mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
>>  wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
>>  btrfs fi show -m /btrfs
>>
>>  Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
>> 	Total devices 2 FS bytes used 128.00KiB
>> 	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
>> 	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
>>
>> This is because btrfs-progs does it fundamentally wrong way that
>> it deduces the missing device status in the user land instead of
>> refuting from the kernel.
>>
>> At the same time in the kernel when we know that there is device
>> with non-btrfs magic, then remove that device from the list so
>> that btrfs-progs or someother userland utility won't be confused.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/disk-io.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 326d5281ad93..e05856432456 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>>  	if (btrfs_super_bytenr(super) != bytenr ||
>>  		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>>  		brelse(bh);
>> -		return -EINVAL;
>> +		return -EUCLEAN;
>
> This is really non-obvious and you are propagating the special-meaning
> of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
> patch does is make the following call chain return EUCLAN:
>
> btrfs_open_one_device <-- finally removing the device in this function
>  btrfs_get_bdev_and_sb <-- propagating it to here
>   btrfs_read_dev_super
>     btrfs_read_dev_one_super <-- you return the EUCLEAN
>
>
> And your commit log doesn't mention anything about that. EUCLEAN
> warrants a comment in this case since it changes behavior in
> higher-level layers.


And, for most case, EUCLEAN should have a proper kernel message for
what's going wrong, the value we hit and the value we expect.

And for EUCLEAN, it's more like graceful error out for impossible thing.
This is definitely not the case, and I believe the original EINVAL makes
more sense than EUCLEAN.

Thanks,
Qu

>
>>  	}
>>
>>  	*bh_ret = bh;
>>

Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[David Sterba]

On Mon, Oct 07, 2019 at 09:37:49PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/10/7 下午9:30, Nikolay Borisov wrote:
> >
> >
> > On 7.10.19 г. 12:45 ч., Anand Jain wrote:
> >> Following test case explains it all, even though the degraded mount is
> >> successful the btrfs-progs fails to report the missing device.
> >>
> >>  mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
> >>  wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
> >>  btrfs fi show -m /btrfs
> >>
> >>  Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
> >> 	Total devices 2 FS bytes used 128.00KiB
> >> 	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
> >> 	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
> >>
> >> This is because btrfs-progs does it fundamentally wrong way that
> >> it deduces the missing device status in the user land instead of
> >> refuting from the kernel.
> >>
> >> At the same time in the kernel when we know that there is device
> >> with non-btrfs magic, then remove that device from the list so
> >> that btrfs-progs or someother userland utility won't be confused.
> >>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >>  fs/btrfs/disk-io.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index 326d5281ad93..e05856432456 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
> >>  	if (btrfs_super_bytenr(super) != bytenr ||
> >>  		    btrfs_super_magic(super) != BTRFS_MAGIC) {
> >>  		brelse(bh);
> >> -		return -EINVAL;
> >> +		return -EUCLEAN;
> >
> > This is really non-obvious and you are propagating the special-meaning
> > of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
> > patch does is make the following call chain return EUCLAN:
> >
> > btrfs_open_one_device <-- finally removing the device in this function
> >  btrfs_get_bdev_and_sb <-- propagating it to here
> >   btrfs_read_dev_super
> >     btrfs_read_dev_one_super <-- you return the EUCLEAN
> >
> >
> > And your commit log doesn't mention anything about that. EUCLEAN
> > warrants a comment in this case since it changes behavior in
> > higher-level layers.
> 
> 
> And, for most case, EUCLEAN should have a proper kernel message for
> what's going wrong, the value we hit and the value we expect.
> 
> And for EUCLEAN, it's more like graceful error out for impossible thing.
> This is definitely not the case, and I believe the original EINVAL makes
> more sense than EUCLEAN.

I agree, EUCLEAN is wrong here.

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

[David Sterba]

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.

Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Anand Jain]

On 10/8/19 1:03 AM, David Sterba wrote:
> On Mon, Oct 07, 2019 at 09:37:49PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/10/7 下午9:30, Nikolay Borisov wrote:
>>>
>>>
>>> On 7.10.19 г. 12:45 ч., Anand Jain wrote:
>>>> Following test case explains it all, even though the degraded mount is
>>>> successful the btrfs-progs fails to report the missing device.
>>>>
>>>>   mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
>>>>   wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
>>>>   btrfs fi show -m /btrfs
>>>>
>>>>   Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
>>>> 	Total devices 2 FS bytes used 128.00KiB
>>>> 	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
>>>> 	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
>>>>
>>>> This is because btrfs-progs does it fundamentally wrong way that
>>>> it deduces the missing device status in the user land instead of
>>>> refuting from the kernel.
>>>>
>>>> At the same time in the kernel when we know that there is device
>>>> with non-btrfs magic, then remove that device from the list so
>>>> that btrfs-progs or someother userland utility won't be confused.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>   fs/btrfs/disk-io.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index 326d5281ad93..e05856432456 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>>>>   	if (btrfs_super_bytenr(super) != bytenr ||
>>>>   		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>>>>   		brelse(bh);
>>>> -		return -EINVAL;
>>>> +		return -EUCLEAN;
>>>
>>> This is really non-obvious and you are propagating the special-meaning
>>> of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
>>> patch does is make the following call chain return EUCLAN:
>>>
>>> btrfs_open_one_device <-- finally removing the device in this function
>>>   btrfs_get_bdev_and_sb <-- propagating it to here
>>>    btrfs_read_dev_super
>>>      btrfs_read_dev_one_super <-- you return the EUCLEAN
>>>
>>>
>>> And your commit log doesn't mention anything about that. EUCLEAN
>>> warrants a comment in this case since it changes behavior in
>>> higher-level layers.

  Ok. Which means the commit log needs to be fixed.

>>
>> And, for most case, EUCLEAN should have a proper kernel message for
>> what's going wrong, the value we hit and the value we expect.

  If its a kernel message for EUCLEAN in the context of mounted state,
  I would say yes, as it indicates a corruption.
  But here we are still in the unmounted context and moving towards
  mounted context. Having an alien device in the fs_devices is not
  something logical bug nor a corruption, it just about a disk whose
  disk superblock got overwritten by the user operation. And its not
  a good idea to log the kernel messages arising from the user
  operation especially in the unmounted context. Otherwise we shall
  endup cluttering the kernel messages. Moreover if there is an alien
  device, the user must use -o degraded and we do have the missing
  kernel messages, and this will suffice to explain the state about the
  fsid being mounted. And the alien fsid, its a stale we don't worry
  about it. So no kernel messages.

>> And for EUCLEAN, it's more like graceful error out for impossible thing.
>> This is definitely not the case, and I believe the original EINVAL makes
>> more sense than EUCLEAN.
> 
> I agree, EUCLEAN is wrong here.
>

   I am ok with any other suitable. It does not matter. And the other
   choice I have is ESTALE.

   But EINVAL is no go because we still want to keep the messed up device
   unless there is a confirmation that its alien.

   In the same function we use EINVAL if the device/partition size
   changed to smaller size after we have scanned the device. Which
   means we still keep the device in the list. Its the user choice,
   no need to meddle with it.

    bytenr = btrfs_sb_offset(copy_num);
    if (bytenr + BTRFS_SUPER_INFO_SIZE >= i_size_read(bdev->bd_inode))
           return -EINVAL;

   But, EUCLEAN /* structure needs cleaning */ is errno which exactly
   says whats needed here. Because an alien device is a kind of
   corruption but it can easily happen due to unplanned device operations
   in the userland. But as it happened before we assembled the volume so
   its safe to clean and is not a road block when there is redundancy
   with degraded option.

Thanks, Anand

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

[Anand Jain]

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

Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Anand Jain]

On 8/10/19 11:26 AM, Anand Jain wrote:
> On 10/8/19 1:03 AM, David Sterba wrote:
>> On Mon, Oct 07, 2019 at 09:37:49PM +0800, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/10/7 下午9:30, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 7.10.19 г. 12:45 ч., Anand Jain wrote:
>>>>> Following test case explains it all, even though the degraded mount is
>>>>> successful the btrfs-progs fails to report the missing device.
>>>>>
>>>>>   mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
>>>>>   wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
>>>>>   btrfs fi show -m /btrfs
>>>>>
>>>>>   Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
>>>>>     Total devices 2 FS bytes used 128.00KiB
>>>>>     devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
>>>>>     devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
>>>>>
>>>>> This is because btrfs-progs does it fundamentally wrong way that
>>>>> it deduces the missing device status in the user land instead of
>>>>> refuting from the kernel.
>>>>>
>>>>> At the same time in the kernel when we know that there is device
>>>>> with non-btrfs magic, then remove that device from the list so
>>>>> that btrfs-progs or someother userland utility won't be confused.
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>> ---
>>>>>   fs/btrfs/disk-io.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index 326d5281ad93..e05856432456 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct 
>>>>> block_device *bdev, int copy_num,
>>>>>       if (btrfs_super_bytenr(super) != bytenr ||
>>>>>               btrfs_super_magic(super) != BTRFS_MAGIC) {
>>>>>           brelse(bh);
>>>>> -        return -EINVAL;
>>>>> +        return -EUCLEAN;
>>>>
>>>> This is really non-obvious and you are propagating the special-meaning
>>>> of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
>>>> patch does is make the following call chain return EUCLAN:
>>>>
>>>> btrfs_open_one_device <-- finally removing the device in this function
>>>>   btrfs_get_bdev_and_sb <-- propagating it to here
>>>>    btrfs_read_dev_super
>>>>      btrfs_read_dev_one_super <-- you return the EUCLEAN
>>>>
>>>>
>>>> And your commit log doesn't mention anything about that. EUCLEAN
>>>> warrants a comment in this case since it changes behavior in
>>>> higher-level layers.
> 
>   Ok. Which means the commit log needs to be fixed.
> 
>>>
>>> And, for most case, EUCLEAN should have a proper kernel message for
>>> what's going wrong, the value we hit and the value we expect.
> 
>   If its a kernel message for EUCLEAN in the context of mounted state,
>   I would say yes, as it indicates a corruption.
>   But here we are still in the unmounted context and moving towards
>   mounted context. Having an alien device in the fs_devices is not
>   something logical bug nor a corruption, it just about a disk whose
>   disk superblock got overwritten by the user operation. And its not
>   a good idea to log the kernel messages arising from the user
>   operation especially in the unmounted context. Otherwise we shall
>   endup cluttering the kernel messages. Moreover if there is an alien
>   device, the user must use -o degraded and we do have the missing
>   kernel messages, and this will suffice to explain the state about the
>   fsid being mounted. And the alien fsid, its a stale we don't worry
>   about it. So no kernel messages.
> 
>>> And for EUCLEAN, it's more like graceful error out for impossible thing.
>>> This is definitely not the case, and I believe the original EINVAL makes
>>> more sense than EUCLEAN.
>>
>> I agree, EUCLEAN is wrong here.
>>
> 
>    I am ok with any other suitable. It does not matter. And the other
>    choice I have is ESTALE.
> 
>    But EINVAL is no go because we still want to keep the messed up device
>    unless there is a confirmation that its alien.
> 
>    In the same function we use EINVAL if the device/partition size
>    changed to smaller size after we have scanned the device. Which
>    means we still keep the device in the list. Its the user choice,
>    no need to meddle with it.
> 
>     bytenr = btrfs_sb_offset(copy_num);
>     if (bytenr + BTRFS_SUPER_INFO_SIZE >= i_size_read(bdev->bd_inode))
>            return -EINVAL;


    I didn't know that btrfs/197 btrfs/198 is failing.
    I found that we need this patch series so that it
    passes these tests. Logs for failing test cases are here [1].

    Any feedback if errno -ESTALE is better instead of -EUCLEAN? As
    mentioned I can't use -EINVAL because its already been used.

[1]
btrfs/197	- output mismatch (see /xfstests-dev/results//btrfs/197.out.bad)
     --- tests/btrfs/197.out	2020-01-08 18:08:44.040258593 +0800
     +++ /xfstests-dev/results//btrfs/197.out.bad	2020-01-15 
16:32:23.870187397 +0800
     @@ -3,23 +3,19 @@
      Label: none  uuid: <UUID>
      	Total devices <NUM> FS bytes used <SIZE>
      	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
     -	*** Some devices missing

      raid5
      Label: none  uuid: <UUID>
     ...
     (Run 'diff -u /xfstests-dev/tests/btrfs/197.out 
/xfstests-dev/results//btrfs/197.out.bad'  to see the entire diff)
btrfs/198	- output mismatch (see /xfstests-dev/results//btrfs/198.out.bad)
     --- tests/btrfs/198.out	2020-01-08 18:08:44.040258593 +0800
     +++ /xfstests-dev/results//btrfs/198.out.bad	2020-01-15 
16:32:28.882282030 +0800
     @@ -3,23 +3,19 @@
      Label: none  uuid: <UUID>
      	Total devices <NUM> FS bytes used <SIZE>
      	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
     -	*** Some devices missing

      raid5
      Label: none  uuid: <UUID>
     ...
     (Run 'diff -u /xfstests-dev/tests/btrfs/198.out 
/xfstests-dev/results//btrfs/198.out.bad'  to see the entire diff)

Thanks, Anand



>    But, EUCLEAN /* structure needs cleaning */ is errno which exactly
>    says whats needed here. Because an alien device is a kind of
>    corruption but it can easily happen due to unplanned device operations
>    in the userland. But as it happened before we assembled the volume so
>    its safe to clean and is not a road block when there is redundancy
>    with degraded option.
> 
> Thanks, Anand
> 

Re: [PATCH 2/5] btrfs: include non-missing as a qualifier for the latest_bdev

[Josef Bacik]

On 10/7/19 5:45 AM, Anand Jain wrote:
> btrfs_free_extra_devids() reorgs fs_devices::latest_bdev
> to point to the bdev with greatest device::generation number.
> For a typical-missing device the generation number is zero so
> fs_devices::latest_bdev will never point to it.
> 
> But if the missing device is due to alienating [1], then
> device::generation is not-zero and if it is >= to rest of
> device::generation in the list, then fs_devices::latest_bdev
> ends up pointing to the missing device and reports the error
> like this [2]
> 
> [1]
> mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sdb /dev/sdc
> sleep 3 # avoid racing with udev's useless scans if needed
> btrfs dev add -f /dev/sdb /btrfs
> 
> mount -o degraded /dev/sdc /btrfs1
> 
> [2]
> mount: wrong fs type, bad option, bad superblock on /dev/sdc,
>         missing codepage or helper program, or other error
> 
>         In some cases useful info is found in syslog - try
>         dmesg | tail or so.
> 
> kernel: BTRFS warning (device sdc): devid 1 uuid 072a0192-675b-4d5a-8640-a5cf2b2c704d is missing
> kernel: BTRFS error (device sdc): failed to read devices
> kernel: BTRFS error (device sdc): open_ctree failed
> 
> Fix the root of the issue, by checking if the the device is not
> missing before it can be a contender for the fs_devices::latest_bdev
> title.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

This should be turned into an xfstests.  You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 1/5] btrfs: drop useless goto in open_fs_devices

[Josef Bacik]

On 10/7/19 5:45 AM, Anand Jain wrote:
> There is no need of goto out in open_fs_devices() as there is nothing
> special done at %out:. So refactor it.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v3 3/5] btrfs: remove identified alien btrfs device in open_fs_devices

[Josef Bacik]

On 10/7/19 5:45 AM, Anand Jain wrote:
> In open_fs_devices() we identify alien device but we don't reset its
> the device::name. So progs device list does not show the device missing
> as shown in the script below.
> 
> mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdb
> sleep 3 # avoid racing with udev's useless scans if needed
> btrfs dev add -f /dev/sdb /btrfs
> mount -o degraded /dev/sdc /btrfs1
> 
> No missing device:
> btrfs fi show -m /btrfs1
> Label: none  uuid: 3eb7cd50-4594-458f-9d68-c243cc49954d
> 	Total devices 2 FS bytes used 128.00KiB
> 	devid    1 size 12.00GiB used 1.26GiB path /dev/sdc
> 	devid    2 size 12.00GiB used 1.26GiB path /dev/sdb
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Why not just remove the device if there's any error?  I'm not sure why these 
particular checks make a difference from any other error?  Thanks,

Josef

Re: [PATCH 5/5] btrfs: free alien device due to device add

[Josef Bacik]

On 10/7/19 5:45 AM, Anand Jain wrote:
> When the old device has new fsid through btrfs device add -f <dev> our
> fs_devices list has an alien device in one of the fs_devices.
> 
> By having an alien device in fs_devices, we have two issues so far
> 
> 1. missing device is not shows as missing in the userland
> 
> Which is due to cracks in the function btrfs_open_one_device() and
> hardened by patch
>   btrfs: delete identified alien device in open_fs_devices
> 
> 2. mount of a degraded fs_devices fails
> 
> Which is due to cracks in the function btrfs_free_extra_devids() and
> hardened by patch
>   btrfs: include non-missing as a qualifier for the latest_bdev
> 
> Now the reason for both of this issue is that there is an alien (does not
> contain the intended fsid) device in the fs_devices.
> 
> We know a device can be scanned/added through
> btrfs-control::BTRFS_IOC_SCAN_DEV|BTRFS_IOC_DEVICES_READY
> or by
> ioctl::BTRFS_IOC_ADD_DEV
> 
> And device coming through btrfs-control is checked against the all other
> devices in btrfs kernel but not coming through BTRFS_IOC_ADD_DEV.
> 
> This patch checks if the device add is alienating any other scanned
> device and deletes it.
> 
> In fact, this patch fixes both the issues 1 and 2 (above) by eliminating
> the source of the issue, but still they have their own patch as well
> because its the right way to harden the functions and fill the cracks.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

I agree, but mostly because btrfs_scan_one_device(), which is what gets called 
by the control thingy, does this in the case that it adds a device.  This patch 
makes add dev consistent with the normal scanning stuff, so I'm ok with it.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 2/5] btrfs: include non-missing as a qualifier for the latest_bdev

[Anand Jain]

[...]

> This should be turned into an xfstests.

xfstests btrfs/197 was added.

43602315cad9 btrfs: test for alien btrfs-devices

Thanks, Anand


>  You can add
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> 
> Thanks,
> 
> Josef

Re: [PATCH v3 3/5] btrfs: remove identified alien btrfs device in open_fs_devices

[Anand Jain]

On 1/16/20 11:56 PM, Josef Bacik wrote:
> On 10/7/19 5:45 AM, Anand Jain wrote:
>> In open_fs_devices() we identify alien device but we don't reset its
>> the device::name. So progs device list does not show the device missing
>> as shown in the script below.
>>
>> mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdb
>> sleep 3 # avoid racing with udev's useless scans if needed
>> btrfs dev add -f /dev/sdb /btrfs
>> mount -o degraded /dev/sdc /btrfs1
>>
>> No missing device:
>> btrfs fi show -m /btrfs1
>> Label: none  uuid: 3eb7cd50-4594-458f-9d68-c243cc49954d
>>     Total devices 2 FS bytes used 128.00KiB
>>     devid    1 size 12.00GiB used 1.26GiB path /dev/sdc
>>     devid    2 size 12.00GiB used 1.26GiB path /dev/sdb
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Why not just remove the device if there's any error?  I'm not sure why 
> these particular checks make a difference from any other error?  Thanks,

  That's interesting, but disadvantage is user has to re-run the
  device scan if we remove the device for a non-alien device which can
  fail temporarily in btrfs_open_one_device() function stack such as


     *bdev = blkdev_get_by_path(device_path, flags, holder);

   If user land has opened the device with O_EXCL this shall
   fail with -EBUSY. So here we shouldn't remove.


      ret = set_blocksize(*bdev, BTRFS_BDEV_BLOCKSIZE);

   This can fail if the bdev does not accept the blocksize and its
   rather a good idea to remove the device as we won't be able to
   use this device any time. So as this is not a temporary issue,
   here we could remove the device.


         *bh = btrfs_read_dev_super(*bdev);

   This function is still an incomplete (because we don't yet handle
   the corrupted super block #1, there is a patch in the ML but
   in dispute, I think). Needs clarity on how a completed function
   will look like. So here it depends on when this function completes.


      bh = __bread(bdev, bytenr / BTRFS_BDEV_BLOCKSIZE, 
BTRFS_SUPER_INFO_SIZE);

   Read can fail momentarily for transport/disconnect/plug-out issue
   and which can reappears and assume if there isn't systemd auto scan
   so here we shouldn't remove.


Thanks, Anand


> Josef

Re: [PATCH v3 3/5] btrfs: remove identified alien btrfs device in open_fs_devices

[Josef Bacik]

On 1/17/20 4:10 AM, Anand Jain wrote:
> 
> 
> On 1/16/20 11:56 PM, Josef Bacik wrote:
>> On 10/7/19 5:45 AM, Anand Jain wrote:
>>> In open_fs_devices() we identify alien device but we don't reset its
>>> the device::name. So progs device list does not show the device missing
>>> as shown in the script below.
>>>
>>> mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
>>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdb
>>> sleep 3 # avoid racing with udev's useless scans if needed
>>> btrfs dev add -f /dev/sdb /btrfs
>>> mount -o degraded /dev/sdc /btrfs1
>>>
>>> No missing device:
>>> btrfs fi show -m /btrfs1
>>> Label: none  uuid: 3eb7cd50-4594-458f-9d68-c243cc49954d
>>>     Total devices 2 FS bytes used 128.00KiB
>>>     devid    1 size 12.00GiB used 1.26GiB path /dev/sdc
>>>     devid    2 size 12.00GiB used 1.26GiB path /dev/sdb
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> Why not just remove the device if there's any error?  I'm not sure why these 
>> particular checks make a difference from any other error?  Thanks,
> 
>   That's interesting, but disadvantage is user has to re-run the
>   device scan if we remove the device for a non-alien device which can
>   fail temporarily in btrfs_open_one_device() function stack such as
> 
> 
>      *bdev = blkdev_get_by_path(device_path, flags, holder);
> 
>    If user land has opened the device with O_EXCL this shall
>    fail with -EBUSY. So here we shouldn't remove.
> 
> 
>       ret = set_blocksize(*bdev, BTRFS_BDEV_BLOCKSIZE);
> 
>    This can fail if the bdev does not accept the blocksize and its
>    rather a good idea to remove the device as we won't be able to
>    use this device any time. So as this is not a temporary issue,
>    here we could remove the device.
> 
> 
>          *bh = btrfs_read_dev_super(*bdev);
> 
>    This function is still an incomplete (because we don't yet handle
>    the corrupted super block #1, there is a patch in the ML but
>    in dispute, I think). Needs clarity on how a completed function
>    will look like. So here it depends on when this function completes.
> 
> 
>       bh = __bread(bdev, bytenr / BTRFS_BDEV_BLOCKSIZE, BTRFS_SUPER_INFO_SIZE);
> 
>    Read can fail momentarily for transport/disconnect/plug-out issue
>    and which can reappears and assume if there isn't systemd auto scan
>    so here we shouldn't remove.
> 

Alright that's fair, thanks,

Josef