| Message ID | 20191113102728.8835-6-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] btrfs: decrement number of open devices after closing the device not before | expand |
On Wed, Nov 13, 2019 at 11:27:26AM +0100, Johannes Thumshirn wrote: > Now that the preparation work is done, remove the temporary BUG_ON() in > close_fs_devices() and return an error instead. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > > --- > Changes to v1: > - btrfs_fs_devices::seeding is a 'boolean' flags and no counter, don't > decrement it (Qu) > --- > fs/btrfs/volumes.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index be1fd935edf7..25e4608e20f1 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1128,7 +1128,11 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices) > mutex_lock(&fs_devices->device_list_mutex); > list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { > ret = btrfs_close_one_device(device); > - BUG_ON(ret); /* -ENOMEM */ > + if (ret) { > + mutex_unlock(&fs_devices->device_list_mutex); > + return ret; This can fail in the middle of the loop thus leaving some devices in the list and keeping open_devices half-changed. Not all callers of close_fs_devices handle the errors so this can break invariants, where eg. fs_devices->opened is expected to be 0 after the function call, similar for ->seeding or ->rw_devices. > + } > + fs_devices->opened--; > } > mutex_unlock(&fs_devices->device_list_mutex); > > -- > 2.16.4
On 13/11/2019 16:02, David Sterba wrote: > On Wed, Nov 13, 2019 at 11:27:26AM +0100, Johannes Thumshirn wrote: >> Now that the preparation work is done, remove the temporary BUG_ON() in >> close_fs_devices() and return an error instead. >> >> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> >> >> --- >> Changes to v1: >> - btrfs_fs_devices::seeding is a 'boolean' flags and no counter, don't >> decrement it (Qu) >> --- >> fs/btrfs/volumes.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index be1fd935edf7..25e4608e20f1 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -1128,7 +1128,11 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices) >> mutex_lock(&fs_devices->device_list_mutex); >> list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { >> ret = btrfs_close_one_device(device); >> - BUG_ON(ret); /* -ENOMEM */ >> + if (ret) { >> + mutex_unlock(&fs_devices->device_list_mutex); >> + return ret; > > This can fail in the middle of the loop thus leaving some devices in the > list and keeping open_devices half-changed. > > Not all callers of close_fs_devices handle the errors so this can break > invariants, where eg. fs_devices->opened is expected to be 0 after the > function call, similar for ->seeding or ->rw_devices. Please see my answer to "btrfs: handle device allocation failure in btrfs_close_one_device()" on this topic.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index be1fd935edf7..25e4608e20f1 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1128,7 +1128,11 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices) mutex_lock(&fs_devices->device_list_mutex); list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { ret = btrfs_close_one_device(device); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + mutex_unlock(&fs_devices->device_list_mutex); + return ret; + } + fs_devices->opened--; } mutex_unlock(&fs_devices->device_list_mutex);
Now that the preparation work is done, remove the temporary BUG_ON() in close_fs_devices() and return an error instead. Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- Changes to v1: - btrfs_fs_devices::seeding is a 'boolean' flags and no counter, don't decrement it (Qu) --- fs/btrfs/volumes.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)