| Message ID | dbc067b24194241f6d87b8f9799d9b6484984a13.1600473987.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free device without BTRFS_MAGIC | expand |
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>
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.
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.
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;
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(-)