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

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 6/15/23 21:44, Ilya Dryomov wrote:
> On Thu, Jun 15, 2023 at 2:57 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
> > 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 ?
> pr_info() and friends are used throughout the kernel, so the name is
> very recognizable.  We also have a lot fewer of them compared to douts,
> so I would stick with pr_info_client() (i.e. no shorthands).

Okay. Thanks

- Xiubo


> Thanks,
>
>                  Ilya