[PATCH] btrfs: free device without BTRFS_MAGIC

[PATCH] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

Many things can happen after the device is scanned and before the device
is mounted.

One such thing is losing the BTRFS_MAGIC on the device.

If it happens we still won't free that device from the memory and causes
the userland to confuse.

For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
does not have the BTRFS_MAGIC, the btrfs fi show still shows device
which does not belong. As shown below.

mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb

wipefs -a /dev/sdb
mount -o degraded /dev/sda /btrfs
btrfs fi show -m

/dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
btrfs.
Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
        Total devices 2 FS bytes used 128.00KiB
        devid    1 size 3.00GiB used 571.19MiB path /dev/sda
        devid    2 size 3.00GiB used 571.19MiB path /dev/sdb

Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
when BTRFS_MAGIC check fails, so that its parent open_fs_devices()
shall free the device in the mount-thread.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Now the fstests btrfs/198 pass with this fix.

 fs/btrfs/disk-io.c |  2 +-
 fs/btrfs/volumes.c | 19 +++++++++++++------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 160b485d2cc0..9c91d12530a6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3432,7 +3432,7 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
 	if (btrfs_super_bytenr(super) != bytenr ||
 		    btrfs_super_magic(super) != BTRFS_MAGIC) {
 		btrfs_release_disk_super(super);
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(-ENODATA);
 	}
 
 	return super;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7cc677a7e544..ec9dac40b4f1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1198,17 +1198,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 {
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_device *tmp_device;
 
 	flags |= FMODE_EXCL;
 
-	list_for_each_entry(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;
+	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
+				 dev_list) {
+		int ret;
 
-		if (!latest_dev ||
-		    device->generation > latest_dev->generation)
+		/* Just open everything we can; ignore failures here */
+		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
+		if (ret == 0 && (!latest_dev ||
+		    device->generation > latest_dev->generation)) {
 			latest_dev = device;
+		} else if (ret == -ENODATA) {
+			fs_devices->num_devices--;
+			list_del(&device->dev_list);
+			btrfs_free_device(device);
+		}
 	}
 	if (fs_devices->open_devices == 0)
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH] btrfs: free device without BTRFS_MAGIC

[Johannes Thumshirn]

On 19/09/2020 04:53, Anand Jain wrote:
> Many things can happen after the device is scanned and before the device
> is mounted.
> 
> One such thing is losing the BTRFS_MAGIC on the device.
> 
> If it happens we still won't free that device from the memory and causes
> the userland to confuse.
> 
> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
> which does not belong. As shown below.
> 
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
> 
> wipefs -a /dev/sdb
> mount -o degraded /dev/sda /btrfs
> btrfs fi show -m
> 
> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
> btrfs.
> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>         Total devices 2 FS bytes used 128.00KiB
>         devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>         devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
> 
> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
> when BTRFS_MAGIC check fails, so that its parent open_fs_devices()
> shall free the device in the mount-thread.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> Now the fstests btrfs/198 pass with this fix.
> 
>  fs/btrfs/disk-io.c |  2 +-
>  fs/btrfs/volumes.c | 19 +++++++++++++------
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 160b485d2cc0..9c91d12530a6 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3432,7 +3432,7 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
>  	if (btrfs_super_bytenr(super) != bytenr ||
>  		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>  		btrfs_release_disk_super(super);
> -		return ERR_PTR(-EINVAL);
> +		return ERR_PTR(-ENODATA);

Is ENODATA the return value we've settled in the discussion? Sorry I 
haven't followed it closely.

>  	}
>  
>  	return super;
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7cc677a7e544..ec9dac40b4f1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1198,17 +1198,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>  {
>  	struct btrfs_device *device;
>  	struct btrfs_device *latest_dev = NULL;
> +	struct btrfs_device *tmp_device;
>  
>  	flags |= FMODE_EXCL;
>  
> -	list_for_each_entry(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;
> +	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
> +				 dev_list) {
> +		int ret;
>  
> -		if (!latest_dev ||
> -		    device->generation > latest_dev->generation)
> +		/* Just open everything we can; ignore failures here */
> +		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
> +		if (ret == 0 && (!latest_dev ||
> +		    device->generation > latest_dev->generation)) {
>  			latest_dev = device;
> +		} else if (ret == -ENODATA) {
> +			fs_devices->num_devices--;
> +			list_del(&device->dev_list);
> +			btrfs_free_device(device);
> +		}
>  	}
>  	if (fs_devices->open_devices == 0)
>  		return -EINVAL;
> 


The code itself looks good I think, at least I haven't spotted anything
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] btrfs: free device without BTRFS_MAGIC

