Re: [PATCH v2 5/7] btrfs: remove final BUG_ON() in close_fs_devices()

[David Sterba <dsterba@suse.cz>]

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