* [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups @ 2019-04-02 18:09 jeffm 2019-04-02 18:09 ` [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata jeffm 2019-04-02 19:19 ` [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups Filipe Manana 0 siblings, 2 replies; 10+ messages in thread From: jeffm @ 2019-04-02 18:09 UTC (permalink / raw) To: linux-btrfs; +Cc: Jeff Mahoney 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata 2019-04-02 18:09 [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups jeffm @ 2019-04-02 18:09 ` jeffm 2019-04-02 19:21 ` Filipe Manana 2019-04-02 19:19 ` [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups Filipe Manana 1 sibling, 1 reply; 10+ messages in thread From: jeffm @ 2019-04-02 18:09 UTC (permalink / raw) To: linux-btrfs; +Cc: Jeff Mahoney 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata 2019-04-02 18:09 ` [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata jeffm @ 2019-04-02 19:21 ` Filipe Manana 0 siblings, 0 replies; 10+ messages in thread From: Filipe Manana @ 2019-04-02 19:21 UTC (permalink / raw) To: Jeff Mahoney; +Cc: linux-btrfs 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.” ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-04-02 18:09 [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups jeffm 2019-04-02 18:09 ` [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata jeffm @ 2019-04-02 19:19 ` Filipe Manana 2019-04-04 2:38 ` Jeff Mahoney 1 sibling, 1 reply; 10+ messages in thread From: Filipe Manana @ 2019-04-02 19:19 UTC (permalink / raw) To: Jeff Mahoney; +Cc: linux-btrfs 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.” ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-04-02 19:19 ` [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups Filipe Manana @ 2019-04-04 2:38 ` Jeff Mahoney 2019-05-15 14:16 ` David Sterba 0 siblings, 1 reply; 10+ messages in thread From: Jeff Mahoney @ 2019-04-04 2:38 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-04-04 2:38 ` Jeff Mahoney @ 2019-05-15 14:16 ` David Sterba 2019-05-15 14:45 ` Filipe Manana 0 siblings, 1 reply; 10+ messages in thread From: David Sterba @ 2019-05-15 14:16 UTC (permalink / raw) To: Jeff Mahoney; +Cc: fdmanana, linux-btrfs 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-05-15 14:16 ` David Sterba @ 2019-05-15 14:45 ` Filipe Manana 2019-05-17 13:12 ` David Sterba 0 siblings, 1 reply; 10+ messages in thread From: Filipe Manana @ 2019-05-15 14:45 UTC (permalink / raw) To: dsterba, Jeff Mahoney, Filipe Manana, linux-btrfs 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.” ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-05-15 14:45 ` Filipe Manana @ 2019-05-17 13:12 ` David Sterba 2019-07-24 13:53 ` Jeff Mahoney 0 siblings, 1 reply; 10+ messages in thread From: David Sterba @ 2019-05-17 13:12 UTC (permalink / raw) To: Filipe Manana; +Cc: dsterba, Jeff Mahoney, linux-btrfs 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-05-17 13:12 ` David Sterba @ 2019-07-24 13:53 ` Jeff Mahoney 2019-07-24 14:17 ` David Sterba 0 siblings, 1 reply; 10+ messages in thread From: Jeff Mahoney @ 2019-07-24 13:53 UTC (permalink / raw) To: dsterba, Filipe Manana, linux-btrfs [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups 2019-07-24 13:53 ` Jeff Mahoney @ 2019-07-24 14:17 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2019-07-24 14:17 UTC (permalink / raw) To: Jeff Mahoney; +Cc: dsterba, Filipe Manana, linux-btrfs 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-24 14:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-02 18:09 [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups jeffm 2019-04-02 18:09 ` [PATCH 2/2] btrfs-progs: check: fixup_extent_flags needs to deal with non-skinny metadata jeffm 2019-04-02 19:21 ` Filipe Manana 2019-04-02 19:19 ` [PATCH 1/2] btrfs-progs: check: run delayed refs after writing out dirty block groups Filipe Manana 2019-04-04 2:38 ` Jeff Mahoney 2019-05-15 14:16 ` David Sterba 2019-05-15 14:45 ` Filipe Manana 2019-05-17 13:12 ` David Sterba 2019-07-24 13:53 ` Jeff Mahoney 2019-07-24 14:17 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).