linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] docs: block: null_blk: enhance document style
@ 2019-09-11 14:46 André Almeida
  2019-09-11 14:46 ` [PATCH 2/3] null_blk: fix module name at log message André Almeida
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: André Almeida @ 2019-09-11 14:46 UTC (permalink / raw)
  To: linux-block, linux-doc
  Cc: corbet, axboe, m, kernel, krisman, André Almeida

Use proper ReST syntax for chapters. Add more information to enhance
standardization in the file and to make the rendering more homogeneous.
Add a SPDX identifier. Mark single-queue mode as deprecated.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
I've asked the file creator (Matias Bjørling <m@bjorling.me>), and
he confirmed that I could use GPL-2.0 for this file.

 Documentation/block/null_blk.rst | 33 +++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/Documentation/block/null_blk.rst b/Documentation/block/null_blk.rst
index 31451d80783c..edbbab2f12f8 100644
--- a/Documentation/block/null_blk.rst
+++ b/Documentation/block/null_blk.rst
@@ -1,19 +1,16 @@
+.. SPDX-License-Identifier: GPL-2.0
+
 ========================
 Null block device driver
 ========================
 
-1. Overview
-===========
+Overview
+========
 
-The null block device (/dev/nullb*) is used for benchmarking the various
+The null block device (``/dev/nullb*``) is used for benchmarking the various
 block-layer implementations. It emulates a block device of X gigabytes in size.
-The following instances are possible:
-
-  Single-queue block-layer
-
-    - Request-based.
-    - Single submission queue per device.
-    - Implements IO scheduling algorithms (CFQ, Deadline, noop).
+It does not execute any read/write operation, just mark them as complete in
+the request queue. The following instances are possible:
 
   Multi-queue block-layer
 
@@ -27,15 +24,15 @@ The following instances are possible:
 
 All of them have a completion queue for each core in the system.
 
-2. Module parameters applicable for all instances
-=================================================
+Module parameters
+=================
 
 queue_mode=[0-2]: Default: 2-Multi-queue
   Selects which block-layer the module should instantiate with.
 
   =  ============
   0  Bio-based
-  1  Single-queue
+  1  Single-queue (deprecated)
   2  Multi-queue
   =  ============
 
@@ -67,7 +64,7 @@ irqmode=[0-2]: Default: 1-Soft-irq
 completion_nsec=[ns]: Default: 10,000ns
   Combined with irqmode=2 (timer). The time each completion event must wait.
 
-submit_queues=[1..nr_cpus]:
+submit_queues=[1..nr_cpus]: Default: 1
   The number of submission queues attached to the device driver. If unset, it
   defaults to 1. For multi-queue, it is ignored when use_per_node_hctx module
   parameter is 1.
@@ -75,9 +72,11 @@ submit_queues=[1..nr_cpus]:
 hw_queue_depth=[0..qdepth]: Default: 64
   The hardware queue depth of the device.
 
-III: Multi-queue specific parameters
+Multi-queue specific parameters
+-------------------------------
 
 use_per_node_hctx=[0/1]: Default: 0
+  Number of hardware context queues.
 
   =  =====================================================================
   0  The number of submit queues are set to the value of the submit_queues
@@ -87,6 +86,7 @@ use_per_node_hctx=[0/1]: Default: 0
   =  =====================================================================
 
 no_sched=[0/1]: Default: 0
+  Enable/disable the io scheduler.
 
   =  ======================================
   0  nullb* use default blk-mq io scheduler
@@ -94,6 +94,7 @@ no_sched=[0/1]: Default: 0
   =  ======================================
 
 blocking=[0/1]: Default: 0
+  Blocking behavior of the request queue.
 
   =  ===============================================================
   0  Register as a non-blocking blk-mq driver device.
@@ -103,6 +104,7 @@ blocking=[0/1]: Default: 0
   =  ===============================================================
 
 shared_tags=[0/1]: Default: 0
+  Sharing tags between devices.
 
   =  ================================================================
   0  Tag set is not shared.
@@ -111,6 +113,7 @@ shared_tags=[0/1]: Default: 0
   =  ================================================================
 
 zoned=[0/1]: Default: 0
+  Device is a random-access or a zoned block device.
 
   =  ======================================================================
   0  Block device is exposed as a random-access block device.
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/3] null_blk: fix module name at log message
  2019-09-11 14:46 [PATCH 1/3] docs: block: null_blk: enhance document style André Almeida
@ 2019-09-11 14:46 ` André Almeida
  2019-09-12 15:47   ` Ezequiel Garcia
  2019-09-11 14:46 ` [PATCH 3/3] null_blk: validated the number of devices André Almeida
  2019-09-11 22:05 ` [PATCH 1/3] docs: block: null_blk: enhance document style Jens Axboe
  2 siblings, 1 reply; 17+ messages in thread
From: André Almeida @ 2019-09-11 14:46 UTC (permalink / raw)
  To: linux-block, linux-doc
  Cc: corbet, axboe, m, kernel, krisman, André Almeida

The name of the module is "null_blk", not "null". Make `pr_info()` follow
the pattern of `pr_err()` log messages.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/block/null_blk_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 99328ded60d1..7bc553ff7407 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -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: rq %p timed out\n", rq);
+	pr_info("null_blk: rq %p timed out\n", rq);
 	blk_mq_complete_request(rq);
 	return BLK_EH_DONE;
 }
@@ -1803,7 +1803,7 @@ static int __init null_init(void)
 		}
 	}
 
-	pr_info("null: module loaded\n");
+	pr_info("null_blk: module loaded\n");
 	return 0;
 
 err_dev:
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/3] null_blk: validated the number of devices
  2019-09-11 14:46 [PATCH 1/3] docs: block: null_blk: enhance document style André Almeida
  2019-09-11 14:46 ` [PATCH 2/3] null_blk: fix module name at log message André Almeida
