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). Thanks, Ilya
