On Sat, Nov 12, 2016 at 12:59:59AM +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.
>
> Also, display unknown flags as hex.
>
> Signed-off-by: Adam Borowski <[email protected]>
> ---
> fs/btrfs/relocation.c | 41 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index c4af0cd..57d867d 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4333,6 +4333,43 @@ static struct reloc_control
> *alloc_reloc_control(struct btrfs_fs_info *fs_info)
> }
>
> /*
> + * printk the block group being relocated
> + */
> +static void describe_relocation(struct btrfs_fs_info *fs_info,
> + struct btrfs_block_group_cache *block_group)
> +{
> + char buf[128]; // prefixed by a '|' that'll be dropped
Oh right, moving the buffer to the function is the right way.
As my main objection is addressed, we can proceed to the codinstyle.
Please don't use the // comments.
> + u64 flags = block_group->flags;
> +
> + if (unlikely(!flags)) // shouldn't happen
add explicit { ... } if there's an multi-statement else block, drop
'unlikely'
> + strcpy(buf, "|NONE");
> + else {
> + char *bp = buf;
newline between declarations and code
> +#define DESCRIBE_FLAG(f, d) \
> + if (flags & BTRFS_BLOCK_GROUP_##f) { \
> + bp += snprintf(bp, buf - bp + sizeof(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 (unlikely(flags))
drop 'unlikely'
> + snprintf(buf, buf - bp + sizeof(buf), "|0x%llx", flags);
> +#undef DESCRIBE_FLAG
> + }
> +
> + btrfs_info(fs_info,
> + "relocating block group %llu flags %s",
> + block_group->key.objectid, buf+1);
block_group->key.objectid, buf + 1);
space around a binary operator
> +}
> +
> +/*
> * function to relocate all extents in a block group.
> */
> int btrfs_relocate_block_group(struct btrfs_root *extent_root, u64
> group_start)
> @@ -4388,9 +4425,7 @@ int btrfs_relocate_block_group(struct btrfs_root
> *extent_root, u64 group_start)
> goto out;
> }
>
> - btrfs_info(extent_root->fs_info,
> - "relocating block group %llu flags %llu",
> - rc->block_group->key.objectid, rc->block_group->flags);
> + describe_relocation(extent_root->fs_info, rc->block_group);
>
> btrfs_wait_block_group_reservations(rc->block_group);
> btrfs_wait_nocow_writers(rc->block_group);
> --
> 2.10.2
>
> This is a version that uses a temp buffer on the stack, but does it in a
> separate function so it doesn't cost us anything when deep call chains are
> involved. While balance that can trigger deep call chain, it's not called
> deeply itself.
>
> This approach is simpler than mucking with allocs and avoids code
> duplication that would be needed for handling failed alloc.
Agreed.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
