| Message ID | 20230614013025.291314-2-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: print the client global id for debug logs | expand |
On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > 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@redhat.com> > Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > 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. Thanks, Ilya
On 6/14/23 16:21, Ilya Dryomov wrote: > On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> 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@redhat.com> >> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> 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. Thanks - Xiubo > Thanks, > > Ilya >
On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: > > > On 6/14/23 16:21, Ilya Dryomov wrote: > > On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: > >> From: Xiubo Li <xiubli@redhat.com> > >> > >> 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@redhat.com> > >> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> > >> Signed-off-by: Xiubo Li <xiubli@redhat.com> > >> --- > >> 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
On 6/15/23 20:50, Ilya Dryomov wrote: > On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: >> >> On 6/14/23 16:21, Ilya Dryomov wrote: >>> On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: >>>> From: Xiubo Li <xiubli@redhat.com> >>>> >>>> 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@redhat.com> >>>> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> --- >>>> 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. Okay. > 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. Yeah, this looks much better. Will fix this. Thanks - Xiubo > Thanks, > > Ilya >
On 6/15/23 20:50, Ilya Dryomov wrote: > On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: >> >> On 6/14/23 16:21, Ilya Dryomov wrote: >>> On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: >>>> From: Xiubo Li <xiubli@redhat.com> >>>> >>>> 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@redhat.com> >>>> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> --- >>>> 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 >
On Thu, Jun 15, 2023 at 2:57 PM Xiubo Li <xiubli@redhat.com> wrote: > > > On 6/15/23 20:50, Ilya Dryomov wrote: > > On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: > >> > >> On 6/14/23 16:21, Ilya Dryomov wrote: > >>> On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: > >>>> From: Xiubo Li <xiubli@redhat.com> > >>>> > >>>> 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@redhat.com> > >>>> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> > >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> > >>>> --- > >>>> 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). Thanks, Ilya
On 6/15/23 21:44, Ilya Dryomov wrote: > On Thu, Jun 15, 2023 at 2:57 PM Xiubo Li <xiubli@redhat.com> wrote: >> >> On 6/15/23 20:50, Ilya Dryomov wrote: >>> On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: >>>> On 6/14/23 16:21, Ilya Dryomov wrote: >>>>> On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: >>>>>> From: Xiubo Li <xiubli@redhat.com> >>>>>> >>>>>> 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@redhat.com> >>>>>> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> >>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>>>> --- >>>>>> 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 >
On 6/15/23 21:44, Ilya Dryomov wrote: > On Thu, Jun 15, 2023 at 2:57 PM Xiubo Li <xiubli@redhat.com> wrote: >> >> On 6/15/23 20:50, Ilya Dryomov wrote: >>> On Thu, Jun 15, 2023 at 3:49 AM Xiubo Li <xiubli@redhat.com> wrote: >>>> On 6/14/23 16:21, Ilya Dryomov wrote: >>>>> On Wed, Jun 14, 2023 at 3:33 AM <xiubli@redhat.com> wrote: >>>>>> From: Xiubo Li <xiubli@redhat.com> >>>>>> >>>>>> 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@redhat.com> >>>>>> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> >>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>>>> --- >>>>>> 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 >
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__) +# define pr_info_client(client, fmt, ...) \ + pr_info("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) +# define pr_warn_client(client, fmt, ...) \ + pr_warn("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) +# define pr_warn_once_client(client, fmt, ...) \ + pr_warn_once("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) +# define pr_err_client(client, fmt, ...) \ + pr_err("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) +# define pr_warn_ratelimited_client(client, fmt, ...) \ + pr_warn_ratelimited("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) +# define pr_err_ratelimited_client(client, fmt, ...) \ + pr_err_ratelimited("[%pU %lld] %s: " fmt, &client->fsid, \ + client->monc.auth->global_id, __func__, \ + ##__VA_ARGS__) + #endif