Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210209202857.4185846-1-harshadshirwadkar@gmail.com>
 <20210209202857.4185846-6-harshadshirwadkar@gmail.com> <95D11120-F8E4-48DC-B149-1B2A5804E74D@dilger.ca>
In-Reply-To: <95D11120-F8E4-48DC-B149-1B2A5804E74D@dilger.ca>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Tue, 16 Feb 2021 08:55:03 -0800
Message-ID: <CAD+ocbyJ8JTX_42f8=48iGtZ3O0OpwRdUSF3oSnsxpgYysNLVQ@mail.gmail.com>
Subject: Re: [PATCH v2 5/5] ext4: add proc files to monitor new structures
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        "Theodore Ts'o" <tytso@mit.edu>,
        Alex Zhuravlev <bzzz@whamcloud.com>,
        =?UTF-8?B?0JHQu9Cw0LPQvtC00LDRgNC10L3QutC+INCQ0YDRgtGR0Lw=?= 
        <artem.blagodarenko@gmail.com>, Shuichi Ihara <sihara@ddn.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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