| Message ID | 20180521063745.29343-2-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 21, 2018 at 02:37:43PM +0800, Anand Jain wrote: > Improve on describe_relocation() add a common helper function to describe > the block groups. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v3: Born. > > fs/btrfs/relocation.c | 30 +++--------------------------- > fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > fs/btrfs/volumes.h | 1 + > 3 files changed, 48 insertions(+), 27 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 879b76fa881a..b9b32b0e4ba4 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void) > 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 */ > - u64 flags = block_group->flags; > + char buf[128]; > > - /* Shouldn't happen */ > - if (!flags) { > - strcpy(buf, "|NONE"); > - } else { > - char *bp = buf; > - > -#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 (flags) > - snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags); > -#undef DESCRIBE_FLAG > - } > + describe_block_groups(block_group->flags, buf, sizeof(buf)); > > btrfs_info(fs_info, > "relocating block group %llu flags %s", > - block_group->key.objectid, buf + 1); > + block_group->key.objectid, buf); > } > > /* > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 43bd65395106..02b1d14f510d 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -126,6 +126,50 @@ const char *get_raid_name(enum btrfs_raid_types type) > return btrfs_raid_array[type].raid_name; > } > > +u32 describe_block_groups(u64 bg_flags, char *describe, u32 sz) > +{ > + int i; > + u32 ret; > + char *dp = describe; > + u64 flags = bg_flags; > + > + if (!flags) > + return snprintf(dp, sz, "%s", "NONE"); > + > +#define DESCRIBE_FLAG(f, d) \ > + do { \ > + if (flags & BTRFS_BLOCK_GROUP_##f) { \ > + dp += snprintf(dp, describe - dp + sz, "%s|", d); \ > + flags &= ~BTRFS_BLOCK_GROUP_##f; \ > + } \ > + } while (0) > + DESCRIBE_FLAG(DATA, "data"); > + DESCRIBE_FLAG(SYSTEM, "system"); > + DESCRIBE_FLAG(METADATA, "metadata"); > +#undef DESCRIBE_FLAG Adding the convenience macro for 3 uses and then opencoding it 2 times does not make much sense to me. Why don't you change that to use the 1st argument as the exact flag to mask out and the 2nd argument as the string to print? > + > + if (flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) { > + dp += snprintf(dp, describe - dp + sz, > + "%s|", "single"); > + flags &= ~BTRFS_AVAIL_ALLOC_BIT_SINGLE; > + } > + > + for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) { And this would work inside the loop too. > + if (flags & btrfs_raid_array[i].bg_flag) { > + dp += snprintf(dp, describe - dp + sz, > + "%s|", btrfs_raid_array[i].raid_name); > + flags &= ~btrfs_raid_array[i].bg_flag; > + } > + } > + > + if (flags) > + dp += snprintf(dp, describe - dp + sz, "0x%llx|", flags); > + > + ret = dp - describe - 1; > + describe[ret] = '\0'; /* remove last | */ > + return ret; > +} > + > static int init_first_rw_device(struct btrfs_trans_handle *trans, > struct btrfs_fs_info *fs_info); > static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info); > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 5139ec8daf4c..52ac258daa17 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -433,6 +433,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path); > int btrfs_balance(struct btrfs_fs_info *fs_info, > struct btrfs_balance_control *bctl, > struct btrfs_ioctl_balance_args *bargs); > +u32 describe_block_groups(u64 flags, char *describe, u32 sz); Please prefix an exported function with btrfs_ and don't use the overly abbreviated variable names like 'sz'. > int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info); > int btrfs_recover_balance(struct btrfs_fs_info *fs_info); > int btrfs_pause_balance(struct btrfs_fs_info *fs_info); > -- > 2.7.0 > > -- > 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 05/22/2018 08:26 PM, David Sterba wrote: > On Mon, May 21, 2018 at 02:37:43PM +0800, Anand Jain wrote: >> Improve on describe_relocation() add a common helper function to describe >> the block groups. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v3: Born. >> >> fs/btrfs/relocation.c | 30 +++--------------------------- >> fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> fs/btrfs/volumes.h | 1 + >> 3 files changed, 48 insertions(+), 27 deletions(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 879b76fa881a..b9b32b0e4ba4 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void) >> 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 */ >> - u64 flags = block_group->flags; >> + char buf[128]; >> >> - /* Shouldn't happen */ >> - if (!flags) { >> - strcpy(buf, "|NONE"); >> - } else { >> - char *bp = buf; >> - >> -#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 (flags) >> - snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags); >> -#undef DESCRIBE_FLAG >> - } >> + describe_block_groups(block_group->flags, buf, sizeof(buf)); >> >> btrfs_info(fs_info, >> "relocating block group %llu flags %s", >> - block_group->key.objectid, buf + 1); >> + block_group->key.objectid, buf); >> } >> >> /* >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 43bd65395106..02b1d14f510d 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -126,6 +126,50 @@ const char *get_raid_name(enum btrfs_raid_types type) >> return btrfs_raid_array[type].raid_name; >> } >> >> +u32 describe_block_groups(u64 bg_flags, char *describe, u32 sz) >> +{ >> + int i; >> + u32 ret; >> + char *dp = describe; >> + u64 flags = bg_flags; >> + >> + if (!flags) >> + return snprintf(dp, sz, "%s", "NONE"); >> + >> +#define DESCRIBE_FLAG(f, d) \ >> + do { \ >> + if (flags & BTRFS_BLOCK_GROUP_##f) { \ >> + dp += snprintf(dp, describe - dp + sz, "%s|", d); \ >> + flags &= ~BTRFS_BLOCK_GROUP_##f; \ >> + } \ >> + } while (0) >> + DESCRIBE_FLAG(DATA, "data"); >> + DESCRIBE_FLAG(SYSTEM, "system"); >> + DESCRIBE_FLAG(METADATA, "metadata"); >> +#undef DESCRIBE_FLAG > > Adding the convenience macro for 3 uses and then opencoding it 2 times > does not make much sense to me. Why don't you change that to use the 1st > argument as the exact flag to mask out and the 2nd argument as the > string to print? >> + >> + if (flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) { >> + dp += snprintf(dp, describe - dp + sz, >> + "%s|", "single"); >> + flags &= ~BTRFS_AVAIL_ALLOC_BIT_SINGLE; >> + } >> + >> + for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) { > > And this would work inside the loop too. fixed it. >> + if (flags & btrfs_raid_array[i].bg_flag) { >> + dp += snprintf(dp, describe - dp + sz, >> + "%s|", btrfs_raid_array[i].raid_name); >> + flags &= ~btrfs_raid_array[i].bg_flag; >> + } >> + } >> + >> + if (flags) >> + dp += snprintf(dp, describe - dp + sz, "0x%llx|", flags); >> + >> + ret = dp - describe - 1; >> + describe[ret] = '\0'; /* remove last | */ >> + return ret; >> +} >> + >> static int init_first_rw_device(struct btrfs_trans_handle *trans, >> struct btrfs_fs_info *fs_info); >> static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info); >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >> index 5139ec8daf4c..52ac258daa17 100644 >> --- a/fs/btrfs/volumes.h >> +++ b/fs/btrfs/volumes.h >> @@ -433,6 +433,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path); >> int btrfs_balance(struct btrfs_fs_info *fs_info, >> struct btrfs_balance_control *bctl, >> struct btrfs_ioctl_balance_args *bargs); >> +u32 describe_block_groups(u64 flags, char *describe, u32 sz); > > Please prefix an exported function with btrfs_ and don't use the overly > abbreviated variable names like 'sz'. Yes. Also I have dropped the return u32 to void in V4 in the ML. Thanks, Anand >> int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info); >> int btrfs_recover_balance(struct btrfs_fs_info *fs_info); >> int btrfs_pause_balance(struct btrfs_fs_info *fs_info); >> -- >> 2.7.0 >> >> -- >> 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 > -- 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
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 879b76fa881a..b9b32b0e4ba4 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void) 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 */ - u64 flags = block_group->flags; + char buf[128]; - /* Shouldn't happen */ - if (!flags) { - strcpy(buf, "|NONE"); - } else { - char *bp = buf; - -#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 (flags) - snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags); -#undef DESCRIBE_FLAG - } + describe_block_groups(block_group->flags, buf, sizeof(buf)); btrfs_info(fs_info, "relocating block group %llu flags %s", - block_group->key.objectid, buf + 1); + block_group->key.objectid, buf); } /* diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 43bd65395106..02b1d14f510d 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -126,6 +126,50 @@ const char *get_raid_name(enum btrfs_raid_types type) return btrfs_raid_array[type].raid_name; } +u32 describe_block_groups(u64 bg_flags, char *describe, u32 sz) +{ + int i; + u32 ret; + char *dp = describe; + u64 flags = bg_flags; + + if (!flags) + return snprintf(dp, sz, "%s", "NONE"); + +#define DESCRIBE_FLAG(f, d) \ + do { \ + if (flags & BTRFS_BLOCK_GROUP_##f) { \ + dp += snprintf(dp, describe - dp + sz, "%s|", d); \ + flags &= ~BTRFS_BLOCK_GROUP_##f; \ + } \ + } while (0) + DESCRIBE_FLAG(DATA, "data"); + DESCRIBE_FLAG(SYSTEM, "system"); + DESCRIBE_FLAG(METADATA, "metadata"); +#undef DESCRIBE_FLAG + + if (flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) { + dp += snprintf(dp, describe - dp + sz, + "%s|", "single"); + flags &= ~BTRFS_AVAIL_ALLOC_BIT_SINGLE; + } + + for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) { + if (flags & btrfs_raid_array[i].bg_flag) { + dp += snprintf(dp, describe - dp + sz, + "%s|", btrfs_raid_array[i].raid_name); + flags &= ~btrfs_raid_array[i].bg_flag; + } + } + + if (flags) + dp += snprintf(dp, describe - dp + sz, "0x%llx|", flags); + + ret = dp - describe - 1; + describe[ret] = '\0'; /* remove last | */ + return ret; +} + static int init_first_rw_device(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info); static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 5139ec8daf4c..52ac258daa17 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -433,6 +433,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path); int btrfs_balance(struct btrfs_fs_info *fs_info, struct btrfs_balance_control *bctl, struct btrfs_ioctl_balance_args *bargs); +u32 describe_block_groups(u64 flags, char *describe, u32 sz); int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info); int btrfs_recover_balance(struct btrfs_fs_info *fs_info); int btrfs_pause_balance(struct btrfs_fs_info *fs_info);
Improve on describe_relocation() add a common helper function to describe the block groups. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v3: Born. fs/btrfs/relocation.c | 30 +++--------------------------- fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ fs/btrfs/volumes.h | 1 + 3 files changed, 48 insertions(+), 27 deletions(-)