@ 2019-09-11 14:46 ` André Almeida
  2019-09-12 16:19   ` Matthew Wilcox
  2019-09-11 22:05 ` [PATCH 1/3] docs: block: null_blk: enhance document style Jens Axboe
  2 siblings, 1 reply; 17+ messages in thread
From: André Almeida @ 2019-09-11 14:46 UTC (permalink / raw)
  To: linux-block, linux-doc
  Cc: corbet, axboe, m, kernel, krisman, André Almeida

There is no sense defining a negative number of devices, so change the type
to unsigned. If the number of devices is 0, it is impossible to the
userspace interact with the module, so there is no reasoning in loading
it.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/block/null_blk_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 7bc553ff7407..ab4b87677139 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -141,7 +141,7 @@ static int g_bs = 512;
 module_param_named(bs, g_bs, int, 0444);
 MODULE_PARM_DESC(bs, "Block size (in bytes)");
 
-static int nr_devices = 1;
+static unsigned int nr_devices = 1;
 module_param(nr_devices, int, 0444);
 MODULE_PARM_DESC(nr_devices, "Number of devices to register");
 
@@ -1758,6 +1758,10 @@ 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] docs: block: null_blk: enhance document style
  2019-09-11 14:46 [PATCH 1/3] docs: block: null_blk: enhance document style André Almeida
  2019-09-11 14:46 ` [PATCH 2/3] null_blk: fix module name at log message André Almeida
  2019-09-11 14:46 ` [PATCH 3/3] null_blk: validated the number of devices André Almeida
@ 2019-09-11 22:05 ` Jens Axboe
  2019-09-11 22:26   ` André Almeida
  2 siblings, 1 reply; 17+ messages in thread
From: Jens Axboe @ 2019-09-11 22:05 UTC (permalink / raw)
  To: André Almeida, linux-block, linux-doc; +Cc: corbet, m, kernel, krisman

On 9/11/19 8:46 AM, André Almeida wrote:
> Use proper ReST syntax for chapters. Add more information to enhance
> standardization in the file and to make the rendering more homogeneous.
> Add a SPDX identifier. Mark single-queue mode as deprecated.

Please use a cover letter for sending block patches if there's more
than one in the series.

I applied these, though rewrote the commit message for patch #3.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] docs: block: null_blk: enhance document style
  2019-09-11 22:05 ` [PATCH 1/3] docs: block: null_blk: enhance document style Jens Axboe
