[PATCH 0/4] null_blk: fixes around nr_devices and log improvements

[PATCH 0/4] null_blk: fixes around nr_devices and log improvements

[André Almeida]

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

[PATCH 1/4] null_blk: do not fail the module load with zero devices

[André Almeida]

The module load should fail only if there is something wrong with the
configuration or if an error prevents it to work properly. The module
should be able to be loaded with (nr_device == 0), since it will not
trigger errors or be in malfunction state. Preventing loading with zero
devices also breaks applications that configures this module using
configfs API. Remove the nr_device check to fix this.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
Fixes: f7c4ce890dd2 ("null_blk: validate the number of devices")
---
 drivers/block/null_blk_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index ab4b87677139..be32cb5ed339 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1758,10 +1758,6 @@ static int __init null_init(void)
 		pr_err("null_blk: legacy IO path no longer available\n");
 		return -EINVAL;
 	}
-	if (!nr_devices) {
-		pr_err("null_blk: invalid number of devices\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",
-- 
2.23.0

[PATCH 2/4] null_blk: match the type of parameter nr_devices

[André Almeida]

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 <andrealmeid@collabora.com>
---
 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

[PATCH 3/4] null_blk: format pr_* logs with pr_fmt

[André Almeida]

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 <andrealmeid@collabora.com>
---
 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 <kkc6196@fb.com> and
  * Shaohua Li <shli@fb.com>
  */
+#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

Re: [PATCH 3/4] null_blk: format pr_* logs with pr_fmt

[Chaitanya Kulkarni]

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 <andrealmeid@collabora.com>
> ---
>   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 <kkc6196@fb.com> and
>    * Shaohua Li <shli@fb.com>
>    */
> +#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);
>   	}
>
>

[PATCH 4/4] coding-style: add explanation about pr_fmt macro

[André Almeida]

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 <andrealmeid@collabora.com>
---
 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

Re: [PATCH 4/4] coding-style: add explanation about pr_fmt macro

[Joe Perches]

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.

Re: [PATCH 4/4] coding-style: add explanation about pr_fmt macro

[André Almeida]

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.

Re: [PATCH 4/4] coding-style: add explanation about pr_fmt macro

[Bart Van Assche]

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 <andrealmeid@collabora.com>

Since Jonathan Corbet is documentation maintainer, shouldn't he be Cc'ed 
explicitly for documentation patches? See also the MAINTAINERS file.

Bart.

Re: [PATCH 4/4] coding-style: add explanation about pr_fmt macro

[Jonathan Corbet]

On Fri, 13 Sep 2019 13:22:18 -0700
Bart Van Assche <bvanassche@acm.org> 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 <andrealmeid@collabora.com>  
> 
> 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

Re: [PATCH 0/4] null_blk: fixes around nr_devices and log improvements

[Jens Axboe]

On Sep 13, 2019, at 11:58 AM, André Almeida <andrealmeid@collabora.com> 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. 

Re: [PATCH 0/4] null_blk: fixes around nr_devices and log improvements

[André Almeida]

On 9/13/19 4:03 PM, Jens Axboe wrote:
> On Sep 13, 2019, at 11:58 AM, André Almeida <andrealmeid@collabora.com> 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?

Re: [PATCH 0/4] null_blk: fixes around nr_devices and log improvements

[Jens Axboe]

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 <andrealmeid@collabora.com> 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