| Message ID | 20161104072654.13411-1-kilobyte@angband.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 11/04/16 08:26, Adam Borowski wrote: > They're not even documented anywhere, letting users with no recourse but > to RTFS. It's no big burden to output the bitfield as words. > > Also, display unknown flags as hex. > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> Very helpful and works (for me) as advertised. Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com> > --- > fs/btrfs/relocation.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 0ec8ffa..388216f 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4326,6 +4326,34 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) > } > > /* > + * explain bit flags, prefixed by a '|' that'll be dropped > + */ > +static void describe_block_group_flags(char *buf, u64 flags) > +{ > + if (!flags) > + *buf += sprintf(buf, "|NONE"); > + else { > +#define DESCRIBE_FLAG(f, d) \ > + if (flags & BTRFS_BLOCK_GROUP_##f) { \ > + buf += sprintf(buf, "|%s", d); \ > + flags &= ~BTRFS_BLOCK_GROUP_##f; \ > + } > + DESCRIBE_FLAG(DATA, "data"); > + DESCRIBE_FLAG(SYSTEM, "system"); > + DESCRIBE_FLAG(METADATA, "metadata"); > + DESCRIBE_FLAG(RAID0, "raid0"); > + DESCRIBE_FLAG(RAID1, "raid1"); > + DESCRIBE_FLAG(DUP, "dup"); > + DESCRIBE_FLAG(RAID10, "raid10"); > + DESCRIBE_FLAG(RAID5, "raid5"); > + DESCRIBE_FLAG(RAID6, "raid6"); > + if (flags) > + buf += sprintf(buf, "|0x%llx", flags); > + } > + *buf = 0; > +} > + > +/* > * function to relocate all extents in a block group. > */ > int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > @@ -4337,6 +4365,7 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > int ret; > int rw = 0; > int err = 0; > + char flags_str[128]; > > rc = alloc_reloc_control(fs_info); > if (!rc) > @@ -4381,9 +4410,10 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > goto out; > } > > + describe_block_group_flags(flags_str, rc->block_group->flags); > btrfs_info(extent_root->fs_info, > - "relocating block group %llu flags %llu", > - rc->block_group->key.objectid, rc->block_group->flags); > + "relocating block group %llu flags %s", > + rc->block_group->key.objectid, flags_str+1); > > btrfs_wait_block_group_reservations(rc->block_group); > btrfs_wait_nocow_writers(rc->block_group); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 08:26:54AM +0100, Adam Borowski wrote: > They're not even documented anywhere, letting users with no recourse but > to RTFS. It's no big burden to output the bitfield as words. Ok. Below are some comments (and style nitpicks). > Also, display unknown flags as hex. > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > fs/btrfs/relocation.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 0ec8ffa..388216f 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4326,6 +4326,34 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) > } > > /* > + * explain bit flags, prefixed by a '|' that'll be dropped > + */ > +static void describe_block_group_flags(char *buf, u64 flags) > +{ > + if (!flags) if (!flags) { > + *buf += sprintf(buf, "|NONE"); Should it be just 'buf'? Otherwise this stores 5 after 'E', which gets overwritten by 0 at the end but still. Unless I'm missing something. } else { > + else { > +#define DESCRIBE_FLAG(f, d) \ > + if (flags & BTRFS_BLOCK_GROUP_##f) { \ > + buf += sprintf(buf, "|%s", d); \ > + flags &= ~BTRFS_BLOCK_GROUP_##f; \ > + } > + DESCRIBE_FLAG(DATA, "data"); > + DESCRIBE_FLAG(SYSTEM, "system"); > + DESCRIBE_FLAG(METADATA, "metadata"); > + DESCRIBE_FLAG(RAID0, "raid0"); > + DESCRIBE_FLAG(RAID1, "raid1"); > + DESCRIBE_FLAG(DUP, "dup"); > + DESCRIBE_FLAG(RAID10, "raid10"); > + DESCRIBE_FLAG(RAID5, "raid5"); > + DESCRIBE_FLAG(RAID6, "raid6"); An #undef should go here. > + if (flags) > + buf += sprintf(buf, "|0x%llx", flags); > + } > + *buf = 0; > +} > + > +/* > * function to relocate all extents in a block group. > */ > int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > @@ -4337,6 +4365,7 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > int ret; > int rw = 0; > int err = 0; > + char flags_str[128]; This could be a problem. The relocation can trigger deep call chains when doing IO, that need stack. I'd rather avoid the static buffer, please switch it to kmalloc. As this is not a critical allocation, the fallback would be to print just the raw value as now. Also, please use snprintf. This would need extra variable to track number of already printed chars, but should be easy, schematically like: remaining = BUFFERSIZE; ret = snprintf(buf, remaining, "..."); remaining -= ret; buf += ret; > > rc = alloc_reloc_control(fs_info); > if (!rc) > @@ -4381,9 +4410,10 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) > goto out; > } > > + describe_block_group_flags(flags_str, rc->block_group->flags); > btrfs_info(extent_root->fs_info, > - "relocating block group %llu flags %llu", > - rc->block_group->key.objectid, rc->block_group->flags); > + "relocating block group %llu flags %s", > + rc->block_group->key.objectid, flags_str+1); > > btrfs_wait_block_group_reservations(rc->block_group); > btrfs_wait_nocow_writers(rc->block_group); > -- > 2.10.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 07, 2016 at 05:58:41PM +0100, David Sterba wrote: > On Fri, Nov 04, 2016 at 08:26:54AM +0100, Adam Borowski wrote: > > They're not even documented anywhere, letting users with no recourse but > > to RTFS. It's no big burden to output the bitfield as words. > > Ok. Below are some comments (and style nitpicks). [...] > > + char flags_str[128]; > > This could be a problem. The relocation can trigger deep call chains > when doing IO, that need stack. I'd rather avoid the static buffer, > please switch it to kmalloc. Are the chains callers or callees of relocation? I believe exclusively the latter, as balance is a long process that wouldn't make sense inside a deep chain -- am I right? In that case, moving the printk to the display function would localize the buffer, avoiding all complications of an alloc. (v2 does alloc) > As this is not a critical allocation, the fallback would be to print just > the raw value as now. If the kernel is that low on memory, is it reasonable to print an unimportant message? As I see, lots of btrfs code would just fall over and die horribly, with BUG_ON or aborting; perhaps skipping the message would be safer? (v2 has a fallback message) > Also, please use snprintf. This would need extra variable to track > number of already printed chars, but should be easy M'kay. Even with all flags on (most are mutually exclusive) we won't get anywhere close to 128 characters, but paranoia can't hurt. Meow!
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 0ec8ffa..388216f 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4326,6 +4326,34 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) } /* + * explain bit flags, prefixed by a '|' that'll be dropped + */ +static void describe_block_group_flags(char *buf, u64 flags) +{ + if (!flags) + *buf += sprintf(buf, "|NONE"); + else { +#define DESCRIBE_FLAG(f, d) \ + if (flags & BTRFS_BLOCK_GROUP_##f) { \ + buf += sprintf(buf, "|%s", d); \ + flags &= ~BTRFS_BLOCK_GROUP_##f; \ + } + DESCRIBE_FLAG(DATA, "data"); + DESCRIBE_FLAG(SYSTEM, "system"); + DESCRIBE_FLAG(METADATA, "metadata"); + DESCRIBE_FLAG(RAID0, "raid0"); + DESCRIBE_FLAG(RAID1, "raid1"); + DESCRIBE_FLAG(DUP, "dup"); + DESCRIBE_FLAG(RAID10, "raid10"); + DESCRIBE_FLAG(RAID5, "raid5"); + DESCRIBE_FLAG(RAID6, "raid6"); + if (flags) + buf += sprintf(buf, "|0x%llx", flags); + } + *buf = 0; +} + +/* * function to relocate all extents in a block group. */ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) @@ -4337,6 +4365,7 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) int ret; int rw = 0; int err = 0; + char flags_str[128]; rc = alloc_reloc_control(fs_info); if (!rc) @@ -4381,9 +4410,10 @@ int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64 group_start) goto out; } + describe_block_group_flags(flags_str, rc->block_group->flags); btrfs_info(extent_root->fs_info, - "relocating block group %llu flags %llu", - rc->block_group->key.objectid, rc->block_group->flags); + "relocating block group %llu flags %s", + rc->block_group->key.objectid, flags_str+1); btrfs_wait_block_group_reservations(rc->block_group); btrfs_wait_nocow_writers(rc->block_group);
They're not even documented anywhere, letting users with no recourse but to RTFS. It's no big burden to output the bitfield as words. Also, display unknown flags as hex. Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- fs/btrfs/relocation.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)