@ 2019-09-11 22:26   ` André Almeida
  0 siblings, 0 replies; 17+ messages in thread
From: André Almeida @ 2019-09-11 22:26 UTC (permalink / raw)
  To: Jens Axboe, linux-block, linux-doc; +Cc: corbet, m, kernel, krisman

On 9/11/19 7:05 PM, Jens Axboe wrote:
> On 9/11/19 8:46 AM, André Almeida wrote:
>> Use proper ReST syntax for chapters. Add more information to enhance
>> standardization in the file and to make the rendering more homogeneous.
>> Add a SPDX identifier. Mark single-queue mode as deprecated.
> 
> Please use a cover letter for sending block patches if there's more
> than one in the series.
>

Thanks for the feedback, I'll do it for next series.

> I applied these, though rewrote the commit message for patch #3.
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] null_blk: fix module name at log message
  2019-09-11 14:46 ` [PATCH 2/3] null_blk: fix module name at log message André Almeida
@ 2019-09-12 15:47   ` Ezequiel Garcia
  2019-09-12 20:47     ` Jens Axboe
  0 siblings, 1 reply; 17+ messages in thread
From: Ezequiel Garcia @ 2019-09-12 15:47 UTC (permalink / raw)
  To: André Almeida, linux-block, linux-doc
  Cc: corbet, axboe, m, kernel, krisman

Hi André, Jens,

On Wed, 2019-09-11 at 11:46 -0300, André Almeida wrote:
> The name of the module is "null_blk", not "null". Make `pr_info()` follow
> the pattern of `pr_err()` log messages.
> 

Instead of doing these fixes manually, it's more consistent and smarter
to use pr_fmt. There are many examples of drivers doing that.

I don't know if this patch can be dropped, and replaced with one used pr_fmt
or if doesn't worth the trouble.

It would be even better to also patch Documentation/process/coding-style.rst,
in particular the printing section, making a mention to pr_fmt.

Thanks,
Ezequiel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-11 14:46 ` [PATCH 3/3] null_blk: validated the number of devices André Almeida
@ 2019-09-12 16:19   ` Matthew Wilcox
  2019-09-12 22:07     ` André Almeida
  0 siblings, 1 reply; 17+ messages in thread
From: Matthew Wilcox @ 2019-09-12 16:19 UTC (permalink / raw)
  To: André Almeida
  Cc: linux-block, linux-doc, corbet, axboe, m, kernel, krisman

On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>  
> -static int nr_devices = 1;
> +static unsigned int nr_devices = 1;
>  module_param(nr_devices, int, 0444);

^^^ you forgot to change the module_param to match

> +	if (!nr_devices) {
> +		pr_err("null_blk: invalid number of devices\n");
> +		return -EINVAL;
> +	}

I don't think this is necessary.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] null_blk: fix module name at log message
  2019-09-12 15:47   ` Ezequiel Garcia
@ 2019-09-12 20:47     ` Jens Axboe
  2019-09-12 22:08       ` André Almeida
  0 siblings, 1 reply; 17+ messages in thread
From: Jens Axboe @ 2019-09-12 20:47 UTC (permalink / raw)
  To: Ezequiel Garcia, André Almeida, linux-block, linux-doc
  Cc: corbet, m, kernel, krisman

On 9/12/19 9:47 AM, Ezequiel Garcia wrote:
> Hi André, Jens,
> 
> On Wed, 2019-09-11 at 11:46 -0300, André Almeida wrote:
>> The name of the module is "null_blk", not "null". Make `pr_info()` follow
>> the pattern of `pr_err()` log messages.
>>
> 
> Instead of doing these fixes manually, it's more consistent and smarter
> to use pr_fmt. There are many examples of drivers doing that.
> 
> I don't know if this patch can be dropped, and replaced with one used pr_fmt
> or if doesn't worth the trouble.
> 
> It would be even better to also patch Documentation/process/coding-style.rst,
> in particular the printing section, making a mention to pr_fmt.

