| Message ID | 461693e5c015857e684878e99e5e65075bb97c13.1597953516.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some leaked root fixes | expand |
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 >
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.
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. >
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
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?
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>; }
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
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(-)