| Message ID | cover.1538873915.git.rfreire@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | CIFS: Info-level log support, print message when attempting mount. | expand |
Merged into cifs-2.6.git for-next Made a trivial tab/space correction in the patch (pointed out by checkpatch) and then added a trivial followon patch to address a comment/style (trivial) issue pointed out by checkpatch and to add a little more detailed comments about generally when to use each debug function. If any objections let me know. On Sun, Oct 7, 2018 at 10:21 AM Rodrigo Freire <rfreire@redhat.com> wrote: > > Hi Steve, > From our conversation over v2, I came out with this v3 patch, which I broke > in two commits: > > * The first commit in cifs_debug.h, creating the cifs_info() function. > - The aim of this commit is to allow to the developer to be able to print > informational-level data without having to use pr_info, pr_notice etc, > in line with other filesystems. > . One interesting and noteworthy feature of cifs_info() is that it is > transparent to the CIFS_DEBUG config state, either in "y" or "n". > . Also, by using KERN_INFO level, it can be safely filtered by a > maintainer / administrator, without cluttering their log monitors, > since this is a low level alert. > - I took the liberty to not add it inside the existing pr_debug, because > of the eventual need of some developer to print stuff that should be > printed not only in CIFS_DEBUG mode (as there are plenty of pr_notice > scattered over the code). > - Also, this is not a debug but a info message, so i liked cifs_info() > more ;-) > - I saw plenty of pr_notice() in CIFS code, but I resisted the urge to > convert them to cifs_info(). > > * The second commit contains the code printing a cifs_info() when attempting > a CIFS mount operation. > > Appreciate your review. > > V3: Created a new cifs_info() function, moved the mount attempt message to > cifs_info > > V2: Created a loop to select the right cifs_dbg message to be printed, > considering the current system's scenario, in order to avoid a > duplicate message or stripping out important information in > debug. > > Rodrigo Freire (2): > CIFS: Adds information-level logging function > CIFS: Print message when attempting a mount > > fs/cifs/cifs_debug.h | 16 ++++++++++++++++ > fs/cifs/cifsfs.c | 7 ++++++- > fs/cifs/transport.c | 2 +- > 3 files changed, 23 insertions(+), 2 deletions(-) > > -- > 1.8.3.1 >
Thanks Steve! Sorry for overlooking it, whoops. One more question; what’s your/community opinion on rewriting the existing pr_notice to the new cifs_info? I could happily retrofit it. Have a great week! - RF Sent from a mobile device > On 7 Oct 2018, at 15:59, Steve French <smfrench@gmail.com> wrote: > > Merged into cifs-2.6.git for-next > > Made a trivial tab/space correction in the patch (pointed out by > checkpatch) and then added a trivial followon patch to address a > comment/style (trivial) > issue pointed out by checkpatch and to add a little more detailed > comments about generally when to use each debug function. If any > objections let me know. > > >> On Sun, Oct 7, 2018 at 10:21 AM Rodrigo Freire <rfreire@redhat.com> wrote: >> >> Hi Steve, >> From our conversation over v2, I came out with this v3 patch, which I broke >> in two commits: >> >> * The first commit in cifs_debug.h, creating the cifs_info() function. >> - The aim of this commit is to allow to the developer to be able to print >> informational-level data without having to use pr_info, pr_notice etc, >> in line with other filesystems. >> . One interesting and noteworthy feature of cifs_info() is that it is >> transparent to the CIFS_DEBUG config state, either in "y" or "n". >> . Also, by using KERN_INFO level, it can be safely filtered by a >> maintainer / administrator, without cluttering their log monitors, >> since this is a low level alert. >> - I took the liberty to not add it inside the existing pr_debug, because >> of the eventual need of some developer to print stuff that should be >> printed not only in CIFS_DEBUG mode (as there are plenty of pr_notice >> scattered over the code). >> - Also, this is not a debug but a info message, so i liked cifs_info() >> more ;-) >> - I saw plenty of pr_notice() in CIFS code, but I resisted the urge to >> convert them to cifs_info(). >> >> * The second commit contains the code printing a cifs_info() when attempting >> a CIFS mount operation. >> >> Appreciate your review. >> >> V3: Created a new cifs_info() function, moved the mount attempt message to >> cifs_info >> >> V2: Created a loop to select the right cifs_dbg message to be printed, >> considering the current system's scenario, in order to avoid a >> duplicate message or stripping out important information in >> debug. >> >> Rodrigo Freire (2): >> CIFS: Adds information-level logging function >> CIFS: Print message when attempting a mount >> >> fs/cifs/cifs_debug.h | 16 ++++++++++++++++ >> fs/cifs/cifsfs.c | 7 ++++++- >> fs/cifs/transport.c | 2 +- >> 3 files changed, 23 insertions(+), 2 deletions(-) >> >> -- >> 1.8.3.1 >> > > > -- > Thanks, > > Steve > <0001-cifs-minor-clarification-in-comments.patch>
On Sun, Oct 7, 2018 at 3:27 PM Rodrigo Freire <rfreire@redhat.com> wrote: > > Thanks Steve! Sorry for overlooking it, whoops. > > One more question; what’s your/community opinion on rewriting the existing pr_notice to the new cifs_info? > > I could happily retrofit it. I don't have a strong opinion. The four callers could be changed, but my gut reaction is that it is a much higher priority to add useful dynamic tracepoints (ftrace ie trace-cmd) to cifs to make it easier to debug real customer problems that are currently awkward to debug. By comparison, even ext4 has over 100 dynamic tracepoints, but larger file systems like XFS has 500 (!), and nfs (including SunRPC) has about 100. My guess is that going from our current 28 dynamic trace points (in cifs.ko) to double that (at least) will happen as developers add tracepoints to help them debug real problems (and of course it will help developers working on future problems even more ...) > > On 7 Oct 2018, at 15:59, Steve French <smfrench@gmail.com> wrote: > > > > Merged into cifs-2.6.git for-next > > > > Made a trivial tab/space correction in the patch (pointed out by > > checkpatch) and then added a trivial followon patch to address a > > comment/style (trivial) > > issue pointed out by checkpatch and to add a little more detailed > > comments about generally when to use each debug function. If any > > objections let me know. > > > > > >> On Sun, Oct 7, 2018 at 10:21 AM Rodrigo Freire <rfreire@redhat.com> wrote: > >> > >> Hi Steve, > >> From our conversation over v2, I came out with this v3 patch, which I broke > >> in two commits: > >> > >> * The first commit in cifs_debug.h, creating the cifs_info() function. > >> - The aim of this commit is to allow to the developer to be able to print > >> informational-level data without having to use pr_info, pr_notice etc, > >> in line with other filesystems. > >> . One interesting and noteworthy feature of cifs_info() is that it is > >> transparent to the CIFS_DEBUG config state, either in "y" or "n". <snip>
Hi Steve, From our conversation over v2, I came out with this v3 patch, which I broke in two commits: * The first commit in cifs_debug.h, creating the cifs_info() function. - The aim of this commit is to allow to the developer to be able to print informational-level data without having to use pr_info, pr_notice etc, in line with other filesystems. . One interesting and noteworthy feature of cifs_info() is that it is transparent to the CIFS_DEBUG config state, either in "y" or "n". . Also, by using KERN_INFO level, it can be safely filtered by a maintainer / administrator, without cluttering their log monitors, since this is a low level alert. - I took the liberty to not add it inside the existing pr_debug, because of the eventual need of some developer to print stuff that should be printed not only in CIFS_DEBUG mode (as there are plenty of pr_notice scattered over the code). - Also, this is not a debug but a info message, so i liked cifs_info() more ;-) - I saw plenty of pr_notice() in CIFS code, but I resisted the urge to convert them to cifs_info(). * The second commit contains the code printing a cifs_info() when attempting a CIFS mount operation. Appreciate your review. V3: Created a new cifs_info() function, moved the mount attempt message to cifs_info V2: Created a loop to select the right cifs_dbg message to be printed, considering the current system's scenario, in order to avoid a duplicate message or stripping out important information in debug. Rodrigo Freire (2): CIFS: Adds information-level logging function CIFS: Print message when attempting a mount fs/cifs/cifs_debug.h | 16 ++++++++++++++++ fs/cifs/cifsfs.c | 7 ++++++- fs/cifs/transport.c | 2 +- 3 files changed, 23 insertions(+), 2 deletions(-)