| Message ID | 1536110090-100798-1-git-send-email-bo.liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: remove redundant btrfs_trans_release_metadata" | expand |
On 5.09.2018 04:14, Liu Bo wrote: > __btrfs_end_transaction() has done the metadata release twice, > probably because it used to process delayed refs in between, but now > that we don't process delayed refs any more, the 2nd release is always > a noop. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/transaction.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index bb1b9f526e98..94b036a74d11 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, > return 0; > } > > - btrfs_trans_release_metadata(trans); > - trans->block_rsv = NULL; > - > - if (!list_empty(&trans->new_bgs)) > - btrfs_create_pending_block_groups(trans); > - The only code which can have any implications to the transaction reserve is the btrfs_Create_pending_block_groups since it does insert items. But at this point trans->block_rsv is already null and additionally even if more reservations are made for this transaction further down either btrfs_commit_transaction is called or the transaction kthread is called which is going to commit it. So this change really seems inconsequential. > trans->delayed_ref_updates = 0; > if (!trans->sync) { > must_run_delayed_refs = >
Somehow this ends up with crash in btrfs/124, I'm trying to figure out what went wrong. thanks, liubo On Tue, Sep 4, 2018 at 6:14 PM, Liu Bo <bo.liu@linux.alibaba.com> wrote: > __btrfs_end_transaction() has done the metadata release twice, > probably because it used to process delayed refs in between, but now > that we don't process delayed refs any more, the 2nd release is always > a noop. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > --- > fs/btrfs/transaction.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index bb1b9f526e98..94b036a74d11 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, > return 0; > } > > - btrfs_trans_release_metadata(trans); > - trans->block_rsv = NULL; > - > - if (!list_empty(&trans->new_bgs)) > - btrfs_create_pending_block_groups(trans); > - > trans->delayed_ref_updates = 0; > if (!trans->sync) { > must_run_delayed_refs = > -- > 1.8.3.1 >
On Wed, Sep 5, 2018 at 10:45 PM, Liu Bo <obuil.liubo@gmail.com> wrote: > Somehow this ends up with crash in btrfs/124, I'm trying to figure out > what went wrong. > It revealed the problem addressed in Josef's patch[1], so with it, this patch works just fine. [1] btrfs: make sure we create all new bgs thanks, liubo > > On Tue, Sep 4, 2018 at 6:14 PM, Liu Bo <bo.liu@linux.alibaba.com> wrote: >> __btrfs_end_transaction() has done the metadata release twice, >> probably because it used to process delayed refs in between, but now >> that we don't process delayed refs any more, the 2nd release is always >> a noop. >> >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >> --- >> fs/btrfs/transaction.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >> index bb1b9f526e98..94b036a74d11 100644 >> --- a/fs/btrfs/transaction.c >> +++ b/fs/btrfs/transaction.c >> @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, >> return 0; >> } >> >> - btrfs_trans_release_metadata(trans); >> - trans->block_rsv = NULL; >> - >> - if (!list_empty(&trans->new_bgs)) >> - btrfs_create_pending_block_groups(trans); >> - >> trans->delayed_ref_updates = 0; >> if (!trans->sync) { >> must_run_delayed_refs = >> -- >> 1.8.3.1 >>
On 6.09.2018 09:47, Liu Bo wrote: > On Wed, Sep 5, 2018 at 10:45 PM, Liu Bo <obuil.liubo@gmail.com> wrote: >> Somehow this ends up with crash in btrfs/124, I'm trying to figure out >> what went wrong. >> > > It revealed the problem addressed in Josef's patch[1], so with it, > this patch works just fine. What exactly was the crash ? > > [1] btrfs: make sure we create all new bgs > > thanks, > liubo > >> >> On Tue, Sep 4, 2018 at 6:14 PM, Liu Bo <bo.liu@linux.alibaba.com> wrote: >>> __btrfs_end_transaction() has done the metadata release twice, >>> probably because it used to process delayed refs in between, but now >>> that we don't process delayed refs any more, the 2nd release is always >>> a noop. >>> >>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >>> --- >>> fs/btrfs/transaction.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >>> index bb1b9f526e98..94b036a74d11 100644 >>> --- a/fs/btrfs/transaction.c >>> +++ b/fs/btrfs/transaction.c >>> @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, >>> return 0; >>> } >>> >>> - btrfs_trans_release_metadata(trans); >>> - trans->block_rsv = NULL; >>> - >>> - if (!list_empty(&trans->new_bgs)) >>> - btrfs_create_pending_block_groups(trans); >>> - >>> trans->delayed_ref_updates = 0; >>> if (!trans->sync) { >>> must_run_delayed_refs = >>> -- >>> 1.8.3.1 >>> >
On Thu, Sep 6, 2018 at 11:50 AM, Nikolay Borisov <nborisov@suse.com> wrote: > > > On 6.09.2018 09:47, Liu Bo wrote: >> On Wed, Sep 5, 2018 at 10:45 PM, Liu Bo <obuil.liubo@gmail.com> wrote: >>> Somehow this ends up with crash in btrfs/124, I'm trying to figure out >>> what went wrong. >>> >> >> It revealed the problem addressed in Josef's patch[1], so with it, >> this patch works just fine. > > What exactly was the crash ? > assertion failed: list_empty(&block_group->bg_list), file: fs/btrfs/extent-tree.c, kernel BUG at fs/btrfs/ctree.h:3427! ... close_ctree+0x142/0x310 [btrfs] thanks, liubo >> >> [1] btrfs: make sure we create all new bgs >> >> thanks, >> liubo >> >>> >>> On Tue, Sep 4, 2018 at 6:14 PM, Liu Bo <bo.liu@linux.alibaba.com> wrote: >>>> __btrfs_end_transaction() has done the metadata release twice, >>>> probably because it used to process delayed refs in between, but now >>>> that we don't process delayed refs any more, the 2nd release is always >>>> a noop. >>>> >>>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >>>> --- >>>> fs/btrfs/transaction.c | 6 ------ >>>> 1 file changed, 6 deletions(-) >>>> >>>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >>>> index bb1b9f526e98..94b036a74d11 100644 >>>> --- a/fs/btrfs/transaction.c >>>> +++ b/fs/btrfs/transaction.c >>>> @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, >>>> return 0; >>>> } >>>> >>>> - btrfs_trans_release_metadata(trans); >>>> - trans->block_rsv = NULL; >>>> - >>>> - if (!list_empty(&trans->new_bgs)) >>>> - btrfs_create_pending_block_groups(trans); >>>> - >>>> trans->delayed_ref_updates = 0; >>>> if (!trans->sync) { >>>> must_run_delayed_refs = >>>> -- >>>> 1.8.3.1 >>>> >>
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index bb1b9f526e98..94b036a74d11 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -826,12 +826,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, return 0; } - btrfs_trans_release_metadata(trans); - trans->block_rsv = NULL; - - if (!list_empty(&trans->new_bgs)) - btrfs_create_pending_block_groups(trans); - trans->delayed_ref_updates = 0; if (!trans->sync) { must_run_delayed_refs =
__btrfs_end_transaction() has done the metadata release twice, probably because it used to process delayed refs in between, but now that we don't process delayed refs any more, the 2nd release is always a noop. Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> --- fs/btrfs/transaction.c | 6 ------ 1 file changed, 6 deletions(-)