Re: [PATCH v2] Btrfs: fix memory leak of block group cache

[Liu Bo <bo.li.liu@oracle.com>]

On Thu, Jul 21, 2016 at 03:17:41PM -0400, Chris Mason wrote:
> 
> 
> On 07/21/2016 03:03 PM, Liu Bo wrote:
> > On Thu, Jul 21, 2016 at 11:32:26AM -0400, Chris Mason wrote:
> > > 
> > > 
> > > On 07/20/2016 08:44 PM, Liu Bo wrote:
> > > > While processing delayed refs, we may update block group's statistics
> > > > and attach it to cur_trans->dirty_bgs, and later writing dirty block
> > > > groups will process the list, which happens during
> > > > btrfs_commit_transaction().
> > > > 
> > > > For whatever reason, the transaction is aborted and dirty_bgs
> > > > is not processed in cleanup_transaction(), we end up with memory leak
> > > > of these dirty block group cache.
> > > > 
> > > > Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
> > > > critical section, this also adds the cleanup work inside it.
> > > 
> > > It's the start_drity_block_groups() hunt that worries me a bit:
> > > 
> > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > > index 50bd683..7a35c9d 100644
> > > > --- a/fs/btrfs/extent-tree.c
> > > > +++ b/fs/btrfs/extent-tree.c
> > > > @@ -3698,6 +3698,8 @@ again:
> > > >  			goto again;
> > > >  		}
> > > >  		spin_unlock(&cur_trans->dirty_bgs_lock);
> > > > +	} else if (ret < 0) {
> > > > +		btrfs_cleanup_dirty_bgs(cur_trans, root);
> > > >  	}
> > > > 
> > > >  	btrfs_free_path(path);
> > > > 
> > > 
> > > We have checks in here to make sure only one process runs
> > > btrfs_start_dirty_block_groups() but that doesn't mean that only one process
> > > its messing around with the cache inode.  Is there any reason we can't let
> > > this cleanup wait for the cleanup_transaction code?
> > > 
> > > btrfs_run_delayed_refs() already aborts when it fails.
> > 
> > update_block_group() is the only producer to add block group cache to
> > dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
> > is aborted, so seems that there won't be anyone manipulating dirty_bgs
> > list, am I missing?
> > 
> 
> No, the dirty_bgs processing is safe I think.  My concern is with the cache
> inode which we iput()

I think iput() is OK, we're doing iput() on block group cache on the io_bgs
list, where all block groups's inodes has been igrab()'d.  If others are
messing around with our cache inode, they should have their own igrab,
too.

> 
> > Another point is that when we fail on btrfs_start_dirty_block_groups(),
> > btrfs_commit_transaction() won't get to cleanup_transaction error
> > handling,
> 
> Right, because we don't actually finish the commit.  Someone will eventually
> though ;)

Hmm yes, it's possible that there's a concurrent commit transaction
running.  If that's not true, we may still resort to
btrfs_error_commit_super(), other than that, I don't see who could
commit/cleanup the transaction after entering into BTRFS_FS_STATE_ERROR
state.

Thanks,

-liubo

> 
> > 
> > btrfs_commit_transaction() {
> > 	...
> > 	if (!test_bit(BTRFS_TRANS_DIRTY_BG_RUN, &cur_trans->flags)) {
> > 		...
> > 		ret = btrfs_start_dirty_block_groups(trans, root);
> > 	}
> > 	if (ret) {
> > 		btrfs_end_transaction(trans, root);
> > 		return ret;
> > 	}
> > 	...
> > 	cleanup_transaction();
> > }
> > 
> > 
> > But yes, if we delay the cleanup, we still have a chance to do cleanup
> > in btrfs_error_commit_super(), and I have sent another patch to add
> > several ASSERT()s to check block group related memory leak, with which
> > we'll be warned if anything wrong.
> > 
> > I'm OK to remove the part that causes concerns.
> 
> Thanks.
> 
> -chris
>