| Message ID | 20200407103858.31029-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Btrfs: fix reclaim counter leak of space_info objects | expand |
On 7.04.20 г. 13:38 ч., fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > The reclaim_size counter of a space_info object is unsigned. So its value > can never be negative, it's pointless to have an assertion that checks > its value is >= 0, therefore remove it. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> True, Reviewed-by: Nikolay Borisov <nborisov@suse.com> I guess this could be squashed. > --- > fs/btrfs/space-info.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c > index ff17a4420358..88d7dea215ff 100644 > --- a/fs/btrfs/space-info.c > +++ b/fs/btrfs/space-info.c > @@ -1198,7 +1198,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info, > * the list and we will do our own flushing further down. > */ > if (ret && flush != BTRFS_RESERVE_NO_FLUSH) { > - ASSERT(space_info->reclaim_size >= 0); > ticket.bytes = orig_bytes; > ticket.error = 0; > space_info->reclaim_size += ticket.bytes; >
On Tue, Apr 7, 2020 at 12:32 PM Nikolay Borisov <nborisov@suse.com> wrote: > > > > On 7.04.20 г. 13:38 ч., fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > The reclaim_size counter of a space_info object is unsigned. So its value > > can never be negative, it's pointless to have an assertion that checks > > its value is >= 0, therefore remove it. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > True, > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> I guess this could be > squashed. Despite being a trivial and small change, I don't think it should be squashed into the previous patch, as it's not part of the bug fix regarding the counter leak. Different changes and unrelated changes should be separate patches. Thanks. > > > --- > > fs/btrfs/space-info.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c > > index ff17a4420358..88d7dea215ff 100644 > > --- a/fs/btrfs/space-info.c > > +++ b/fs/btrfs/space-info.c > > @@ -1198,7 +1198,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info, > > * the list and we will do our own flushing further down. > > */ > > if (ret && flush != BTRFS_RESERVE_NO_FLUSH) { > > - ASSERT(space_info->reclaim_size >= 0); > > ticket.bytes = orig_bytes; > > ticket.error = 0; > > space_info->reclaim_size += ticket.bytes; > >
On 7.04.20 г. 17:14 ч., Filipe Manana wrote: > On Tue, Apr 7, 2020 at 12:32 PM Nikolay Borisov <nborisov@suse.com> wrote: >> >> >> >> On 7.04.20 г. 13:38 ч., fdmanana@kernel.org wrote: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> The reclaim_size counter of a space_info object is unsigned. So its value >>> can never be negative, it's pointless to have an assertion that checks >>> its value is >= 0, therefore remove it. >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> >> True, >> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> I guess this could be >> squashed. > > Despite being a trivial and small change, I don't think it should be > squashed into the previous patch, as it's not part of the bug fix > regarding the counter leak. > Different changes and unrelated changes should be separate patches. I meant to say squashed into the original commit that introduced the assert but it seems it has already been merged into master so yeah, it will go as it is.
On Tue, Apr 07, 2020 at 11:38:58AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > The reclaim_size counter of a space_info object is unsigned. So its value > can never be negative, it's pointless to have an assertion that checks > its value is >= 0, therefore remove it. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c index ff17a4420358..88d7dea215ff 100644 --- a/fs/btrfs/space-info.c +++ b/fs/btrfs/space-info.c @@ -1198,7 +1198,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info, * the list and we will do our own flushing further down. */ if (ret && flush != BTRFS_RESERVE_NO_FLUSH) { - ASSERT(space_info->reclaim_size >= 0); ticket.bytes = orig_bytes; ticket.error = 0; space_info->reclaim_size += ticket.bytes;