[Johannes Thumshirn]

On 19/09/2020 04:53, Anand Jain wrote:
> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
> when BTRFS_MAGIC check fails, so that its parent open_fs_devices()
> shall free the device in the mount-thread.

But now it doesn't only fail if the BTRFS_MAGIC check failed but also,
if the offset of the superblock doesn't match the offset for the copy.

Sorry for not spotting this earlier.

Re: [PATCH] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

On 21/9/20 6:52 pm, Johannes Thumshirn wrote:
> On 19/09/2020 04:53, Anand Jain wrote:
>> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
>> when BTRFS_MAGIC check fails, so that its parent open_fs_devices()
>> shall free the device in the mount-thread.
> 
> But now it doesn't only fail if the BTRFS_MAGIC check failed but also,
> if the offset of the superblock doesn't match the offset for the copy.
> 
> Sorry for not spotting this earlier.
> 


Here are the links to the older comments.

  https://patchwork.kernel.org/patch/11177085/
  https://patchwork.kernel.org/patch/11177081/

I am not sure if -ENODATA is ok. It is open to comments. If it is not ok
then suggestions better alternative will help.

You are right I didn't intend to include btrfs_super_bytenr(super) != 
bytenr under -ENODATA thanks for spotting. Will fix.

[PATCH v2] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

Many things can happen after the device is scanned and before the device
is mounted.

One such thing is losing the BTRFS_MAGIC on the device.

If it happens we still won't free that device from the memory and causes
the userland to confuse.

For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
does not have the BTRFS_MAGIC, the btrfs fi show still shows device
which does not belong. As shown below.

mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb

wipefs -a /dev/sdb
mount -o degraded /dev/sda /btrfs
btrfs fi show -m

/dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
btrfs.
Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
        Total devices 2 FS bytes used 128.00KiB
        devid    1 size 3.00GiB used 571.19MiB path /dev/sda
        devid    2 size 3.00GiB used 571.19MiB path /dev/sdb

Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
when BTRFS_MAGIC check fails, and its parent open_fs_devices() to
free the device in the mount-thread.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Do not return ENODATA on `btrfs_super_bytenr(super) != bytenr`

 fs/btrfs/disk-io.c |  8 ++++++--
 fs/btrfs/volumes.c | 19 +++++++++++++------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 160b485d2cc0..d84a49fe9639 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3429,12 +3429,16 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
 		return ERR_CAST(page);
 
 	super = page_address(page);
