Hello,
This patch series address feedback for a previous patch series sent by
me "docs: block: null_blk: enhance document style"[1].
First patch removes a restriction that prevents null_blk to load with
(nr_devices == 0). This restriction breaks applications, so it's a bug. I
have tested it running the kernel with `null_blk.nr_devices=0`.
In the previous series I have changed the type of var nr_devices, but I
forgot to change the type at module_param(). The second patch fix that.
The third patch uses a cleaver approach to make log messages consistent
using pr_fmt and the last one add a note on how to do that at the
coding style documentation.
Thanks,
André
[1] https://patchwork.kernel.org/project/linux-block/list/?series=172853
André Almeida (4):
null_blk: do not fail the module load with zero devices
null_blk: match the type of module parameter
null_blk: format pr_* logs with pr_fmt
coding-style: add explanation about pr_fmt macro
Documentation/process/coding-style.rst | 10 +++++++++-
drivers/block/null_blk.h | 4 +++-
drivers/block/null_blk_main.c | 24 ++++++++++--------------
drivers/block/null_blk_zoned.c | 6 +++---
4 files changed, 25 insertions(+), 19 deletions(-)
--
2.23.0
The pr_fmt macro is useful to format log messages printed by pr_XXXX()
functions. Add text to explain the purpose of it, how to use and an
example.
Signed-off-by: André Almeida <[email protected]>
---
Documentation/process/coding-style.rst | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index f4a2198187f9..276787bc2ff2 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -819,7 +819,15 @@ which you should use to make sure messages are matched to the right device
and driver, and are tagged with the right level: dev_err(), dev_warn(),
dev_info(), and so forth. For messages that aren't associated with a
particular device, <linux/printk.h> defines pr_notice(), pr_info(),
-pr_warn(), pr_err(), etc.
+pr_warn(), pr_err(), etc. It's possible to format pr_XXX() messages using the
+macro pr_fmt() to prevent rewriting the style of messages. It should be
+defined before including ``include/printk.h``, to avoid compiler warning about
+redefinitions. This is particularly useful for adding the name of the module at
+the beginning of the message, for instance:
+
+.. code-block:: c
+
+ #define pr_fmt(fmt) "module_name: " fmt
Coming up with good debugging messages can be quite a challenge; and once
you have them, they can be a huge help for remote troubleshooting. However
--
2.23.0
On Sep 13, 2019, at 11:58 AM, André Almeida <[email protected]> wrote:
>
> Hello,
>
> This patch series address feedback for a previous patch series sent by
> me "docs: block: null_blk: enhance document style"[1].
>
> First patch removes a restriction that prevents null_blk to load with
> (nr_devices == 0). This restriction breaks applications, so it's a bug. I
> have tested it running the kernel with `null_blk.nr_devices=0`.
>
> In the previous series I have changed the type of var nr_devices, but I
> forgot to change the type at module_param(). The second patch fix that.
>
> The third patch uses a cleaver approach to make log messages consistent
> using pr_fmt and the last one add a note on how to do that at the
> coding style documentation.
Please add Fixes tags to your patches.
On 9/13/19 4:08 PM, Joe Perches wrote:
> On Fri, 2019-09-13 at 15:57 -0300, André Almeida wrote:
>> The pr_fmt macro is useful to format log messages printed by pr_XXXX()
>> functions. Add text to explain the purpose of it, how to use and an
>> example.
> []
>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> []
>> @@ -819,7 +819,15 @@ which you should use to make sure messages are matched to the right device
>> and driver, and are tagged with the right level: dev_err(), dev_warn(),
>> dev_info(), and so forth. For messages that aren't associated with a
>> particular device, <linux/printk.h> defines pr_notice(), pr_info(),
>> -pr_warn(), pr_err(), etc.
>> +pr_warn(), pr_err(), etc. It's possible to format pr_XXX() messages using the
>> +macro pr_fmt() to prevent rewriting the style of messages. It should be
>> +defined before including ``include/printk.h``, to avoid compiler warning about
>
> Please make this '#include <linux/kernel.h>'
>
> printk.h should normally not be #included.
>
Thanks for the feedback, changed for v2.
On 9/13/19 11:57 AM, André Almeida wrote:
> The pr_fmt macro is useful to format log messages printed by pr_XXXX()
> functions. Add text to explain the purpose of it, how to use and an
> example.
>
> Signed-off-by: André Almeida <[email protected]>
Since Jonathan Corbet is documentation maintainer, shouldn't he be Cc'ed
explicitly for documentation patches? See also the MAINTAINERS file.
Bart.
On 9/13/19 1:22 PM, André Almeida wrote:
> On 9/13/19 4:03 PM, Jens Axboe wrote:
>> On Sep 13, 2019, at 11:58 AM, André Almeida <[email protected]> wrote:
>>>
>>> Hello,
>>>
>>> This patch series address feedback for a previous patch series sent by
>>> me "docs: block: null_blk: enhance document style"[1].
>>>
>>> First patch removes a restriction that prevents null_blk to load with
>>> (nr_devices == 0). This restriction breaks applications, so it's a bug. I
>>> have tested it running the kernel with `null_blk.nr_devices=0`.
>>>
>>> In the previous series I have changed the type of var nr_devices, but I
>>> forgot to change the type at module_param(). The second patch fix that.
>>>
>>> The third patch uses a cleaver approach to make log messages consistent
>>> using pr_fmt and the last one add a note on how to do that at the
>>> coding style documentation.
>>
>> Please add Fixes tags to your patches.
>>
>
> I've added to [PATCH 1/4], should I add to all 4 patches?
The ones where it's pertinent, yes, always. Both 1-2 fix issues with
the previous patches.
--
Jens Axboe
Since the variable nr_devices is an unsigned int, the module_param()
should also use this type. Change the type so they can match.
Signed-off-by: André Almeida <[email protected]>
---
drivers/block/null_blk_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index be32cb5ed339..5d20d65041bd 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -142,7 +142,7 @@ module_param_named(bs, g_bs, int, 0444);
MODULE_PARM_DESC(bs, "Block size (in bytes)");
static unsigned int nr_devices = 1;
-module_param(nr_devices, int, 0444);
+module_param(nr_devices, uint, 0444);
MODULE_PARM_DESC(nr_devices, "Number of devices to register");
static bool g_blocking;
--
2.23.0
Instead of writing "null_blk: " at the beginning of each pr_err/info/warn
log message, format messages using pr_fmt() macro. Change the order of
includes to prevent '"pr_fmt" redefined' warning messages.
Signed-off-by: André Almeida <[email protected]>
---
drivers/block/null_blk.h | 4 +++-
drivers/block/null_blk_main.c | 18 +++++++++---------
drivers/block/null_blk_zoned.c | 6 +++---
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
index a1b9929bd911..2d2183320be2 100644
--- a/drivers/block/null_blk.h
+++ b/drivers/block/null_blk.h
@@ -2,6 +2,8 @@
#ifndef __BLK_NULL_BLK_H
#define __BLK_NULL_BLK_H
+#define pr_fmt(fmt) "null_blk: " fmt
+
#include <linux/blkdev.h>
#include <linux/slab.h>
#include <linux/blk-mq.h>
@@ -96,7 +98,7 @@ void null_zone_reset(struct nullb_cmd *cmd, sector_t sector);
#else
static inline int null_zone_init(struct nullb_device *dev)
{
- pr_err("null_blk: CONFIG_BLK_DEV_ZONED not enabled\n");
+ pr_err("CONFIG_BLK_DEV_ZONED not enabled\n");
return -EINVAL;
}
static inline void null_zone_exit(struct nullb_device *dev) {}
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 5d20d65041bd..081ca55bb0a6 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -3,13 +3,13 @@
* Add configfs and memory store: Kyungchan Koh <[email protected]> and
* Shaohua Li <[email protected]>
*/
+#include "null_blk.h"
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/sched.h>
#include <linux/fs.h>
#include <linux/init.h>
-#include "null_blk.h"
#define PAGE_SECTORS_SHIFT (PAGE_SHIFT - SECTOR_SHIFT)
#define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
@@ -1311,7 +1311,7 @@ static bool should_requeue_request(struct request *rq)
static enum blk_eh_timer_return null_timeout_rq(struct request *rq, bool res)
{
- pr_info("null_blk: rq %p timed out\n", rq);
+ pr_info("rq %p timed out\n", rq);
blk_mq_complete_request(rq);
return BLK_EH_DONE;
}
@@ -1739,28 +1739,28 @@ static int __init null_init(void)
struct nullb_device *dev;
if (g_bs > PAGE_SIZE) {
- pr_warn("null_blk: invalid block size\n");
- pr_warn("null_blk: defaults block size to %lu\n", PAGE_SIZE);
+ pr_warn("invalid block size\n");
+ pr_warn("defaults block size to %lu\n", PAGE_SIZE);
g_bs = PAGE_SIZE;
}
if (!is_power_of_2(g_zone_size)) {
- pr_err("null_blk: zone_size must be power-of-two\n");
+ pr_err("zone_size must be power-of-two\n");
return -EINVAL;
}
if (g_home_node != NUMA_NO_NODE && g_home_node >= nr_online_nodes) {
- pr_err("null_blk: invalid home_node value\n");
+ pr_err("invalid home_node value\n");
g_home_node = NUMA_NO_NODE;
}
if (g_queue_mode == NULL_Q_RQ) {
- pr_err("null_blk: legacy IO path no longer available\n");
+ pr_err("legacy IO path no longer available\n");
return -EINVAL;
}
if (g_queue_mode == NULL_Q_MQ && g_use_per_node_hctx) {
if (g_submit_queues != nr_online_nodes) {
- pr_warn("null_blk: submit_queues param is set to %u.\n",
+ pr_warn("submit_queues param is set to %u.\n",
nr_online_nodes);
g_submit_queues = nr_online_nodes;
}
@@ -1803,7 +1803,7 @@ static int __init null_init(void)
}
}
- pr_info("null_blk: module loaded\n");
+ pr_info("module loaded\n");
return 0;
err_dev:
diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index cb28d93f2bd1..670f9cf79d80 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/vmalloc.h>
#include "null_blk.h"
+#include <linux/vmalloc.h>
/* zone_size in MBs to sectors. */
#define ZONE_SIZE_SHIFT 11
@@ -17,7 +17,7 @@ int null_zone_init(struct nullb_device *dev)
unsigned int i;
if (!is_power_of_2(dev->zone_size)) {
- pr_err("null_blk: zone_size must be power-of-two\n");
+ pr_err("zone_size must be power-of-two\n");
return -EINVAL;
}
@@ -31,7 +31,7 @@ int null_zone_init(struct nullb_device *dev)
if (dev->zone_nr_conv >= dev->nr_zones) {
dev->zone_nr_conv = dev->nr_zones - 1;
- pr_info("null_blk: changed the number of conventional zones to %u",
+ pr_info("changed the number of conventional zones to %u",
dev->zone_nr_conv);
}
--
2.23.0
On Fri, 2019-09-13 at 15:57 -0300, Andr? Almeida wrote:
> The pr_fmt macro is useful to format log messages printed by pr_XXXX()
> functions. Add text to explain the purpose of it, how to use and an
> example.
[]
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
[]
> @@ -819,7 +819,15 @@ which you should use to make sure messages are matched to the right device
> and driver, and are tagged with the right level: dev_err(), dev_warn(),
> dev_info(), and so forth. For messages that aren't associated with a
> particular device, <linux/printk.h> defines pr_notice(), pr_info(),
> -pr_warn(), pr_err(), etc.
> +pr_warn(), pr_err(), etc. It's possible to format pr_XXX() messages using the
> +macro pr_fmt() to prevent rewriting the style of messages. It should be
> +defined before including ``include/printk.h``, to avoid compiler warning about
Please make this '#include <linux/kernel.h>'
printk.h should normally not be #included.
On 9/13/19 4:03 PM, Jens Axboe wrote:
> On Sep 13, 2019, at 11:58 AM, André Almeida <[email protected]> wrote:
>>
>> Hello,
>>
>> This patch series address feedback for a previous patch series sent by
>> me "docs: block: null_blk: enhance document style"[1].
>>
>> First patch removes a restriction that prevents null_blk to load with
>> (nr_devices == 0). This restriction breaks applications, so it's a bug. I
>> have tested it running the kernel with `null_blk.nr_devices=0`.
>>
>> In the previous series I have changed the type of var nr_devices, but I
>> forgot to change the type at module_param(). The second patch fix that.
>>
>> The third patch uses a cleaver approach to make log messages consistent
>> using pr_fmt and the last one add a note on how to do that at the
>> coding style documentation.
>
> Please add Fixes tags to your patches.
>
I've added to [PATCH 1/4], should I add to all 4 patches?
On 09/13/2019 12:00 PM, Andr? Almeida wrote:
> Instead of writing "null_blk: " at the beginning of each pr_err/info/warn
Please limit above line to 72 character.
> log message, format messages using pr_fmt() macro. Change the order of
> includes to prevent '"pr_fmt" redefined' warning messages.
Why change the order of headers ? Can we use #undef ?
This change of order for block driver looks bit odd as compare to
othe kernel block driver source code.
See :- drivers/scsi, drivers/nvme.
>
> Signed-off-by: Andr? Almeida <[email protected]>
> ---
> drivers/block/null_blk.h | 4 +++-
> drivers/block/null_blk_main.c | 18 +++++++++---------
> drivers/block/null_blk_zoned.c | 6 +++---
> 3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
> index a1b9929bd911..2d2183320be2 100644
> --- a/drivers/block/null_blk.h
> +++ b/drivers/block/null_blk.h
> @@ -2,6 +2,8 @@
> #ifndef __BLK_NULL_BLK_H
> #define __BLK_NULL_BLK_H
>
> +#define pr_fmt(fmt) "null_blk: " fmt
Is there a reason why KBUILD_MODNAME is not used here ?
> +
> #include <linux/blkdev.h>
> #include <linux/slab.h>
> #include <linux/blk-mq.h>
> @@ -96,7 +98,7 @@ void null_zone_reset(struct nullb_cmd *cmd, sector_t sector);
> #else
> static inline int null_zone_init(struct nullb_device *dev)
> {
> - pr_err("null_blk: CONFIG_BLK_DEV_ZONED not enabled\n");
> + pr_err("CONFIG_BLK_DEV_ZONED not enabled\n");
> return -EINVAL;
> }
> static inline void null_zone_exit(struct nullb_device *dev) {}
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 5d20d65041bd..081ca55bb0a6 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -3,13 +3,13 @@
> * Add configfs and memory store: Kyungchan Koh <[email protected]> and
> * Shaohua Li <[email protected]>
> */
> +#include "null_blk.h"
> #include <linux/module.h>
>
> #include <linux/moduleparam.h>
> #include <linux/sched.h>
> #include <linux/fs.h>
> #include <linux/init.h>
> -#include "null_blk.h"
Unless there is a strong reason we should avoid this change ?
>
> #define PAGE_SECTORS_SHIFT (PAGE_SHIFT - SECTOR_SHIFT)
> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
> @@ -1311,7 +1311,7 @@ static bool should_requeue_request(struct request *rq)
>
> static enum blk_eh_timer_return null_timeout_rq(struct request *rq, bool res)
> {
> - pr_info("null_blk: rq %p timed out\n", rq);
> + pr_info("rq %p timed out\n", rq);
> blk_mq_complete_request(rq);
> return BLK_EH_DONE;
> }
> @@ -1739,28 +1739,28 @@ static int __init null_init(void)
> struct nullb_device *dev;
>
> if (g_bs > PAGE_SIZE) {
> - pr_warn("null_blk: invalid block size\n");
> - pr_warn("null_blk: defaults block size to %lu\n", PAGE_SIZE);
> + pr_warn("invalid block size\n");
> + pr_warn("defaults block size to %lu\n", PAGE_SIZE);
> g_bs = PAGE_SIZE;
> }
>
> if (!is_power_of_2(g_zone_size)) {
> - pr_err("null_blk: zone_size must be power-of-two\n");
> + pr_err("zone_size must be power-of-two\n");
> return -EINVAL;
> }
>
> if (g_home_node != NUMA_NO_NODE && g_home_node >= nr_online_nodes) {
> - pr_err("null_blk: invalid home_node value\n");
> + pr_err("invalid home_node value\n");
> g_home_node = NUMA_NO_NODE;
> }
>
> if (g_queue_mode == NULL_Q_RQ) {
> - pr_err("null_blk: legacy IO path no longer available\n");
> + pr_err("legacy IO path no longer available\n");
> return -EINVAL;
> }
> if (g_queue_mode == NULL_Q_MQ && g_use_per_node_hctx) {
> if (g_submit_queues != nr_online_nodes) {
> - pr_warn("null_blk: submit_queues param is set to %u.\n",
> + pr_warn("submit_queues param is set to %u.\n",
> nr_online_nodes);
> g_submit_queues = nr_online_nodes;
> }
> @@ -1803,7 +1803,7 @@ static int __init null_init(void)
> }
> }
>
> - pr_info("null_blk: module loaded\n");
> + pr_info("module loaded\n");
> return 0;
>
> err_dev:
> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
> index cb28d93f2bd1..670f9cf79d80 100644
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> -#include <linux/vmalloc.h>
> #include "null_blk.h"
> +#include <linux/vmalloc.h>
Same here.
>
> /* zone_size in MBs to sectors. */
> #define ZONE_SIZE_SHIFT 11
> @@ -17,7 +17,7 @@ int null_zone_init(struct nullb_device *dev)
> unsigned int i;
>
> if (!is_power_of_2(dev->zone_size)) {
> - pr_err("null_blk: zone_size must be power-of-two\n");
> + pr_err("zone_size must be power-of-two\n");
> return -EINVAL;
> }
>
> @@ -31,7 +31,7 @@ int null_zone_init(struct nullb_device *dev)
>
> if (dev->zone_nr_conv >= dev->nr_zones) {
> dev->zone_nr_conv = dev->nr_zones - 1;
> - pr_info("null_blk: changed the number of conventional zones to %u",
> + pr_info("changed the number of conventional zones to %u",
> dev->zone_nr_conv);
> }
>
>
On Fri, 13 Sep 2019 13:22:18 -0700
Bart Van Assche <[email protected]> wrote:
> On 9/13/19 11:57 AM, André Almeida wrote:
> > The pr_fmt macro is useful to format log messages printed by pr_XXXX()
> > functions. Add text to explain the purpose of it, how to use and an
> > example.
> >
> > Signed-off-by: André Almeida <[email protected]>
>
> Since Jonathan Corbet is documentation maintainer, shouldn't he be Cc'ed
> explicitly for documentation patches? See also the MAINTAINERS file.
...and indeed I was CC'd on the patch - and your response :)
Thanks,
jon