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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Thanks,

                Ilya




[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux