* [PATCH 0/2][v3] Some leaked root fixes @ 2020-09-03 18:29 Josef Bacik 2020-09-03 18:29 ` [PATCH 1/2] btrfs: free fs roots on failed mount Josef Bacik 2020-09-03 18:29 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik 0 siblings, 2 replies; 13+ messages in thread From: Josef Bacik @ 2020-09-03 18:29 UTC (permalink / raw) To: linux-btrfs, kernel-team v2->v3: - Reworked the pretty print for roots so it's a little cleaner, also made it so only reloc roots spit out the offset, since that's the only time we care. - Added Nikolay's reviewed-by for the first patch. --- Original email --- Hello, These are two fixes for a leaked root problem I noticed a while ago. One is the actual leaked root fix, the other is to add a pretty print for the leaked root message, as figuring out that some giant %llu number was the data reloc root is not a feat meant for humans. Thanks, Josef Josef Bacik (2): btrfs: free fs roots on failed mount btrfs: pretty print leaked root name fs/btrfs/disk-io.c | 8 ++++++-- fs/btrfs/print-tree.c | 39 +++++++++++++++++++++++++++++++++++++++ fs/btrfs/print-tree.h | 3 +++ 3 files changed, 48 insertions(+), 2 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] btrfs: free fs roots on failed mount 2020-09-03 18:29 [PATCH 0/2][v3] Some leaked root fixes Josef Bacik @ 2020-09-03 18:29 ` Josef Bacik 2020-09-07 12:50 ` David Sterba 2020-09-03 18:29 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik 1 sibling, 1 reply; 13+ messages in thread From: Josef Bacik @ 2020-09-03 18:29 UTC (permalink / raw) To: linux-btrfs, kernel-team; +Cc: Nikolay Borisov While testing a weird problem with -o degraded, I noticed I was getting leaked root errors BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices BTRFS error (device loop0): open_ctree failed BTRFS error (device loop0): leaked root -9-0 refcount 1 This is the DATA_RELOC root, which gets read before the other fs roots, but is included in the fs roots radix tree. Handle this by adding a btrfs_drop_and_free_fs_root() on the data reloc root if it exists. This is ok to do here if we fail further up because we will only drop the ref if we delete the root from the radix tree, and all other cleanup won't be duplicated. Reviewed-by: Nikolay Borisov <nborisov@suse.com> Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/disk-io.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 0df589c95d86..7147237d9bf0 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3415,6 +3415,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device btrfs_put_block_group_cache(fs_info); fail_tree_roots: + if (fs_info->data_reloc_root) + btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root); free_root_pointers(fs_info, true); invalidate_inode_pages2(fs_info->btree_inode->i_mapping); -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] btrfs: free fs roots on failed mount 2020-09-03 18:29 ` [PATCH 1/2] btrfs: free fs roots on failed mount Josef Bacik @ 2020-09-07 12:50 ` David Sterba 0 siblings, 0 replies; 13+ messages in thread From: David Sterba @ 2020-09-07 12:50 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs, kernel-team, Nikolay Borisov On Thu, Sep 03, 2020 at 02:29:50PM -0400, Josef Bacik wrote: > While testing a weird problem with -o degraded, I noticed I was getting > leaked root errors > > BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices > BTRFS error (device loop0): open_ctree failed > BTRFS error (device loop0): leaked root -9-0 refcount 1 > > This is the DATA_RELOC root, which gets read before the other fs roots, > but is included in the fs roots radix tree. Handle this by adding a > btrfs_drop_and_free_fs_root() on the data reloc root if it exists. This > is ok to do here if we fail further up because we will only drop the ref > if we delete the root from the radix tree, and all other cleanup won't > be duplicated. > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> I'll add this to misc-next as this is the fix and does not need to wait on the root pretty printing. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] btrfs: pretty print leaked root name 2020-09-03 18:29 [PATCH 0/2][v3] Some leaked root fixes Josef Bacik 2020-09-03 18:29 ` [PATCH 1/2] btrfs: free fs roots on failed mount Josef Bacik @ 2020-09-03 18:29 ` Josef Bacik 2020-09-07 12:28 ` David Sterba 1 sibling, 1 reply; 13+ messages in thread From: Josef Bacik @ 2020-09-03 18:29 UTC (permalink / raw) To: linux-btrfs, kernel-team I'm a actual human being so am incapable of converting u64 to s64 in my head, so add a helper to get the pretty name of a root objectid and use that helper to spit out the name for any special roots for leaked roots, so I don't have to scratch my head and figure out which root I messed up the refs for. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/disk-io.c | 6 ++++-- fs/btrfs/print-tree.c | 39 +++++++++++++++++++++++++++++++++++++++ fs/btrfs/print-tree.h | 3 +++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 7147237d9bf0..71beb9493ab4 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1504,10 +1504,12 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) struct btrfs_root *root; while (!list_empty(&fs_info->allocated_roots)) { + char buf[BTRFS_ROOT_NAME_BUF_LEN]; + root = list_first_entry(&fs_info->allocated_roots, struct btrfs_root, leak_list); - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", - root->root_key.objectid, root->root_key.offset, + btrfs_err(fs_info, "leaked root %s refcount %d", + btrfs_root_name(root->root_key.objectid, buf), refcount_read(&root->refs)); while (refcount_read(&root->refs) > 1) btrfs_put_root(root); diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c index 80567c11ec12..d0370075a719 100644 --- a/fs/btrfs/print-tree.c +++ b/fs/btrfs/print-tree.c @@ -7,6 +7,45 @@ #include "disk-io.h" #include "print-tree.h" +struct name_map { + u64 id; + char *name; +}; + +static const struct name_map root_map[] = { + { BTRFS_ROOT_TREE_OBJECTID, "ROOT_TREE" }, + { BTRFS_EXTENT_TREE_OBJECTID, "EXTENT_TREE" }, + { BTRFS_CHUNK_TREE_OBJECTID, "CHUNK_TREE" }, + { BTRFS_DEV_TREE_OBJECTID, "DEV_TREE" }, + { BTRFS_FS_TREE_OBJECTID, "FS_TREE" }, + { BTRFS_ROOT_TREE_DIR_OBJECTID, "ROOT_TREE_DIR" }, + { BTRFS_CSUM_TREE_OBJECTID, "CSUM_TREE" }, + { BTRFS_TREE_LOG_OBJECTID, "TREE_LOG" }, + { BTRFS_QUOTA_TREE_OBJECTID, "QUOTA_TREE" }, + { BTRFS_UUID_TREE_OBJECTID, "UUID_TREE" }, + { BTRFS_FREE_SPACE_TREE_OBJECTID, "FREE_SPACE_TREE" }, + { BTRFS_DATA_RELOC_TREE_OBJECTID, "DATA_RELOC_TREE" }, +}; + +char *btrfs_root_name(u64 objectid, char *buf) +{ + int i; + + if (objectid == BTRFS_TREE_RELOC_OBJECTID) { + snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, + "TREE_RELOC offset=%llu", objectid); + return buf; + } + + for (i = 0; i < ARRAY_SIZE(root_map); i++) { + if (root_map[i].id == objectid) + return root_map[i].name; + } + + snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, "%llu", objectid); + return buf; +} + static void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk) { int num_stripes = btrfs_chunk_num_stripes(eb, chunk); diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h index e6bb38fd75ad..8d07f80cead4 100644 --- a/fs/btrfs/print-tree.h +++ b/fs/btrfs/print-tree.h @@ -6,7 +6,10 @@ #ifndef BTRFS_PRINT_TREE_H #define BTRFS_PRINT_TREE_H +#define BTRFS_ROOT_NAME_BUF_LEN 48 + void btrfs_print_leaf(struct extent_buffer *l); void btrfs_print_tree(struct extent_buffer *c, bool follow); +char *btrfs_root_name(u64 objectid, char *buf); #endif -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-09-03 18:29 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik @ 2020-09-07 12:28 ` David Sterba 2020-09-09 7:37 ` David Sterba 0 siblings, 1 reply; 13+ messages in thread From: David Sterba @ 2020-09-07 12:28 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs, kernel-team On Thu, Sep 03, 2020 at 02:29:51PM -0400, Josef Bacik wrote: > I'm a actual human being so am incapable of converting u64 to s64 in my > head, so add a helper to get the pretty name of a root objectid and use > that helper to spit out the name for any special roots for leaked roots, > so I don't have to scratch my head and figure out which root I messed up > the refs for. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/disk-io.c | 6 ++++-- > fs/btrfs/print-tree.c | 39 +++++++++++++++++++++++++++++++++++++++ > fs/btrfs/print-tree.h | 3 +++ > 3 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 7147237d9bf0..71beb9493ab4 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1504,10 +1504,12 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) > struct btrfs_root *root; > > while (!list_empty(&fs_info->allocated_roots)) { > + char buf[BTRFS_ROOT_NAME_BUF_LEN]; > + > root = list_first_entry(&fs_info->allocated_roots, > struct btrfs_root, leak_list); > - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", > - root->root_key.objectid, root->root_key.offset, > + btrfs_err(fs_info, "leaked root %s refcount %d", > + btrfs_root_name(root->root_key.objectid, buf), > refcount_read(&root->refs)); > while (refcount_read(&root->refs) > 1) > btrfs_put_root(root); > diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c > index 80567c11ec12..d0370075a719 100644 > --- a/fs/btrfs/print-tree.c > +++ b/fs/btrfs/print-tree.c > @@ -7,6 +7,45 @@ > #include "disk-io.h" > #include "print-tree.h" > > +struct name_map { > + u64 id; > + char *name; const char* or maybe even a fixed length string, removing the _TREE suffix > +}; > + > +static const struct name_map root_map[] = { > + { BTRFS_ROOT_TREE_OBJECTID, "ROOT_TREE" }, > + { BTRFS_EXTENT_TREE_OBJECTID, "EXTENT_TREE" }, > + { BTRFS_CHUNK_TREE_OBJECTID, "CHUNK_TREE" }, > + { BTRFS_DEV_TREE_OBJECTID, "DEV_TREE" }, > + { BTRFS_FS_TREE_OBJECTID, "FS_TREE" }, > + { BTRFS_ROOT_TREE_DIR_OBJECTID, "ROOT_TREE_DIR" }, This is not a tree id > + { BTRFS_CSUM_TREE_OBJECTID, "CSUM_TREE" }, > + { BTRFS_TREE_LOG_OBJECTID, "TREE_LOG" }, > + { BTRFS_QUOTA_TREE_OBJECTID, "QUOTA_TREE" }, > + { BTRFS_UUID_TREE_OBJECTID, "UUID_TREE" }, > + { BTRFS_FREE_SPACE_TREE_OBJECTID, "FREE_SPACE_TREE" }, > + { BTRFS_DATA_RELOC_TREE_OBJECTID, "DATA_RELOC_TREE" }, I've noticed we have the id -> str mapping already in btrfs_lockdep_keysets, so there could be just one such structure and we'd add wrappers around that. Also the C99 initializers should be used. > +}; > + > +char *btrfs_root_name(u64 objectid, char *buf) > +{ > + int i; > + > + if (objectid == BTRFS_TREE_RELOC_OBJECTID) { > + snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, > + "TREE_RELOC offset=%llu", objectid); The reloc tree is an outlier here so I wonder if we'd rather have a nice pretty printer of the tree root name and deal with the oddities where required rather than inside the helper. In this case there would have to be some conditionals in btrfs_check_leaked_roots, as we can't do that in the format string. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-09-07 12:28 ` David Sterba @ 2020-09-09 7:37 ` David Sterba 0 siblings, 0 replies; 13+ messages in thread From: David Sterba @ 2020-09-09 7:37 UTC (permalink / raw) To: dsterba, Josef Bacik, linux-btrfs, kernel-team On Mon, Sep 07, 2020 at 02:28:54PM +0200, David Sterba wrote: > On Thu, Sep 03, 2020 at 02:29:51PM -0400, Josef Bacik wrote: > > --- a/fs/btrfs/print-tree.c > > +++ b/fs/btrfs/print-tree.c > > @@ -7,6 +7,45 @@ > > #include "disk-io.h" > > #include "print-tree.h" > > > > +struct name_map { > > + u64 id; > > + char *name; > > const char* or maybe even a fixed length string, removing the > _TREE suffix > > > +}; > > + > > +static const struct name_map root_map[] = { > > + { BTRFS_ROOT_TREE_OBJECTID, "ROOT_TREE" }, > > + { BTRFS_EXTENT_TREE_OBJECTID, "EXTENT_TREE" }, > > + { BTRFS_CHUNK_TREE_OBJECTID, "CHUNK_TREE" }, > > + { BTRFS_DEV_TREE_OBJECTID, "DEV_TREE" }, > > + { BTRFS_FS_TREE_OBJECTID, "FS_TREE" }, > > + { BTRFS_ROOT_TREE_DIR_OBJECTID, "ROOT_TREE_DIR" }, > > This is not a tree id > > > + { BTRFS_CSUM_TREE_OBJECTID, "CSUM_TREE" }, > > + { BTRFS_TREE_LOG_OBJECTID, "TREE_LOG" }, > > + { BTRFS_QUOTA_TREE_OBJECTID, "QUOTA_TREE" }, > > + { BTRFS_UUID_TREE_OBJECTID, "UUID_TREE" }, > > + { BTRFS_FREE_SPACE_TREE_OBJECTID, "FREE_SPACE_TREE" }, > > + { BTRFS_DATA_RELOC_TREE_OBJECTID, "DATA_RELOC_TREE" }, > > I've noticed we have the id -> str mapping already in > btrfs_lockdep_keysets, so there could be just one such structure and > we'd add wrappers around that. > > Also the C99 initializers should be used. > > > +}; > > + > > +char *btrfs_root_name(u64 objectid, char *buf) > > +{ > > + int i; > > + > > + if (objectid == BTRFS_TREE_RELOC_OBJECTID) { > > + snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, > > + "TREE_RELOC offset=%llu", objectid); > > The reloc tree is an outlier here so I wonder if we'd rather have a nice > pretty printer of the tree root name and deal with the oddities where > required rather than inside the helper. In this case there would have to > be some conditionals in btrfs_check_leaked_roots, as we can't do that in > the format string. I had another thought, we want a pretty printer, the above still holds but needs more restructuring so let's take this. I'll do the fixups and add it to misc-next. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2][v2] Some leaked root fixes @ 2020-08-20 20:00 Josef Bacik 2020-08-20 20:00 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik 0 siblings, 1 reply; 13+ messages in thread From: Josef Bacik @ 2020-08-20 20:00 UTC (permalink / raw) To: linux-btrfs, kernel-team Hello, These are two fixes for a leaked root problem I noticed a while ago. One is the actual leaked root fix, the other is to add a pretty print for the leaked root message, as figuring out that some giant %llu number was the data reloc root is not a feat meant for humans. Thanks, Josef ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-20 20:00 [PATCH 0/2][v2] Some leaked root fixes Josef Bacik @ 2020-08-20 20:00 ` Josef Bacik 2020-08-21 7:35 ` Nikolay Borisov 0 siblings, 1 reply; 13+ messages in thread From: Josef Bacik @ 2020-08-20 20:00 UTC (permalink / raw) To: linux-btrfs, kernel-team I'm a actual human being so am incapable of converting u64 to s64 in my head, so add a helper to get the pretty name of a root objectid and use that helper to spit out the name for any special roots for leaked roots, so I don't have to scratch my head and figure out which root I messed up the refs for. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/disk-io.c | 8 +++++--- fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ fs/btrfs/print-tree.h | 1 + 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index ac6d6fddd5f4..a7358e0f59de 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) struct btrfs_root *root; while (!list_empty(&fs_info->allocated_roots)) { + const char *name = btrfs_root_name(root->root_key.objectid); + root = list_first_entry(&fs_info->allocated_roots, struct btrfs_root, leak_list); - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", - root->root_key.objectid, root->root_key.offset, - refcount_read(&root->refs)); + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", + name ? name : "", root->root_key.objectid, + root->root_key.offset, refcount_read(&root->refs)); while (refcount_read(&root->refs) > 1) btrfs_put_root(root); btrfs_put_root(root); diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c index 61f44e78e3c9..c633aec8973d 100644 --- a/fs/btrfs/print-tree.c +++ b/fs/btrfs/print-tree.c @@ -7,6 +7,43 @@ #include "disk-io.h" #include "print-tree.h" +struct name_map { + u64 id; + const char *name; +}; + +static const struct name_map root_map[] = { + { BTRFS_ROOT_TREE_OBJECTID, "ROOT_TREE" }, + { BTRFS_EXTENT_TREE_OBJECTID, "EXTENT_TREE" }, + { BTRFS_CHUNK_TREE_OBJECTID, "CHUNK_TREE" }, + { BTRFS_DEV_TREE_OBJECTID, "DEV_TREE" }, + { BTRFS_FS_TREE_OBJECTID, "FS_TREE" }, + { BTRFS_ROOT_TREE_DIR_OBJECTID, "ROOT_TREE_DIR" }, + { BTRFS_CSUM_TREE_OBJECTID, "CSUM_TREE" }, + { BTRFS_TREE_LOG_OBJECTID, "TREE_LOG" }, + { BTRFS_QUOTA_TREE_OBJECTID, "QUOTA_TREE" }, + { BTRFS_TREE_RELOC_OBJECTID, "TREE_RELOC" }, + { BTRFS_UUID_TREE_OBJECTID, "UUID_TREE" }, + { BTRFS_FREE_SPACE_TREE_OBJECTID, "FREE_SPACE_TREE" }, + { BTRFS_DATA_RELOC_TREE_OBJECTID, "DATA_RELOC_TREE" }, +}; + +const char *btrfs_root_name(u64 objectid) +{ + int i; + + if (objectid >= BTRFS_FIRST_FREE_OBJECTID && + objectid <= BTRFS_LAST_FREE_OBJECTID) + return NULL; + + for (i = 0; i < ARRAY_SIZE(root_map); i++) { + if (root_map[i].id == objectid) + return root_map[i].name; + } + + return NULL; +} + static void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk) { int num_stripes = btrfs_chunk_num_stripes(eb, chunk); diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h index e6bb38fd75ad..dffdfa495297 100644 --- a/fs/btrfs/print-tree.h +++ b/fs/btrfs/print-tree.h @@ -8,5 +8,6 @@ void btrfs_print_leaf(struct extent_buffer *l); void btrfs_print_tree(struct extent_buffer *c, bool follow); +const char *btrfs_root_name(u64 objectid); #endif -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-20 20:00 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik @ 2020-08-21 7:35 ` Nikolay Borisov 2020-08-21 10:13 ` David Sterba 2020-08-24 12:46 ` David Sterba 0 siblings, 2 replies; 13+ messages in thread From: Nikolay Borisov @ 2020-08-21 7:35 UTC (permalink / raw) To: Josef Bacik, linux-btrfs, kernel-team On 20.08.20 г. 23:00 ч., Josef Bacik wrote: > I'm a actual human being so am incapable of converting u64 to s64 in my > head, so add a helper to get the pretty name of a root objectid and use > that helper to spit out the name for any special roots for leaked roots, > so I don't have to scratch my head and figure out which root I messed up > the refs for. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/disk-io.c | 8 +++++--- > fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ > fs/btrfs/print-tree.h | 1 + > 3 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index ac6d6fddd5f4..a7358e0f59de 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) > struct btrfs_root *root; > > while (!list_empty(&fs_info->allocated_roots)) { > + const char *name = btrfs_root_name(root->root_key.objectid); > + > root = list_first_entry(&fs_info->allocated_roots, > struct btrfs_root, leak_list); > - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", > - root->root_key.objectid, root->root_key.offset, > - refcount_read(&root->refs)); > + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", nit: Won't this string result in some rather awkward looking strings, such as: "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the (objectid,offset) pair be marked with parentheses? > + name ? name : "", root->root_key.objectid, > + root->root_key.offset, refcount_read(&root->refs)); > while (refcount_read(&root->refs) > 1) > btrfs_put_root(root); > btrfs_put_root(root); > diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c > index 61f44e78e3c9..c633aec8973d 100644 > --- a/fs/btrfs/print-tree.c > +++ b/fs/btrfs/print-tree.c > @@ -7,6 +7,43 @@ > #include "disk-io.h" > #include "print-tree.h" > > +struct name_map { > + u64 id; > + const char *name; > +}; > + > +static const struct name_map root_map[] = { > + { BTRFS_ROOT_TREE_OBJECTID, "ROOT_TREE" }, > + { BTRFS_EXTENT_TREE_OBJECTID, "EXTENT_TREE" }, > + { BTRFS_CHUNK_TREE_OBJECTID, "CHUNK_TREE" }, > + { BTRFS_DEV_TREE_OBJECTID, "DEV_TREE" }, > + { BTRFS_FS_TREE_OBJECTID, "FS_TREE" }, > + { BTRFS_ROOT_TREE_DIR_OBJECTID, "ROOT_TREE_DIR" }, > + { BTRFS_CSUM_TREE_OBJECTID, "CSUM_TREE" }, > + { BTRFS_TREE_LOG_OBJECTID, "TREE_LOG" }, > + { BTRFS_QUOTA_TREE_OBJECTID, "QUOTA_TREE" }, > + { BTRFS_TREE_RELOC_OBJECTID, "TREE_RELOC" }, > + { BTRFS_UUID_TREE_OBJECTID, "UUID_TREE" }, > + { BTRFS_FREE_SPACE_TREE_OBJECTID, "FREE_SPACE_TREE" }, > + { BTRFS_DATA_RELOC_TREE_OBJECTID, "DATA_RELOC_TREE" }, > +}; > + > +const char *btrfs_root_name(u64 objectid) > +{ > + int i; > + > + if (objectid >= BTRFS_FIRST_FREE_OBJECTID && > + objectid <= BTRFS_LAST_FREE_OBJECTID) > + return NULL; > + > + for (i = 0; i < ARRAY_SIZE(root_map); i++) { > + if (root_map[i].id == objectid) > + return root_map[i].name; > + } > + > + return NULL; > +} > + > static void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk) > { > int num_stripes = btrfs_chunk_num_stripes(eb, chunk); > diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h > index e6bb38fd75ad..dffdfa495297 100644 > --- a/fs/btrfs/print-tree.h > +++ b/fs/btrfs/print-tree.h > @@ -8,5 +8,6 @@ > > void btrfs_print_leaf(struct extent_buffer *l); > void btrfs_print_tree(struct extent_buffer *c, bool follow); > +const char *btrfs_root_name(u64 objectid); > > #endif > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-21 7:35 ` Nikolay Borisov @ 2020-08-21 10:13 ` David Sterba 2020-08-21 10:25 ` Nikolay Borisov 2020-08-21 14:00 ` Josef Bacik 2020-08-24 12:46 ` David Sterba 1 sibling, 2 replies; 13+ messages in thread From: David Sterba @ 2020-08-21 10:13 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Josef Bacik, linux-btrfs, kernel-team On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote: > > > On 20.08.20 г. 23:00 ч., Josef Bacik wrote: > > I'm a actual human being so am incapable of converting u64 to s64 in my > > head, so add a helper to get the pretty name of a root objectid and use > > that helper to spit out the name for any special roots for leaked roots, > > so I don't have to scratch my head and figure out which root I messed up > > the refs for. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/disk-io.c | 8 +++++--- > > fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ > > fs/btrfs/print-tree.h | 1 + > > 3 files changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index ac6d6fddd5f4..a7358e0f59de 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) > > struct btrfs_root *root; > > > > while (!list_empty(&fs_info->allocated_roots)) { > > + const char *name = btrfs_root_name(root->root_key.objectid); > > + > > root = list_first_entry(&fs_info->allocated_roots, > > struct btrfs_root, leak_list); > > - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", > > - root->root_key.objectid, root->root_key.offset, > > - refcount_read(&root->refs)); > > + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", > > nit: Won't this string result in some rather awkward looking strings, > such as: > > "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the > (objectid,offset) pair be marked with parentheses? I don't understand why need/want to print the offset here. It is from the key.offset but for a message we should print it in an understandable way. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-21 10:13 ` David Sterba @ 2020-08-21 10:25 ` Nikolay Borisov 2020-08-21 14:00 ` Josef Bacik 1 sibling, 0 replies; 13+ messages in thread From: Nikolay Borisov @ 2020-08-21 10:25 UTC (permalink / raw) To: dsterba, Josef Bacik, linux-btrfs, kernel-team On 21.08.20 г. 13:13 ч., David Sterba wrote: > On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote: >> >> >> On 20.08.20 г. 23:00 ч., Josef Bacik wrote: >>> I'm a actual human being so am incapable of converting u64 to s64 in my >>> head, so add a helper to get the pretty name of a root objectid and use >>> that helper to spit out the name for any special roots for leaked roots, >>> so I don't have to scratch my head and figure out which root I messed up >>> the refs for. >>> >>> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >>> --- >>> fs/btrfs/disk-io.c | 8 +++++--- >>> fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ >>> fs/btrfs/print-tree.h | 1 + >>> 3 files changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index ac6d6fddd5f4..a7358e0f59de 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) >>> struct btrfs_root *root; >>> >>> while (!list_empty(&fs_info->allocated_roots)) { >>> + const char *name = btrfs_root_name(root->root_key.objectid); >>> + >>> root = list_first_entry(&fs_info->allocated_roots, >>> struct btrfs_root, leak_list); >>> - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", >>> - root->root_key.objectid, root->root_key.offset, >>> - refcount_read(&root->refs)); >>> + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", >> >> nit: Won't this string result in some rather awkward looking strings, >> such as: >> >> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the >> (objectid,offset) pair be marked with parentheses? > > I don't understand why need/want to print the offset here. It is from > the key.offset but for a message we should print it in an understandable > way. According to the dev docs: btrfs_key.offset = either 0 if objectid is one of the BTRFS_*_TREE_OBJECTID defines or if a subvolume and not a snapshot, or if a snapshot the transaction id when this snapshot was created. So it doesn't have a human-readable value either ways. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-21 10:13 ` David Sterba 2020-08-21 10:25 ` Nikolay Borisov @ 2020-08-21 14:00 ` Josef Bacik 2020-08-24 11:30 ` David Sterba 1 sibling, 1 reply; 13+ messages in thread From: Josef Bacik @ 2020-08-21 14:00 UTC (permalink / raw) To: dsterba, Nikolay Borisov, linux-btrfs, kernel-team On 8/21/20 6:13 AM, David Sterba wrote: > On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote: >> >> >> On 20.08.20 г. 23:00 ч., Josef Bacik wrote: >>> I'm a actual human being so am incapable of converting u64 to s64 in my >>> head, so add a helper to get the pretty name of a root objectid and use >>> that helper to spit out the name for any special roots for leaked roots, >>> so I don't have to scratch my head and figure out which root I messed up >>> the refs for. >>> >>> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >>> --- >>> fs/btrfs/disk-io.c | 8 +++++--- >>> fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ >>> fs/btrfs/print-tree.h | 1 + >>> 3 files changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index ac6d6fddd5f4..a7358e0f59de 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) >>> struct btrfs_root *root; >>> >>> while (!list_empty(&fs_info->allocated_roots)) { >>> + const char *name = btrfs_root_name(root->root_key.objectid); >>> + >>> root = list_first_entry(&fs_info->allocated_roots, >>> struct btrfs_root, leak_list); >>> - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", >>> - root->root_key.objectid, root->root_key.offset, >>> - refcount_read(&root->refs)); >>> + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", >> >> nit: Won't this string result in some rather awkward looking strings, >> such as: >> >> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the >> (objectid,offset) pair be marked with parentheses? > > I don't understand why need/want to print the offset here. It is from > the key.offset but for a message we should print it in an understandable > way. > The offset matters for the TREE_RELOC roots, because it's the object id of the fs root that the reloc root refers to. Thanks, Josef ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-21 14:00 ` Josef Bacik @ 2020-08-24 11:30 ` David Sterba 0 siblings, 0 replies; 13+ messages in thread From: David Sterba @ 2020-08-24 11:30 UTC (permalink / raw) To: Josef Bacik; +Cc: dsterba, Nikolay Borisov, linux-btrfs, kernel-team On Fri, Aug 21, 2020 at 10:00:35AM -0400, Josef Bacik wrote: > On 8/21/20 6:13 AM, David Sterba wrote: > > On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote: > >>> root = list_first_entry(&fs_info->allocated_roots, > >>> struct btrfs_root, leak_list); > >>> - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", > >>> - root->root_key.objectid, root->root_key.offset, > >>> - refcount_read(&root->refs)); > >>> + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", > >> > >> nit: Won't this string result in some rather awkward looking strings, > >> such as: > >> > >> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the > >> (objectid,offset) pair be marked with parentheses? > > > > I don't understand why need/want to print the offset here. It is from > > the key.offset but for a message we should print it in an understandable > > way. > > The offset matters for the TREE_RELOC roots, because it's the object id > of the fs root that the reloc root refers to. Thanks, Ok, that makes sense to print, though printing it as offset=... looks confusing. Could it be rephrased to refer to the fs tree and reloc tree? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] btrfs: pretty print leaked root name 2020-08-21 7:35 ` Nikolay Borisov 2020-08-21 10:13 ` David Sterba @ 2020-08-24 12:46 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: David Sterba @ 2020-08-24 12:46 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Josef Bacik, linux-btrfs, kernel-team On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote: > > > On 20.08.20 г. 23:00 ч., Josef Bacik wrote: > > I'm a actual human being so am incapable of converting u64 to s64 in my > > head, so add a helper to get the pretty name of a root objectid and use > > that helper to spit out the name for any special roots for leaked roots, > > so I don't have to scratch my head and figure out which root I messed up > > the refs for. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/disk-io.c | 8 +++++--- > > fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++ > > fs/btrfs/print-tree.h | 1 + > > 3 files changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index ac6d6fddd5f4..a7358e0f59de 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info) > > struct btrfs_root *root; > > > > while (!list_empty(&fs_info->allocated_roots)) { > > + const char *name = btrfs_root_name(root->root_key.objectid); > > + > > root = list_first_entry(&fs_info->allocated_roots, > > struct btrfs_root, leak_list); > > - btrfs_err(fs_info, "leaked root %llu-%llu refcount %d", > > - root->root_key.objectid, root->root_key.offset, > > - refcount_read(&root->refs)); > > + btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d", > > nit: Won't this string result in some rather awkward looking strings, > such as: > > "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the > (objectid,offset) pair be marked with parentheses? We need to print either string name or value, and in one go. Unlike in progs, I'd rather avoid split stings in kernel and using pr_cont/KERN_CONT. The option is see is to supply a buffer to the pretty printer that would be used in case we need the numerical id. Like: while (allocated_roots) { char root_name[24]; ... btrfs_err(..., "leaked root %s ...", get_root_name(id, root_name)); } where char *get_root_name(u64 id, char *buf) { if (numeric root) { sprintf(buf, "%llu", ud); return buf; } return <root name from the table>; } ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-09 7:38 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-03 18:29 [PATCH 0/2][v3] Some leaked root fixes Josef Bacik 2020-09-03 18:29 ` [PATCH 1/2] btrfs: free fs roots on failed mount Josef Bacik 2020-09-07 12:50 ` David Sterba 2020-09-03 18:29 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik 2020-09-07 12:28 ` David Sterba 2020-09-09 7:37 ` David Sterba -- strict thread matches above, loose matches on Subject: below -- 2020-08-20 20:00 [PATCH 0/2][v2] Some leaked root fixes Josef Bacik 2020-08-20 20:00 ` [PATCH 2/2] btrfs: pretty print leaked root name Josef Bacik 2020-08-21 7:35 ` Nikolay Borisov 2020-08-21 10:13 ` David Sterba 2020-08-21 10:25 ` Nikolay Borisov 2020-08-21 14:00 ` Josef Bacik 2020-08-24 11:30 ` David Sterba 2020-08-24 12:46 ` 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).