André, please address the comments in this email and from Willy. Note
that the previous patch is queued up, so you'll need to make it relative
to that.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-12 16:19   ` Matthew Wilcox
@ 2019-09-12 22:07     ` André Almeida
  2019-09-12 22:20       ` Chaitanya Kulkarni
  0 siblings, 1 reply; 17+ messages in thread
From: André Almeida @ 2019-09-12 22:07 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: linux-block, linux-doc, corbet, axboe, m, kernel, krisman

Hello Matthew,

On 9/12/19 1:19 PM, Matthew Wilcox wrote:
> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>  
>> -static int nr_devices = 1;
>> +static unsigned int nr_devices = 1;
>>  module_param(nr_devices, int, 0444);
> 
> ^^^ you forgot to change the module_param to match
> 
>> +	if (!nr_devices) {
>> +		pr_err("null_blk: invalid number of devices\n");
>> +		return -EINVAL;
>> +	}
> 
> I don't think this is necessary.
> 

Could you explain why you don't think is necessary? As I see, the module
can't be used without any /dev/nullb* device, so why we should load it?

Thanks,
	André

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] null_blk: fix module name at log message
  2019-09-12 20:47     ` Jens Axboe
@ 2019-09-12 22:08       ` André Almeida
  0 siblings, 0 replies; 17+ messages in thread
From: André Almeida @ 2019-09-12 22:08 UTC (permalink / raw)
  To: Jens Axboe, Ezequiel Garcia, linux-block, linux-doc
  Cc: corbet, m, kernel, krisman

On 9/12/19 5:47 PM, Jens Axboe wrote:
> On 9/12/19 9:47 AM, Ezequiel Garcia wrote:
>> Hi André, Jens,
>>
>> On Wed, 2019-09-11 at 11:46 -0300, André Almeida wrote:
>>> The name of the module is "null_blk", not "null". Make `pr_info()` follow
>>> the pattern of `pr_err()` log messages.
>>>
>>
>> Instead of doing these fixes manually, it's more consistent and smarter
>> to use pr_fmt. There are many examples of drivers doing that.
>>
>> I don't know if this patch can be dropped, and replaced with one used pr_fmt
>> or if doesn't worth the trouble.
>>
>> It would be even better to also patch Documentation/process/coding-style.rst,
>> in particular the printing section, making a mention to pr_fmt.
> 
> André, please address the comments in this email and from Willy. Note
> that the previous patch is queued up, so you'll need to make it relative
> to that.
> 

Ok Jens, I'll do it.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-12 22:07     ` André Almeida
@ 2019-09-12 22:20       ` Chaitanya Kulkarni
  2019-09-13 14:57         ` André Almeida
  0 siblings, 1 reply; 17+ messages in thread
From: Chaitanya Kulkarni @ 2019-09-12 22:20 UTC (permalink / raw)
  To: André Almeida, Matthew Wilcox
  Cc: linux-block, linux-doc, corbet, axboe, m, kernel, krisman

On 09/12/2019 03:09 PM, André Almeida wrote:
> Hello Matthew,
>
> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>>
>>> -static int nr_devices = 1;
>>> +static unsigned int nr_devices = 1;
>>>   module_param(nr_devices, int, 0444);
>>
>> ^^^ you forgot to change the module_param to match
>>
>>> +	if (!nr_devices) {
>>> +		pr_err("null_blk: invalid number of devices\n");
>>> +		return -EINVAL;
>>> +	}
>>
>> I don't think this is necessary.
>>
>
> Could you explain why you don't think is necessary? As I see, the module
> can't be used without any /dev/nullb* device, so why we should load it?
>
> Thanks,
> 	André
>

I think Matthew is right here. I think module can be loaded with 
nr_devices=0.

Did you get a chance to test nr_device=0 condition ?

Also, did you get a chance to test this patch with all the
possible conditions ?





