[1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups
diff mbox series

Message ID 20190402180956.28893-1-jeffm@suse.com
State New
Headers show
Series
  • [1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups
Related show

Commit Message

Jeff Mahoney April 2, 2019, 6:09 p.m. UTC
From: Jeff Mahoney <jeffm@suse.com>

When repairing the extent tree, it's possible for delayed extents to
be created when running btrfs_write_dirty_block_groups.  We run
delayed refs one last time in the kernel but that is missing in
the userspace tools.

That results in delayed refs getting dropped on the floor, the extent
records not getting created, and in the next tranaction, when the
extent tree is CoW'd again, we hit the BUG_ON when we can't find
the extent record.

We can fix this by running the delayed refs after writing out the
dirty block groups.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Filipe Manana April 2, 2019, 7:19 p.m. UTC | #1
On Tue, Apr 2, 2019 at 7:29 PM <jeffm@suse.com> wrote:
>
> From: Jeff Mahoney <jeffm@suse.com>
>
> When repairing the extent tree, it's possible for delayed extents to
> be created when running btrfs_write_dirty_block_groups.  We run
> delayed refs one last time in the kernel but that is missing in
> the userspace tools.
>
> That results in delayed refs getting dropped on the floor, the extent
> records not getting created, and in the next tranaction, when the
> extent tree is CoW'd again, we hit the BUG_ON when we can't find
> the extent record.
>
> We can fix this by running the delayed refs after writing out the
> dirty block groups.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  transaction.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/transaction.c b/transaction.c
> index e756db33..2f19e9c8 100644
> --- a/transaction.c
> +++ b/transaction.c
> @@ -194,6 +194,8 @@ commit_tree:
>         ret = btrfs_run_delayed_refs(trans, -1);
>         BUG_ON(ret);
>         btrfs_write_dirty_block_groups(trans);
> +       ret = btrfs_run_delayed_refs(trans, -1);
> +       BUG_ON(ret);

And running delayed refs can dirty more block groups as well.
At this point shouldn't we loop running delayed refs until no more
dirty block groups exist? Just like in the kernel.

thanks

>         __commit_transaction(trans, root);
>         if (ret < 0)
>                 goto out;
> --
> 2.16.4
>
Jeff Mahoney April 4, 2019, 2:38 a.m. UTC | #2
On 4/2/19 3:19 PM, Filipe Manana wrote:
> On Tue, Apr 2, 2019 at 7:29 PM <jeffm@suse.com> wrote:
>>
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> When repairing the extent tree, it's possible for delayed extents to
>> be created when running btrfs_write_dirty_block_groups.  We run
>> delayed refs one last time in the kernel but that is missing in
>> the userspace tools.
>>
>> That results in delayed refs getting dropped on the floor, the extent
>> records not getting created, and in the next tranaction, when the
>> extent tree is CoW'd again, we hit the BUG_ON when we can't find
>> the extent record.
>>
>> We can fix this by running the delayed refs after writing out the
>> dirty block groups.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  transaction.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/transaction.c b/transaction.c
>> index e756db33..2f19e9c8 100644
>> --- a/transaction.c
>> +++ b/transaction.c
>> @@ -194,6 +194,8 @@ commit_tree:
>>         ret = btrfs_run_delayed_refs(trans, -1);
>>         BUG_ON(ret);
>>         btrfs_write_dirty_block_groups(trans);
>> +       ret = btrfs_run_delayed_refs(trans, -1);
>> +       BUG_ON(ret);
> 
> And running delayed refs can dirty more block groups as well.
> At this point shouldn't we loop running delayed refs until no more
> dirty block groups exist? Just like in the kernel.

Right.  This is another argument for code sharing between the kernel and
userspace.

-Jeff

> thanks
> 
>>         __commit_transaction(trans, root);
>>         if (ret < 0)
>>                 goto out;
>> --
>> 2.16.4
>>
> 
>
David Sterba May 15, 2019, 2:16 p.m. UTC | #3
On Wed, Apr 03, 2019 at 10:38:09PM -0400, Jeff Mahoney wrote:
> On 4/2/19 3:19 PM, Filipe Manana wrote:
> > On Tue, Apr 2, 2019 at 7:29 PM <jeffm@suse.com> wrote:
> >>
> >> From: Jeff Mahoney <jeffm@suse.com>
> >>
> >> When repairing the extent tree, it's possible for delayed extents to
> >> be created when running btrfs_write_dirty_block_groups.  We run
> >> delayed refs one last time in the kernel but that is missing in
> >> the userspace tools.
> >>
> >> That results in delayed refs getting dropped on the floor, the extent
> >> records not getting created, and in the next tranaction, when the
> >> extent tree is CoW'd again, we hit the BUG_ON when we can't find
> >> the extent record.
> >>
> >> We can fix this by running the delayed refs after writing out the
> >> dirty block groups.
> >>
> >> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> >> ---
> >>  transaction.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/transaction.c b/transaction.c
> >> index e756db33..2f19e9c8 100644
> >> --- a/transaction.c
> >> +++ b/transaction.c
> >> @@ -194,6 +194,8 @@ commit_tree:
> >>         ret = btrfs_run_delayed_refs(trans, -1);
> >>         BUG_ON(ret);
> >>         btrfs_write_dirty_block_groups(trans);
> >> +       ret = btrfs_run_delayed_refs(trans, -1);
> >> +       BUG_ON(ret);
> > 
> > And running delayed refs can dirty more block groups as well.
> > At this point shouldn't we loop running delayed refs until no more
> > dirty block groups exist? Just like in the kernel.
> 
> Right.  This is another argument for code sharing between the kernel and
> userspace.

Sharing code in this function would be really hard, I've implemented the
loop in commit in progs.
Filipe Manana May 15, 2019, 2:45 p.m. UTC | #4
On Wed, May 15, 2019 at 3:15 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Apr 03, 2019 at 10:38:09PM -0400, Jeff Mahoney wrote:
> > On 4/2/19 3:19 PM, Filipe Manana wrote:
> > > On Tue, Apr 2, 2019 at 7:29 PM <jeffm@suse.com> wrote:
> > >>
> > >> From: Jeff Mahoney <jeffm@suse.com>
> > >>
> > >> When repairing the extent tree, it's possible for delayed extents to
> > >> be created when running btrfs_write_dirty_block_groups.  We run
> > >> delayed refs one last time in the kernel but that is missing in
> > >> the userspace tools.
> > >>
> > >> That results in delayed refs getting dropped on the floor, the extent
> > >> records not getting created, and in the next tranaction, when the
> > >> extent tree is CoW'd again, we hit the BUG_ON when we can't find
> > >> the extent record.
> > >>
> > >> We can fix this by running the delayed refs after writing out the
> > >> dirty block groups.
> > >>
> > >> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > >> ---
> > >>  transaction.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/transaction.c b/transaction.c
> > >> index e756db33..2f19e9c8 100644
> > >> --- a/transaction.c
> > >> +++ b/transaction.c
> > >> @@ -194,6 +194,8 @@ commit_tree:
> > >>         ret = btrfs_run_delayed_refs(trans, -1);
> > >>         BUG_ON(ret);
> > >>         btrfs_write_dirty_block_groups(trans);
> > >> +       ret = btrfs_run_delayed_refs(trans, -1);
> > >> +       BUG_ON(ret);
> > >
> > > And running delayed refs can dirty more block groups as well.
> > > At this point shouldn't we loop running delayed refs until no more
> > > dirty block groups exist? Just like in the kernel.
> >
> > Right.  This is another argument for code sharing between the kernel and
> > userspace.
>
> Sharing code in this function would be really hard, I've implemented the
> loop in commit in progs.

Shouldn't the new patch version be sent to the list for review?
It doesn't seem to be a trivial change on first through.

Thanks.
David Sterba May 17, 2019, 1:12 p.m. UTC | #5
On Wed, May 15, 2019 at 03:45:13PM +0100, Filipe Manana wrote:
> > > > And running delayed refs can dirty more block groups as well.
> > > > At this point shouldn't we loop running delayed refs until no more
> > > > dirty block groups exist? Just like in the kernel.
> > >
> > > Right.  This is another argument for code sharing between the kernel and
> > > userspace.
> >
> > Sharing code in this function would be really hard, I've implemented the
> > loop in commit in progs.
> 
> Shouldn't the new patch version be sent to the list for review?
> It doesn't seem to be a trivial change on first through.

Ok, I've removed the patches from devel and will send them once the
release is done.

Patch
diff mbox series

diff --git a/transaction.c b/transaction.c
index e756db33..2f19e9c8 100644
--- a/transaction.c
+++ b/transaction.c
@@ -194,6 +194,8 @@  commit_tree:
 	ret = btrfs_run_delayed_refs(trans, -1);
 	BUG_ON(ret);
 	btrfs_write_dirty_block_groups(trans);
+	ret = btrfs_run_delayed_refs(trans, -1);
+	BUG_ON(ret);
 	__commit_transaction(trans, root);
 	if (ret < 0)
 		goto out;