On Fri, Feb 12, 2021 at 2:37 PM Andreas Dilger <adilger@xxxxxxxxx> wrote: > > On Feb 9, 2021, at 1:28 PM, Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx> wrote: > > > > This patch adds a new file "mb_structs_summary" which allows us to see the > > summary of the new allocator structures added in this series. > > Hmm, it is hard to visualize what these files will look like, could you > please include an example output in the commit message? It looks like > they will no longer have one line per group, which is good, but maybe > one line per order? Sure, will do that in the next version. > > If at all possible, it makes sense to have well-formatted output that > follows YAML formatting (https://yaml-online-parser.appspot.com/ can > verify this) so that it can be easily parsed (both as YAML and via > awk or other text processing tools). That doesn't mean you need to > embed a YAML parser, just a few well-placed ':' and spaces... Yeah I like the idea. YAML sounds good to me! > > Unfortunately, files like "mb_groups" were created before that wisdom > was learned, and are a bit of a nightmare to parse today. > > A few comments inline... > > > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx> > > > > +static int ext4_mb_seq_structs_summary_show(struct seq_file *seq, void *v) > > +{ > > + > > Extra blank line here can be removed Ack > > > + if (position >= MB_NUM_ORDERS(sb)) { > > + seq_puts(seq, "Tree\n"); > > Prefer not to use capitalized words. > > This should have a ':' like "tree:", but this still leaves the question > "what is the tree for?" so using "fragment_size_tree:" or similar would > be better. Ack > > > + n = rb_first(&sbi->s_mb_avg_fragment_size_root); > > + if (!n) { > > + seq_puts(seq, "<Empty>\n"); > > I'm guessing this won't happen very often, but it might be easier if it > kept the same output format, so "min: 0, max: 0, num_nodes: 0", or just > initialize those values and then skip the intermediate processing below > before printing out the summary line (better because there is only one > place that is formatting the output, so it will be consistent)? Sounds good! > > > + return 0; > > + } > > + grp = rb_entry(n, struct ext4_group_info, bb_avg_fragment_size_rb); > > + min = grp->bb_fragments ? grp->bb_free / grp->bb_fragments : 0; > > + count = 1; > > + while (rb_next(n)) { > > + count++; > > + n = rb_next(n); > > + } > > + grp = rb_entry(n, struct ext4_group_info, bb_avg_fragment_size_rb); > > + max = grp->bb_fragments ? grp->bb_free / grp->bb_fragments : 0; > > + > > + seq_printf(seq, "Min: %d, Max: %d, Num Nodes: %d\n", > > These should be "%u" and not "%d"? I'd assume none will ever be negative. Ack > > Prefer not to have spaces within keys, so that it is possible to use > e.g. 'awk /field:/ { print $2 }' to extract a value. "num_nodes:" or > "tree_nodes: is better. To be a subset of "tree:" they should be > indented with 4 spaces or a tab: > > fragment_size_tree: > tree_min: nnn > tree_max: mmm > tree_nodes: ooo Ack > > > > + if (position == 0) > > + seq_puts(seq, "Largest Free Order Lists:\n"); > > Similarly, avoiding spaces in the key makes this easier to parse, > like "max_free_order_lists:" or similar. > > > + seq_printf(seq, "Order %ld list: ", position); > > Here, " list_order_%u: %u groups\n" would be more clear, and > can be printed in a single call instead of being split up. Ack Sounds good, thanks for the feedback. I'll incorporate these changes in the next version. Thanks, Harshad > > > Cheers, Andreas > > > > >