^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-12 22:20       ` Chaitanya Kulkarni
@ 2019-09-13 14:57         ` André Almeida
  2019-09-13 15:12           ` Matthew Wilcox
  0 siblings, 1 reply; 17+ messages in thread
From: André Almeida @ 2019-09-13 14:57 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Matthew Wilcox
  Cc: linux-block, linux-doc, corbet, axboe, m, kernel, krisman

On 9/12/19 7:20 PM, Chaitanya Kulkarni wrote:
> On 09/12/2019 03:09 PM, André Almeida wrote:
>> Hello Matthew,
>>
>> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
>>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>>>
>>>> -static int nr_devices = 1;
>>>> +static unsigned int nr_devices = 1;
>>>>   module_param(nr_devices, int, 0444);
>>>
>>> ^^^ you forgot to change the module_param to match
>>>
>>>> +	if (!nr_devices) {
>>>> +		pr_err("null_blk: invalid number of devices\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> I don't think this is necessary.
>>>
>>
>> Could you explain why you don't think is necessary? As I see, the module
>> can't be used without any /dev/nullb* device, so why we should load it?
>>
>> Thanks,
>> 	André
>>
> 
> I think Matthew is right here. I think module can be loaded with 
> nr_devices=0.
> 
> Did you get a chance to test nr_device=0 condition ?
> 

Yes. It says "module loaded" and lsmod shows that it is loaded indeed.
But you don't have any /dev/nullb*, so you can't do much with the module.

With this patch, it refuses to load the module.

> Also, did you get a chance to test this patch with all the
> possible conditions ?
>

I did not tested with all possible conditions, but I tested with the
following ones:

* Using a negative number of devices:
	- Previously, it would alloc and add a huge number of devices until the
system gets out of memory
	- With module_param as uint, it will throw a "invalid for parameter
`nr_devices'" and refuse to load

* Using a range of values (1, 10, 100, 1000):
	- It will works as expect, creating some /dev/nullbX accordingly with
your parameter. Works fine with and without this patch.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-13 14:57         ` André Almeida
@ 2019-09-13 15:12           ` Matthew Wilcox
  2019-09-13 15:39             ` André Almeida
  2019-09-13 16:23             ` Jens Axboe
  0 siblings, 2 replies; 17+ messages in thread
From: Matthew Wilcox @ 2019-09-13 15:12 UTC (permalink / raw)
  To: André Almeida
  Cc: Chaitanya Kulkarni, linux-block, linux-doc, corbet, axboe, m,
	kernel, krisman

