btrfs-progs: check: run delayed refs after writing out dirty block groups

PATCH In review [email protected] 3 participants 10 messages Apr 2, 2019 – Jul 24, 2019 Btrfs file system

lore

Share & tools

lore

[email protected]

author

2019-04-02 18:10 UTC #0 lore raw

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

From: Jeff Mahoney <[email protected]>

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 <[email protected]>

---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)

transaction.c +2 -0

index e756db33..2f19e9c8 100644
@@ -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;

Signature · 1 line

2.16.4

[email protected]

author

2019-04-02 18:10 UTC #1 lore raw

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

From: Jeff Mahoney <[email protected]>

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 <[email protected]>

---
 check/main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

check/main.c +9 -1

index 7547209c..4a27a443 100644
@@ -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);

Signature · 1 line

2.16.4

Filipe Manana

Reviewed-by on v1 2/2

2019-04-02 19:21 UTC #2 lore raw

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

Quoted 7 lines · On Tue, Apr 2, 2019 at 7:31 PM <[email protected]> wrote:

From: Jeff Mahoney <[email protected]>
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.

Quoted 5 lines

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 <[email protected]>

Reviewed-by Filipe Manana <[email protected]>

Looks good.

Quoted 37 lines

---
 check/main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

check/main.c +9 -2

index 7547209c..4a27a443 100644
@@ -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

Signature · 3 lines

Filipe David Manana,

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

Filipe Manana

2019-04-02 19:19 UTC #3 lore raw

In reply to [email protected]

Quoted 26 lines · On Tue, Apr 2, 2019 at 7:29 PM <[email protected]> wrote:

From: Jeff Mahoney <[email protected]>
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 <[email protected]>
---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)
transaction.c +0 -0
index e756db33..2f19e9c8 100644

transaction.c commit_tree: · line 198

Quoted 5 lines

transaction.c +2 -0

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

Quoted 6 lines

transaction.c +0 -1

        __commit_transaction(trans, root);
        if (ret < 0)
                goto out;
--

2.16.4

Signature · 3 lines

Filipe David Manana,

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

Jeff Mahoney

author

2019-04-04 02:38 UTC #4 lore raw

Quoted 22 lines · Filipe Manana

Quoted 21 lines · On Tue, Apr 2, 2019 at 7:29 PM <[email protected]> wrote:
From: Jeff Mahoney <[email protected]>
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 <[email protected]>
---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 14 lines

transaction.c commit_tree: · line 198
Quoted 10 lines
transaction.c +2 -0
index e756db33..2f19e9c8 100644
@@ -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

Quoted 10 lines

thanks

Quoted 6 lines

transaction.c +0 -1

        __commit_transaction(trans, root);
        if (ret < 0)
                goto out;
--

2.16.4

Signature · 2 lines

Jeff Mahoney
SUSE Labs

David Sterba

2019-05-15 14:15 UTC #5 lore raw

Quoted 38 lines · Jeff Mahoney

Quoted 22 lines · Filipe Manana
Quoted 21 lines · On Tue, Apr 2, 2019 at 7:29 PM <[email protected]> wrote:
From: Jeff Mahoney <[email protected]>
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 <[email protected]>
---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 14 lines
transaction.c commit_tree: · line 198
Quoted 10 lines
transaction.c +2 -0
index e756db33..2f19e9c8 100644
@@ -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.

Quoted 2 lines

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

2019-05-15 14:45 UTC #6 lore raw

Quoted 40 lines · David Sterba

Quoted 38 lines · Jeff Mahoney
Quoted 36 lines · Filipe Manana
Quoted 26 lines · On Tue, Apr 2, 2019 at 7:29 PM <[email protected]> wrote:
From: Jeff Mahoney <[email protected]>
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 <[email protected]>
---
 transaction.c | 2 ++
 1 file changed, 2 insertions(+)
transaction.c +0 -0
index e756db33..2f19e9c8 100644
transaction.c commit_tree: · line 198
Quoted 5 lines
transaction.c +2 -0
@@ -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.

Quoted 5 lines

Quoted 2 lines
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.

Signature · 3 lines

Filipe David Manana,

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

David Sterba

2019-05-17 13:11 UTC #7 lore raw

Quoted 4 lines · Filipe Manana

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Jeff Mahoney

author

2019-07-24 13:53 UTC #8 lore raw

Quoted 8 lines · David Sterba

Quoted 7 lines · Filipe Manana
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
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

Signature · 2 lines

Jeff Mahoney
SUSE Labs

David Sterba

2019-07-24 14:17 UTC #9 lore raw

Quoted 15 lines · Jeff Mahoney

Quoted 14 lines · David Sterba
Quoted 12 lines · Filipe Manana
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.