linux-ext4 - Re: [PATCH v2 5/5] ext4: add proc files to monitor new structures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Feb 2021 08:55:03 -0800
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Alex Zhuravlev <bzzz@...mcloud.com>,
        Благодаренко Артём 
        <artem.blagodarenko@...il.com>, Shuichi Ihara <sihara@....com>
Subject: Re: [PATCH v2 5/5] ext4: add proc files to monitor new structures

On Fri, Feb 12, 2021 at 2:37 PM Andreas Dilger <adilger@...ger.ca> wrote:
>
> On Feb 9, 2021, at 1:28 PM, Harshad Shirwadkar <harshadshirwadkar@...il.com> 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@...il.com>
> >
> > +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
>
>
>
>
>