On Fri, Sep 13, 2019 at 11:57:17AM -0300, André Almeida wrote:
> On 9/12/19 7:20 PM, Chaitanya Kulkarni wrote:
> > On 09/12/2019 03:09 PM, André Almeida wrote:
> >> Hello Matthew,
> >>
> >> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
> >>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
> >>>>
> >>>> -static int nr_devices = 1;
> >>>> +static unsigned int nr_devices = 1;
> >>>>   module_param(nr_devices, int, 0444);
> >>>
> >>> ^^^ you forgot to change the module_param to match
> >>>
> >>>> +	if (!nr_devices) {
> >>>> +		pr_err("null_blk: invalid number of devices\n");
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>
> >>> I don't think this is necessary.
> >>>
> >>
> >> Could you explain why you don't think is necessary? As I see, the module
> >> can't be used without any /dev/nullb* device, so why we should load it?
> >>
> >> Thanks,
> >> 	André
> >>
> > 
> > I think Matthew is right here. I think module can be loaded with 
> > nr_devices=0.
> > 
> > Did you get a chance to test nr_device=0 condition ?
> > 
> 
> Yes. It says "module loaded" and lsmod shows that it is loaded indeed.
> But you don't have any /dev/nullb*, so you can't do much with the module.
> 
> With this patch, it refuses to load the module.

Why is that an improvement?  I agree it's an uninteresting thing to ask
for, but I don't see a compelling need to fail the module load because
of it.

> I did not tested with all possible conditions, but I tested with the
> following ones:
> 
> * Using a negative number of devices:
> 	- Previously, it would alloc and add a huge number of devices until the
> system gets out of memory
> 	- With module_param as uint, it will throw a "invalid for parameter
> `nr_devices'" and refuse to load
> 
> * Using a range of values (1, 10, 100, 1000):
> 	- It will works as expect, creating some /dev/nullbX accordingly with
> your parameter. Works fine with and without this patch.

If you ask it to create 4 billion devices, what happens?  Obviously we'll
run out of dev_t at some point ...


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-13 15:12           ` Matthew Wilcox
@ 2019-09-13 15:39             ` André Almeida
  2019-09-13 16:23             ` Jens Axboe
  1 sibling, 0 replies; 17+ messages in thread
From: André Almeida @ 2019-09-13 15:39 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Chaitanya Kulkarni, linux-block, linux-doc, corbet, axboe, m,
	kernel, krisman

On 9/13/19 12:12 PM, Matthew Wilcox wrote:
> On Fri, Sep 13, 2019 at 11:57:17AM -0300, André Almeida wrote:
>> On 9/12/19 7:20 PM, Chaitanya Kulkarni wrote:
>>> On 09/12/2019 03:09 PM, André Almeida wrote:
>>>> Hello Matthew,
>>>>
>>>> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
>>>>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>>>>>
>>>>>> -static int nr_devices = 1;
>>>>>> +static unsigned int nr_devices = 1;
>>>>>>   module_param(nr_devices, int, 0444);
>>>>>
>>>>> ^^^ you forgot to change the module_param to match
>>>>>
>>>>>> +	if (!nr_devices) {
>>>>>> +		pr_err("null_blk: invalid number of devices\n");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>
>>>>> I don't think this is necessary.
>>>>>
>>>>
>>>> Could you explain why you don't think is necessary? As I see, the module
>>>> can't be used without any /dev/nullb* device, so why we should load it?
>>>>
>>>> Thanks,
>>>> 	André
>>>>
>>>
>>> I think Matthew is right here. I think module can be loaded with 
>>> nr_devices=0.
>>>
>>> Did you get a chance to test nr_device=0 condition ?
>>>
>>
>> Yes. It says "module loaded" and lsmod shows that it is loaded indeed.
>> But you don't have any /dev/nullb*, so you can't do much with the module.
>>
>> With this patch, it refuses to load the module.
> 
> Why is that an improvement?  I agree it's an uninteresting thing to ask
> for, but I don't see a compelling need to fail the module load because
> of it.
> 

Indeed, failing is used when there is something wrong with the module,
and this is not the case when (nr_devices == 0). I will remove this, thanks!

>> I did not tested with all possible conditions, but I tested with the
>> following ones:
>>
>> * Using a negative number of devices:
>> 	- Previously, it would alloc and add a huge number of devices until the
>> system gets out of memory
>> 	- With module_param as uint, it will throw a "invalid for parameter
>> `nr_devices'" and refuse to load
>>
>> * Using a range of values (1, 10, 100, 1000):
>> 	- It will works as expect, creating some /dev/nullbX accordingly with
>> your parameter. Works fine with and without this patch.
> 
> If you ask it to create 4 billion devices, what happens?  Obviously we'll
> run out of dev_t at some point ...
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-13 15:12           ` Matthew Wilcox
  2019-09-13 15:39             ` André Almeida
@ 2019-09-13 16:23             ` Jens Axboe
  2019-09-13 16:27               ` Chaitanya Kulkarni
  1 sibling, 1 reply; 17+ messages in thread
From: Jens Axboe @ 2019-09-13 16:23 UTC (permalink / raw)
  To: Matthew Wilcox, André Almeida
  Cc: Chaitanya Kulkarni, linux-block, linux-doc, corbet, m, kernel, krisman

On 9/13/19 9:12 AM, Matthew Wilcox wrote:
> On Fri, Sep 13, 2019 at 11:57:17AM -0300, André Almeida wrote:
>> On 9/12/19 7:20 PM, Chaitanya Kulkarni wrote:
>>> On 09/12/2019 03:09 PM, André Almeida wrote:
>>>> Hello Matthew,
>>>>
>>>> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
>>>>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>>>>>
>>>>>> -static int nr_devices = 1;
>>>>>> +static unsigned int nr_devices = 1;
>>>>>>    module_param(nr_devices, int, 0444);
>>>>>
>>>>> ^^^ you forgot to change the module_param to match
>>>>>
>>>>>> +	if (!nr_devices) {
>>>>>> +		pr_err("null_blk: invalid number of devices\n");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>
>>>>> I don't think this is necessary.
>>>>>
>>>>
>>>> Could you explain why you don't think is necessary? As I see, the module
>>>> can't be used without any /dev/nullb* device, so why we should load it?
>>>>
>>>> Thanks,
>>>> 	André
>>>>
>>>
>>> I think Matthew is right here. I think module can be loaded with
>>> nr_devices=0.
>>>
>>> Did you get a chance to test nr_device=0 condition ?
>>>
>>
>> Yes. It says "module loaded" and lsmod shows that it is loaded indeed.
>> But you don't have any /dev/nullb*, so you can't do much with the module.
>>
>> With this patch, it refuses to load the module.
> 
> Why is that an improvement?  I agree it's an uninteresting thing to ask
> for, but I don't see a compelling need to fail the module load because
> of it.

It also breaks the case of loading it, then setting up a new device
through configfs.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-13 16:23             ` Jens Axboe
@ 2019-09-13 16:27               ` Chaitanya Kulkarni
  2019-09-13 16:48                 ` Jens Axboe
  0 siblings, 1 reply; 17+ messages in thread
From: Chaitanya Kulkarni @ 2019-09-13 16:27 UTC (permalink / raw)
  To: Jens Axboe, Matthew Wilcox, André Almeida
  Cc: linux-block, linux-doc, corbet, m, kernel, krisman

On 09/13/2019 09:23 AM, Jens Axboe wrote:
> It also breaks the case of loading it, then setting up a new device
> through configfs.

Yep, this is exactly I was asking nr_device=0 as modparam is allowed
and membacked null_blk creation will fail with this check.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] null_blk: validated the number of devices
  2019-09-13 16:27               ` Chaitanya Kulkarni
@ 2019-09-13 16:48                 ` Jens Axboe
  0 siblings, 0 replies; 17+ messages in thread
From: Jens Axboe @ 2019-09-13 16:48 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Matthew Wilcox, André Almeida
  Cc: linux-block, linux-doc, corbet, m, kernel, krisman

On 9/13/19 10:27 AM, Chaitanya Kulkarni wrote:
> On 09/13/2019 09:23 AM, Jens Axboe wrote:
>> It also breaks the case of loading it, then setting up a new device
>> through configfs.
> 
> Yep, this is exactly I was asking nr_device=0 as modparam is allowed
> and membacked null_blk creation will fail with this check.
> 

Yeah, it's totally broken...

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-09-13 16:48 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 14:46 [PATCH 1/3] docs: block: null_blk: enhance document style André Almeida
2019-09-11 14:46 ` [PATCH 2/3] null_blk: fix module name at log message André Almeida
2019-09-12 15:47   ` Ezequiel Garcia
2019-09-12 20:47     ` Jens Axboe
2019-09-12 22:08       ` André Almeida
2019-09-11 14:46 ` [PATCH 3/3] null_blk: validated the number of devices André Almeida
2019-09-12 16:19   ` Matthew Wilcox
2019-09-12 22:07     ` André Almeida
2019-09-12 22:20       ` Chaitanya Kulkarni
2019-09-13 14:57         ` André Almeida
2019-09-13 15:12           ` Matthew Wilcox
2019-09-13 15:39             ` André Almeida
2019-09-13 16:23             ` Jens Axboe
2019-09-13 16:27               ` Chaitanya Kulkarni
2019-09-13 16:48                 ` Jens Axboe
2019-09-11 22:05 ` [PATCH 1/3] docs: block: null_blk: enhance document style Jens Axboe
2019-09-11 22:26   ` André Almeida

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).