-	if (btrfs_super_bytenr(super) != bytenr ||
-		    btrfs_super_magic(super) != BTRFS_MAGIC) {
+	if (btrfs_super_bytenr(super) != bytenr) {
 		btrfs_release_disk_super(super);
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
+		btrfs_release_disk_super(super);
+		return ERR_PTR(-ENODATA);
+	}
+
 	return super;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7cc677a7e544..ec9dac40b4f1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1198,17 +1198,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 {
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_device *tmp_device;
 
 	flags |= FMODE_EXCL;
 
-	list_for_each_entry(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;
+	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
+				 dev_list) {
+		int ret;
 
-		if (!latest_dev ||
-		    device->generation > latest_dev->generation)
+		/* Just open everything we can; ignore failures here */
+		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
+		if (ret == 0 && (!latest_dev ||
+		    device->generation > latest_dev->generation)) {
 			latest_dev = device;
+		} else if (ret == -ENODATA) {
+			fs_devices->num_devices--;
+			list_del(&device->dev_list);
+			btrfs_free_device(device);
+		}
 	}
 	if (fs_devices->open_devices == 0)
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC

[Nikolay Borisov]

On 22.09.20 г. 6:13 ч., Anand Jain wrote:
> Many things can happen after the device is scanned and before the device
> is mounted.
> 
> One such thing is losing the BTRFS_MAGIC on the device.
> 
> If it happens we still won't free that device from the memory and causes
> the userland to confuse.
> 
> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
> which does not belong. As shown below.
> 
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
> 
> wipefs -a /dev/sdb
> mount -o degraded /dev/sda /btrfs
> btrfs fi show -m
> 
> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
> btrfs.
> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>         Total devices 2 FS bytes used 128.00KiB
>         devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>         devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
> 
> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
> when BTRFS_MAGIC check fails, and its parent open_fs_devices() to
> free the device in the mount-thread.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Has an fstest for this been submitted ?

<snip>

Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

On 23/9/20 7:09 pm, Nikolay Borisov wrote:
> 
> 
> On 22.09.20 г. 6:13 ч., Anand Jain wrote:
>> Many things can happen after the device is scanned and before the device
>> is mounted.
>>
>> One such thing is losing the BTRFS_MAGIC on the device.
>>
>> If it happens we still won't free that device from the memory and causes
>> the userland to confuse.
>>
>> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
>> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
>> which does not belong. As shown below.
>>
>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
>>
>> wipefs -a /dev/sdb
>> mount -o degraded /dev/sda /btrfs
>> btrfs fi show -m
>>
>> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
>> btrfs.
>> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>>          Total devices 2 FS bytes used 128.00KiB
>>          devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>>          devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
>>
>> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
>> when BTRFS_MAGIC check fails, and its parent open_fs_devices() to
>> free the device in the mount-thread.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Has an fstest for this been submitted ?
> 

  This is fix for btrfs/198.


> <snip>
> 

Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC

[Josef Bacik]

On 9/21/20 11:13 PM, Anand Jain wrote:
> Many things can happen after the device is scanned and before the device
> is mounted.
> 
> One such thing is losing the BTRFS_MAGIC on the device.
> 
> If it happens we still won't free that device from the memory and causes
> the userland to confuse.
> 
> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
> which does not belong. As shown below.
> 
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
> 
> wipefs -a /dev/sdb
> mount -o degraded /dev/sda /btrfs
> btrfs fi show -m
> 
> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
> btrfs.
> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>          Total devices 2 FS bytes used 128.00KiB
>          devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>          devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
> 

Wouldn't this also happen if the bytenrs didn't match?  In that case you aren't 
freeing anything, and we'd still show the improper device correct?  So why not 
deal with that case in a similar way?  Thanks,

Josef

Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

On 29/9/20 2:14 am, Josef Bacik wrote:
> On 9/21/20 11:13 PM, Anand Jain wrote:
>> Many things can happen after the device is scanned and before the device
>> is mounted.
>>
>> One such thing is losing the BTRFS_MAGIC on the device.
>>
>> If it happens we still won't free that device from the memory and causes
>> the userland to confuse.
>>
>> For example: As the BTRFS_IOC_DEV_INFO still carries the device path 
>> which
>> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
>> which does not belong. As shown below.
>>
>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
>>
>> wipefs -a /dev/sdb
>> mount -o degraded /dev/sda /btrfs
>> btrfs fi show -m
>>
>> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
>> btrfs.
>> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>>          Total devices 2 FS bytes used 128.00KiB
>>          devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>>          devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
>>
> 
> Wouldn't this also happen if the bytenrs didn't match?  In that case you 
> aren't freeing anything, and we'd still show the improper device 
> correct?  So why not deal with that case in a similar way?  Thanks,

Freeing the device without the BTRFS_MAGIC is mandatory because the
device does not belong to btrfs even though we could notice from the
sysfs that there is missing flag on this devid.

I think I should check for the BTRFS_MAGIC first before bytenr check,
I shall swap them in v2 if there are no other comments. We need this
patch as a fix for the test case btrfs/198.

However bytenrs mismatch indicates corruption. If the degraded mount
option is not provided we would fail the mount. The user shall have the
opportunity to fix the corrupted superblock. We don't automatically
recover the corrupted superblock from the backup superblock copies. If
the degraded mount option is provided the corrupted device still be in
the device_list but with the missing flag set. Just by looking at btrfs
fi show the user won't know that one of the devices is not part of the
volume however when he looks into the /sys/fs/btrfs/fsid/devinfo/<devid>
/missing it shall show 1. Our serviceability part of the degraded volume
has some unfinished business when we evaluate it against the standard
RAS features, but we are slowly getting there.

Thanks, Anand

> 
> Josef

Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC

[Josef Bacik]

On 9/30/20 3:21 AM, Anand Jain wrote:
> 
> 
> On 29/9/20 2:14 am, Josef Bacik wrote:
>> On 9/21/20 11:13 PM, Anand Jain wrote:
>>> Many things can happen after the device is scanned and before the device
>>> is mounted.
>>>
>>> One such thing is losing the BTRFS_MAGIC on the device.
>>>
>>> If it happens we still won't free that device from the memory and causes
>>> the userland to confuse.
>>>
>>> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
>>> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
>>> which does not belong. As shown below.
>>>
>>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
>>>
>>> wipefs -a /dev/sdb
>>> mount -o degraded /dev/sda /btrfs
>>> btrfs fi show -m
>>>
>>> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
>>> btrfs.
>>> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>>>          Total devices 2 FS bytes used 128.00KiB
>>>          devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>>>          devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
>>>
>>
>> Wouldn't this also happen if the bytenrs didn't match?  In that case you 
>> aren't freeing anything, and we'd still show the improper device correct?  So 
>> why not deal with that case in a similar way?  Thanks,
> 
> Freeing the device without the BTRFS_MAGIC is mandatory because the
> device does not belong to btrfs even though we could notice from the
> sysfs that there is missing flag on this devid.
> 
> I think I should check for the BTRFS_MAGIC first before bytenr check,
> I shall swap them in v2 if there are no other comments. We need this
> patch as a fix for the test case btrfs/198.
> 
> However bytenrs mismatch indicates corruption. If the degraded mount
> option is not provided we would fail the mount. The user shall have the
> opportunity to fix the corrupted superblock. We don't automatically
> recover the corrupted superblock from the backup superblock copies. If
> the degraded mount option is provided the corrupted device still be in
> the device_list but with the missing flag set. Just by looking at btrfs
> fi show the user won't know that one of the devices is not part of the
> volume however when he looks into the /sys/fs/btrfs/fsid/devinfo/<devid>
> /missing it shall show 1. Our serviceability part of the degraded volume
> has some unfinished business when we evaluate it against the standard
> RAS features, but we are slowly getting there.
> 

Alright that's fair.  You can add

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

Thanks,

Josef

[PATCH v3] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

Many things can happen after the device is scanned and before the device
is mounted.

One such thing is losing the BTRFS_MAGIC on the device.

If it happens we still won't free that device from the memory and causes
the userland to confuse.

For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
does not have the BTRFS_MAGIC, the btrfs fi show still shows device
which does not belong. As shown below.

mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb

wipefs -a /dev/sdb
mount -o degraded /dev/sda /btrfs
btrfs fi show -m

/dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
btrfs.
Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
        Total devices 2 FS bytes used 128.00KiB
        devid    1 size 3.00GiB used 571.19MiB path /dev/sda
        devid    2 size 3.00GiB used 571.19MiB path /dev/sdb

Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
when BTRFS_MAGIC check fails, and its parent open_fs_devices() to
free the device in the mount-thread.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v3: First check for the BTRFS_MAGIC and then the bytenr check.
    Add rb.
v2: Do not return ENODATA on `btrfs_super_bytenr(super) != bytenr`

 fs/btrfs/disk-io.c |  8 ++++++--
 fs/btrfs/volumes.c | 19 +++++++++++++------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3d39f5d47ad3..764001609a15 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3424,8 +3424,12 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
 		return ERR_CAST(page);
 
 	super = page_address(page);
-	if (btrfs_super_bytenr(super) != bytenr ||
-		    btrfs_super_magic(super) != BTRFS_MAGIC) {
+	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
+		btrfs_release_disk_super(super);
+		return ERR_PTR(-ENODATA);
+	}
+
+	if (btrfs_super_bytenr(super) != bytenr) {
 		btrfs_release_disk_super(super);
 		return ERR_PTR(-EINVAL);
 	}
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c69da5eb7636..a208043b4419 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1197,17 +1197,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 {
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_device *tmp_device;
 
 	flags |= FMODE_EXCL;
 
-	list_for_each_entry(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;
+	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
+				 dev_list) {
+		int ret;
 
-		if (!latest_dev ||
-		    device->generation > latest_dev->generation)
+		/* Just open everything we can; ignore failures here */
+		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
+		if (ret == 0 && (!latest_dev ||
+		    device->generation > latest_dev->generation)) {
 			latest_dev = device;
+		} else if (ret == -ENODATA) {
+			fs_devices->num_devices--;
+			list_del(&device->dev_list);
+			btrfs_free_device(device);
+		}
 	}
 	if (fs_devices->open_devices == 0)
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v3] btrfs: free device without BTRFS_MAGIC

[Anand Jain]

David,

  FYI this patch is the fix for fstests btrfs/198

Thanks, Anand

Re: [PATCH v3] btrfs: free device without BTRFS_MAGIC

[David Sterba]

On Wed, Sep 30, 2020 at 09:09:52PM +0800, Anand Jain wrote:
> Many things can happen after the device is scanned and before the device
> is mounted.
> 
> One such thing is losing the BTRFS_MAGIC on the device.
> 
> If it happens we still won't free that device from the memory and causes
> the userland to confuse.
> 
> For example: As the BTRFS_IOC_DEV_INFO still carries the device path which
> does not have the BTRFS_MAGIC, the btrfs fi show still shows device
> which does not belong. As shown below.
> 
> mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb
> 
> wipefs -a /dev/sdb
> mount -o degraded /dev/sda /btrfs
> btrfs fi show -m
> 
> /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of
> btrfs.
> Label: none  uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd
>         Total devices 2 FS bytes used 128.00KiB
>         devid    1 size 3.00GiB used 571.19MiB path /dev/sda
>         devid    2 size 3.00GiB used 571.19MiB path /dev/sdb
> 
> Fix is to return -ENODATA error code in btrfs_read_dev_one_super()
> when BTRFS_MAGIC check fails, and its parent open_fs_devices() to
> free the device in the mount-thread.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v3: First check for the BTRFS_MAGIC and then the bytenr check.
>     Add rb.
> v2: Do not return ENODATA on `btrfs_super_bytenr(super) != bytenr`
> 
>  fs/btrfs/disk-io.c |  8 ++++++--
>  fs/btrfs/volumes.c | 19 +++++++++++++------
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3d39f5d47ad3..764001609a15 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3424,8 +3424,12 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
>  		return ERR_CAST(page);
>  
>  	super = page_address(page);
> -	if (btrfs_super_bytenr(super) != bytenr ||
> -		    btrfs_super_magic(super) != BTRFS_MAGIC) {
> +	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
> +		btrfs_release_disk_super(super);
> +		return ERR_PTR(-ENODATA);
> +	}
> +
> +	if (btrfs_super_bytenr(super) != bytenr) {
>  		btrfs_release_disk_super(super);
>  		return ERR_PTR(-EINVAL);
>  	}
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c69da5eb7636..a208043b4419 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1197,17 +1197,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>  {
>  	struct btrfs_device *device;
>  	struct btrfs_device *latest_dev = NULL;
> +	struct btrfs_device *tmp_device;
>  
>  	flags |= FMODE_EXCL;
>  
> -	list_for_each_entry(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;
> +	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
> +				 dev_list) {
> +		int ret;
>  
> -		if (!latest_dev ||
> -		    device->generation > latest_dev->generation)
> +		/* Just open everything we can; ignore failures here */

The comment does not make sense anymore, the failures are checked now.

> +		ret = btrfs_open_one_device(fs_devices, device, flags, holder);
> +		if (ret == 0 && (!latest_dev ||
> +		    device->generation > latest_dev->generation)) {
>  			latest_dev = device;
> +		} else if (ret == -ENODATA) {
> +			fs_devices->num_devices--;
> +			list_del(&device->dev_list);
> +			btrfs_free_device(device);

Now ret is ENODATA and we don't want to propagate that up the to the
callers. It's not a problem right now as open_fs_devices returns
directly or 0 at the end.

Otherwise it looks ok to me.