On 13/11/2019 15:58, David Sterba wrote:
> On Wed, Nov 13, 2019 at 11:27:23AM +0100, Johannes Thumshirn wrote:
>> In btrfs_close_one_device() we're allocating a new device and if this
>> fails we BUG().
>>
>> Move the allocation to the top of the function and return an error in case
>> it failed.
>>
>> The BUG_ON() is temporarily moved to close_fs_devices(), the caller of
>> btrfs_close_one_device() as further work is pending to untangle this.
>>
>> Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
>> ---
>> fs/btrfs/volumes.c | 27 +++++++++++++++++++++------
>> 1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 5ee26e7fca32..0a2a73907563 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1061,12 +1061,17 @@ static void btrfs_close_bdev(struct btrfs_device *device)
>> blkdev_put(device->bdev, device->mode);
>> }
>>
>> -static void btrfs_close_one_device(struct btrfs_device *device)
>> +static int btrfs_close_one_device(struct btrfs_device *device)
>> {
>> struct btrfs_fs_devices *fs_devices = device->fs_devices;
>> struct btrfs_device *new_device;
>> struct rcu_string *name;
>>
>> + new_device = btrfs_alloc_device(NULL, &device->devid,
>> + device->uuid);
>> + if (IS_ERR(new_device))
>> + goto err_close_device;
>> +
>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>> device->devid != BTRFS_DEV_REPLACE_DEVID) {
>> list_del_init(&device->dev_alloc_list);
>> @@ -1080,10 +1085,6 @@ static void btrfs_close_one_device(struct btrfs_device *device)
>> if (device->bdev)
>> fs_devices->open_devices--;
>>
>> - new_device = btrfs_alloc_device(NULL, &device->devid,
>> - device->uuid);
>> - BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
>> -
>> /* Safe because we are under uuid_mutex */
>> if (device->name) {
>> name = rcu_string_strdup(device->name->str, GFP_NOFS);
>> @@ -1096,18 +1097,32 @@ static void btrfs_close_one_device(struct btrfs_device *device)
>>
>> synchronize_rcu();
>> btrfs_free_device(device);
>> +
>> + return 0;
>> +
>> +err_close_device:
>> + btrfs_close_bdev(device);
>> + if (device->bdev) {
>> + fs_devices->open_devices--;
>> + btrfs_sysfs_rm_device_link(fs_devices, device);
>> + device->bdev = NULL;
>> + }
>
> I don't understand this part: the 'device' pointer is from the argument,
> so the device we want to delete from the list and for that all the state
> bit tests, bdev close, list replace rcu and synchronize_rcu should
> happen -- in case we have a newly allocated new_device.
>
> What I don't understand how the short version after label
> err_close_device: is correct. The device is still left in the list but
> with NULL bdev but rw_devices, missing_devices is untouched.
>
> That a device closing needs to allocate memory for a new device instead
> of reinitializing it again is stupid but with the simplified device
> closing I'm not sure the state is well defined.
As we couldn't allocate memory to remove the device from the list, we
have to keep it in the list (technically even leaking some memory here).
What we definitively need to do is clear the ->bdev pointer, otherwise
we'll trip over a NULL-pointer in open_fs_devices().
open_fs_devices() will traverse the list and call
btrfs_open_one_device() this will fail as device->bdev is (still) set
thus latest_dev is NULL and then this 'fs_devices->latest_bdev =
latest_dev->bdev;' will blow up.
If you have a better solution I'm all ears. This is what I came up with
to tackle the problem of half initialized devices.
One thing we could do though is call btrfs_free_stale_devices() in the
error case.
Byte,
Johannes
--
Johannes Thumshirn SUSE Labs Filesystems
jthumshirn@xxxxxxx +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
