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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
