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

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

[jeffm]

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);
 	__commit_transaction(trans, root);
 	if (ret < 0)
 		goto out;
-- 
2.16.4

[PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

When repairing a file system created by a very old kernel, I ran into
issues fixing up the extent flags since fixup_extent_flags assumed
that a METADATA_ITEM would be present if the record was for metadata.

Since METADATA_ITEMs don't exist without skinny metadata, we need to
fall back to EXTENT_ITEMs.  This also falls back to EXTENT_ITEMs even
with skinny metadata enabled as other parts of the tools do.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 check/main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index 7547209c..4a27a443 100644
--- a/check/main.c
+++ b/check/main.c
@@ -7540,9 +7540,13 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info,
 	struct btrfs_key key;
 	u64 flags;
 	int ret = 0;
+	bool metadata_item = rec->metadata;
 
+	if (!btrfs_fs_incompat(root->fs_info, SKINNY_METADATA))
+		metadata_item = false;
+retry:
 	key.objectid = rec->start;
-	if (rec->metadata) {
+	if (metadata_item) {
 		key.type = BTRFS_METADATA_ITEM_KEY;
 		key.offset = rec->info_level;
 	} else {
@@ -7561,6 +7565,10 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info,
 		btrfs_commit_transaction(trans, root);
 		return ret;
 	} else if (ret) {
+		if (key.type == BTRFS_METADATA_ITEM_KEY) {
+			metadata_item = false;
+			goto retry;
+		}
 		fprintf(stderr, "Didn't find extent for %llu\n",
 			(unsigned long long)rec->start);
 		btrfs_release_path(&path);
-- 
2.16.4

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[Filipe Manana]

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata

[Filipe Manana]

On Tue, Apr 2, 2019 at 7:31 PM <jeffm@suse.com> wrote:
>
> From: Jeff Mahoney <jeffm@suse.com>
>
> When repairing a file system created by a very old kernel, I ran into
> issues fixing up the extent flags since fixup_extent_flags assumed
> that a METADATA_ITEM would be present if the record was for metadata.
>
> Since METADATA_ITEMs don't exist without skinny metadata, we need to
> fall back to EXTENT_ITEMs.  This also falls back to EXTENT_ITEMs even
> with skinny metadata enabled as other parts of the tools do.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good.

> ---
>  check/main.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/check/main.c b/check/main.c
> index 7547209c..4a27a443 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -7540,9 +7540,13 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info,
>         struct btrfs_key key;
>         u64 flags;
>         int ret = 0;
> +       bool metadata_item = rec->metadata;
>
> +       if (!btrfs_fs_incompat(root->fs_info, SKINNY_METADATA))
> +               metadata_item = false;
> +retry:
>         key.objectid = rec->start;
> -       if (rec->metadata) {
> +       if (metadata_item) {
>                 key.type = BTRFS_METADATA_ITEM_KEY;
>                 key.offset = rec->info_level;
>         } else {
> @@ -7561,6 +7565,10 @@ static int fixup_extent_flags(struct btrfs_fs_info *fs_info,
>                 btrfs_commit_transaction(trans, root);
>                 return ret;
>         } else if (ret) {
> +               if (key.type == BTRFS_METADATA_ITEM_KEY) {
> +                       metadata_item = false;
> +                       goto retry;
> +               }
>                 fprintf(stderr, "Didn't find extent for %llu\n",
>                         (unsigned long long)rec->start);
>                 btrfs_release_path(&path);
> --
> 2.16.4
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1696 bytes --]

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
>>
> 
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[David Sterba]

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.

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[Filipe Manana]

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.



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[David Sterba]

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.

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 843 bytes --]

On 5/17/19 9:12 AM, David Sterba wrote:
> 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.
> 

Hi Dave -

Did this ever get revisited?

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups

[David Sterba]

On Wed, Jul 24, 2019 at 09:53:33AM -0400, Jeff Mahoney wrote:
> On 5/17/19 9:12 AM, David Sterba wrote:
> > 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.
> > 
> 
> Hi Dave -
> 
> Did this ever get revisited?

No, but Qu sent the fix, that's now in devel.