* Re: [PATCH] btrfs: free device without BTRFS_MAGIC
2020-09-19 2:52 [PATCH] btrfs: free device without BTRFS_MAGIC Anand Jain
@ 2020-09-21 9:44 ` Johannes Thumshirn
2020-09-21 10:52 ` Johannes Thumshirn
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Johannes Thumshirn @ 2020-09-21 9:44 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] btrfs: free device without BTRFS_MAGIC
2020-09-19 2:52 [PATCH] btrfs: free device without BTRFS_MAGIC Anand Jain
2020-09-21 9:44 ` Johannes Thumshirn
@ 2020-09-21 10:52 ` Johannes Thumshirn
2020-09-21 11:19 ` Anand Jain
2020-09-22 3:13 ` [PATCH v2] " Anand Jain
2020-09-30 13:09 ` [PATCH v3] " Anand Jain
3 siblings, 1 reply; 13+ messages in thread
From: Johannes Thumshirn @ 2020-09-21 10:52 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: free device without BTRFS_MAGIC
2020-09-21 10:52 ` Johannes Thumshirn
@ 2020-09-21 11:19 ` Anand Jain
0 siblings, 0 replies; 13+ messages in thread
From: Anand Jain @ 2020-09-21 11:19 UTC (permalink / raw)
To: Johannes Thumshirn, linux-btrfs
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-19 2:52 [PATCH] btrfs: free device without BTRFS_MAGIC Anand Jain
2020-09-21 9:44 ` Johannes Thumshirn
2020-09-21 10:52 ` Johannes Thumshirn
@ 2020-09-22 3:13 ` Anand Jain
2020-09-23 11:09 ` Nikolay Borisov
2020-09-28 18:14 ` Josef Bacik
2020-09-30 13:09 ` [PATCH v3] " Anand Jain
3 siblings, 2 replies; 13+ messages in thread
From: Anand Jain @ 2020-09-22 3:13 UTC (permalink / raw)
To: linux-btrfs; +Cc: Johannes.Thumshirn
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-22 3:13 ` [PATCH v2] " Anand Jain
@ 2020-09-23 11:09 ` Nikolay Borisov
2020-09-24 11:55 ` Anand Jain
2020-09-28 18:14 ` Josef Bacik
1 sibling, 1 reply; 13+ messages in thread
From: Nikolay Borisov @ 2020-09-23 11:09 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: Johannes.Thumshirn
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-23 11:09 ` Nikolay Borisov
@ 2020-09-24 11:55 ` Anand Jain
0 siblings, 0 replies; 13+ messages in thread
From: Anand Jain @ 2020-09-24 11:55 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs; +Cc: Johannes.Thumshirn
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>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-22 3:13 ` [PATCH v2] " Anand Jain
2020-09-23 11:09 ` Nikolay Borisov
@ 2020-09-28 18:14 ` Josef Bacik
2020-09-30 7:21 ` Anand Jain
1 sibling, 1 reply; 13+ messages in thread
From: Josef Bacik @ 2020-09-28 18:14 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: Johannes.Thumshirn
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-28 18:14 ` Josef Bacik
@ 2020-09-30 7:21 ` Anand Jain
2020-09-30 12:41 ` Josef Bacik
0 siblings, 1 reply; 13+ messages in thread
From: Anand Jain @ 2020-09-30 7:21 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs; +Cc: Johannes.Thumshirn
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs: free device without BTRFS_MAGIC
2020-09-30 7:21 ` Anand Jain
@ 2020-09-30 12:41 ` Josef Bacik
0 siblings, 0 replies; 13+ messages in thread
From: Josef Bacik @ 2020-09-30 12:41 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: Johannes.Thumshirn
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] btrfs: free device without BTRFS_MAGIC
2020-09-19 2:52 [PATCH] btrfs: free device without BTRFS_MAGIC Anand Jain
` (2 preceding siblings ...)
2020-09-22 3:13 ` [PATCH v2] " Anand Jain
@ 2020-09-30 13:09 ` Anand Jain
2020-10-01 1:05 ` Anand Jain
2020-10-01 10:49 ` David Sterba
3 siblings, 2 replies; 13+ messages in thread
From: Anand Jain @ 2020-09-30 13:09 UTC (permalink / raw)
To: linux-btrfs; +Cc: Johannes.Thumshirn, nborisov, josef
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3] btrfs: free device without BTRFS_MAGIC
2020-09-30 13:09 ` [PATCH v3] " Anand Jain
@ 2020-10-01 1:05 ` Anand Jain
2020-10-01 10:49 ` David Sterba
1 sibling, 0 replies; 13+ messages in thread
From: Anand Jain @ 2020-10-01 1:05 UTC (permalink / raw)
To: dsterba; +Cc: Johannes.Thumshirn, nborisov, josef, linux-btrfs
David,
FYI this patch is the fix for fstests btrfs/198
Thanks, Anand
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] btrfs: free device without BTRFS_MAGIC
2020-09-30 13:09 ` [PATCH v3] " Anand Jain
2020-10-01 1:05 ` Anand Jain
@ 2020-10-01 10:49 ` David Sterba
1 sibling, 0 replies; 13+ messages in thread
From: David Sterba @ 2020-10-01 10:49 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, Johannes.Thumshirn, nborisov, josef
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.
^ permalink raw reply [flat|nested] 13+ messages in thread