| Message ID | 20191112122416.30672-6-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] btrfs: decrement number of open devices after closing the device not before | expand |
On 2019/11/12 下午8:24, 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> > --- > fs/btrfs/volumes.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index be1fd935edf7..844333b96075 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1128,7 +1128,12 @@ 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--; > + fs_devices->seeding--; This seeding-- doesn't look safe to me. From what I see, fs_devices->seeding seems to be bool value (0 or 1). Wouldn't this seeding-- underflow? Thanks, Qu > } > mutex_unlock(&fs_devices->device_list_mutex); > >
On Tue, Nov 12, 2019 at 08:41:50PM +0800, Qu Wenruo wrote: > > > On 2019/11/12 下午8:24, 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> > > --- > > fs/btrfs/volumes.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index be1fd935edf7..844333b96075 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -1128,7 +1128,12 @@ 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--; > > + fs_devices->seeding--; > > This seeding-- doesn't look safe to me. Yeah, same here, it could be correct in the sense that it's 1 -> 0 exactly once, but otherwise its a bool, and handled in a special way.
On 12/11/2019 13:55, David Sterba wrote: > On Tue, Nov 12, 2019 at 08:41:50PM +0800, Qu Wenruo wrote: >> >> >> On 2019/11/12 下午8:24, 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> >>> --- >>> fs/btrfs/volumes.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index be1fd935edf7..844333b96075 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -1128,7 +1128,12 @@ 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--; >>> + fs_devices->seeding--; >> >> This seeding-- doesn't look safe to me. > > Yeah, same here, it could be correct in the sense that it's 1 -> 0 > exactly once, but otherwise its a bool, and handled in a special way. Yeah it looks like an overlook on my side, I'll correct it in the next revision.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index be1fd935edf7..844333b96075 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1128,7 +1128,12 @@ 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--; + fs_devices->seeding--; } 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> --- fs/btrfs/volumes.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)