Re: [PATCH v3 1/6] ceph: add the *_client debug macros support

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 6/15/23 20:50, Ilya Dryomov wrote:
> On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
> > On 6/14/23 16:21, Ilya Dryomov wrote:
> > > On Wed, Jun 14, 2023 at 3:33 AM <xiubli@xxxxxxxxxx> wrote:
> > > > From: Xiubo Li <xiubli@xxxxxxxxxx>
> > > >
> > > > This will help print the fsid and client's global_id in debug logs,
> > > > and also print the function names.
> > > >
> > > > URL: https://tracker.ceph.com/issues/61590
> > > > Cc: Patrick Donnelly <pdonnell@xxxxxxxxxx>
> > > > Reviewed-by: Patrick Donnelly <pdonnell@xxxxxxxxxx>
> > > > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > > > ---
> > > >    include/linux/ceph/ceph_debug.h | 44 ++++++++++++++++++++++++++++++++-
> > > >    1 file changed, 43 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/ceph/ceph_debug.h b/include/linux/ceph/ceph_debug.h
> > > > index d5a5da838caf..26b9212bf359 100644
> > > > --- a/include/linux/ceph/ceph_debug.h
> > > > +++ b/include/linux/ceph/ceph_debug.h
> > > > @@ -19,12 +19,22 @@
> > > >           pr_debug("%.*s %12.12s:%-4d : " fmt,                            \
> > > >                    8 - (int)sizeof(KBUILD_MODNAME), "    ",               \
> > > >                    kbasename(__FILE__), __LINE__, ##__VA_ARGS__)
> > > > +#  define dout_client(client, fmt, ...)                                        \
> > > > +       pr_debug("%.*s %12.12s:%-4d : [%pU %lld] " fmt,                 \
> > > > +                8 - (int)sizeof(KBUILD_MODNAME), "    ",               \
> > > > +                kbasename(__FILE__), __LINE__,                         \
> > > > +                &client->fsid, client->monc.auth->global_id,           \
> > > > +                ##__VA_ARGS__)
> > > >    # else
> > > >    /* faux printk call just to see any compiler warnings. */
> > > >    #  define dout(fmt, ...)       do {                            \
> > > >                   if (0)                                          \
> > > >                           printk(KERN_DEBUG fmt, ##__VA_ARGS__);  \
> > > >           } while (0)
> > > > +#  define dout_client(client, fmt, ...)        do {                    \
> > > > +               if (0)                                          \
> > > > +                       printk(KERN_DEBUG fmt, ##__VA_ARGS__);  \
> > > > +       } while (0)
> > > >    # endif
> > > >
> > > >    #else
> > > > @@ -33,7 +43,39 @@
> > > >     * or, just wrap pr_debug
> > > >     */
> > > >    # define dout(fmt, ...)        pr_debug(" " fmt, ##__VA_ARGS__)
> > > > -
> > > > +# define dout_client(client, fmt, ...)                                 \
> > > > +       pr_debug("[%pU %lld] %s: " fmt, &client->fsid,                  \
> > > > +                client->monc.auth->global_id, __func__,                \
> > > > +                ##__VA_ARGS__)
> > > >    #endif
> > > >
> > > > +# define pr_notice_client(client, fmt, ...)                            \
> > > > +       pr_notice("[%pU %lld] %s: " fmt, &client->fsid,                 \
> > > > +                 client->monc.auth->global_id, __func__,               \
> > > > +                 ##__VA_ARGS__)
> > > Hi Xiubo,
> > >
> > > We definitely don't want the framework to include function names in
> > > user-facing messages (i.e. in pr_* messages).  In the example that
> > > spawned this series ("ceph: mds3 session blocklisted"), it's really
> > > irrelevant to the user which function happens to detect blocklisting.
> > >
> > > It's a bit less clear-cut for dout() messages, but honestly I don't
> > > think it's needed there either.  I know that we include it manually in
> > > many places but most of the time it's actually redundant.
> > The function name will include the most info needed in the log messages,
> > before almost all the log messages will add that explicitly or will add
> > one function name directly. Which may make the length of the 'fmt'
> > string exceeded 80 chars.
> >
> > If this doesn't make sense I will remove this from the framework.
> I'm fine with keeping it for dout() messages.  To further help with
> line lengths, how about naming the new macro doutc()?  Since it takes
> an extra argument before the format string, it feels distinctive enough
> despite it being just a single character.

What about the others macros ? Such as for the 'pr_info_client()',etc ?

Thanks

- Xiubo


> Thanks,
>
>                  Ilya