[PATCH 0/1] random vs blk-mq

[PATCH 0/1] random vs blk-mq

[Christoph Hellwig]

A tester found out that we need add_disk_randomness for my scsi-mq
branch.  The patch applied exports add_disk_randomness so that modular
scsi still works with a driver that either uses the low-level I/O
completion routines for the old block layer, or blk-mq, and I'd love
to get this in through Jens' tree which has all the other block work
scsi-mq requires.

But this also brings up an interesting question:  blk-mq currently
does not set QUEUE_FLAG_ADD_RANDOM in the default queue flags, so
simply converting a driver to blk-mq will mean it stops contributing
to the random pool.  Do we need a more fine grained way to control
this, especially for SCSI?

[PATCH 1/1] random: export add_disk_randomness

[Christoph Hellwig]

This will be needed for pending changes to the scsi midlayer that now calls
lower level block APIs, as well as any blk-mq driver that wants to contribute
to the random pool.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/char/random.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 6b75713..0a19d86 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -902,6 +902,7 @@ void add_disk_randomness(struct gendisk *disk)
 	add_timer_randomness(disk->random, 0x100 + disk_devt(disk));
 	trace_add_disk_randomness(disk_devt(disk), ENTROPY_BITS(&input_pool));
 }
+EXPORT_SYMBOL_GPL(add_disk_randomness);
 #endif
 
 /*********************************************************************
-- 
1.7.10.4

Re: [PATCH 1/1] random: export add_disk_randomness

[Theodore Ts'o]

On Fri, Apr 25, 2014 at 12:36:37AM -0700, Christoph Hellwig wrote:
> This will be needed for pending changes to the scsi midlayer that now calls
> lower level block APIs, as well as any blk-mq driver that wants to contribute
> to the random pool.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: "Theodore Ts'o" <tytso@mit.edu>

Re: [PATCH 1/1] random: export add_disk_randomness

[Christoph Hellwig]

On Fri, Apr 25, 2014 at 09:49:51AM -0400, Theodore Ts'o wrote:
> On Fri, Apr 25, 2014 at 12:36:37AM -0700, Christoph Hellwig wrote:
> > This will be needed for pending changes to the scsi midlayer that now calls
> > lower level block APIs, as well as any blk-mq driver that wants to contribute
> > to the random pool.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: "Theodore Ts'o" <tytso@mit.edu>

Jens, can you pull this into the block tree for me?

Re: [PATCH 1/1] random: export add_disk_randomness

[Jens Axboe]

On 04/28/2014 05:29 AM, Christoph Hellwig wrote:
> On Fri, Apr 25, 2014 at 09:49:51AM -0400, Theodore Ts'o wrote:
>> On Fri, Apr 25, 2014 at 12:36:37AM -0700, Christoph Hellwig wrote:
>>> This will be needed for pending changes to the scsi midlayer that now calls
>>> lower level block APIs, as well as any blk-mq driver that wants to contribute
>>> to the random pool.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Acked-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> Jens, can you pull this into the block tree for me?

Yep, I'll add it, thanks.

-- 
Jens Axboe

Re: [PATCH 0/1] random vs blk-mq

[Theodore Ts'o]

On Fri, Apr 25, 2014 at 12:36:10AM -0700, Christoph Hellwig wrote:
> But this also brings up an interesting question:  blk-mq currently
> does not set QUEUE_FLAG_ADD_RANDOM in the default queue flags, so
> simply converting a driver to blk-mq will mean it stops contributing
> to the random pool.  Do we need a more fine grained way to control
> this, especially for SCSI?

In general, more fine grained control is always good.

But getting the defaults right is even more important.  It occurs to
me that what might make sense is to turn on QUEUE_FLAG_ADD_RANDOM if
we know that it is a rotational disk.  But in the case where we know
it's a SSD or a NVMe, it's likely that add_disk_randomness() is not
going to do much in the way that's useful, since we estimate entropy
credits based on the jiffies delta, so if we interrupts more
frequently than the 10ms, we're not going to get any entropy credit
anyway.

Besides, we are sampling all interrupts, so we'll be gathering timing
information and granting a minimal amount of entropy credit (on
average 1/64'th of a bit of entropy per interupt) for each disk
interrupt evenw/o the QUEUE_FLAG_ADD_RANDOM.  The only reason to call
add_disk_randomness() is if we want to give credit for the higher
grade of entropy theoretically available from a disk interrupt ---
which would only be true if we're talking about a rotational storage
device anyway.

							- Ted

Re: [PATCH 0/1] random vs blk-mq

[Jens Axboe]

On 04/25/2014 08:03 AM, Theodore Ts'o wrote:
> On Fri, Apr 25, 2014 at 12:36:10AM -0700, Christoph Hellwig wrote:
>> But this also brings up an interesting question:  blk-mq currently
>> does not set QUEUE_FLAG_ADD_RANDOM in the default queue flags, so
>> simply converting a driver to blk-mq will mean it stops contributing
>> to the random pool.  Do we need a more fine grained way to control
>> this, especially for SCSI?
> 
> In general, more fine grained control is always good.
> 
> But getting the defaults right is even more important.  It occurs to
> me that what might make sense is to turn on QUEUE_FLAG_ADD_RANDOM if
> we know that it is a rotational disk.  But in the case where we know
> it's a SSD or a NVMe, it's likely that add_disk_randomness() is not
> going to do much in the way that's useful, since we estimate entropy
> credits based on the jiffies delta, so if we interrupts more
> frequently than the 10ms, we're not going to get any entropy credit
> anyway.

This is exactly why the flag was added in the first place:

commit e2e1a148bc45855816ae6b4692ce29d0020fa22e
Author: Jens Axboe <jaxboe@fusionio.com>
Date:   Wed Jun 9 10:42:09 2010 +0200

    block: add sysfs knob for turning off disk entropy contributions

    There are two reasons for doing this:

    - On SSD disks, the completion times aren't as random as they
      are for rotational drives. So it's questionable whether they
      should contribute to the random pool in the first place.

    - Calling add_disk_randomness() has a lot of overhead.

-- 
Jens Axboe

Re: [PATCH 0/1] random vs blk-mq

[Jens Axboe]

On 04/25/2014 01:36 AM, Christoph Hellwig wrote:
> A tester found out that we need add_disk_randomness for my scsi-mq
> branch.  The patch applied exports add_disk_randomness so that modular
> scsi still works with a driver that either uses the low-level I/O
> completion routines for the old block layer, or blk-mq, and I'd love
> to get this in through Jens' tree which has all the other block work
> scsi-mq requires.
> 
> But this also brings up an interesting question:  blk-mq currently
> does not set QUEUE_FLAG_ADD_RANDOM in the default queue flags, so
> simply converting a driver to blk-mq will mean it stops contributing
> to the random pool.  Do we need a more fine grained way to control
> this, especially for SCSI?

Or, we could just fix the horribly slow pool mix and have it always be
on if it's "free", or almost free. Perhaps just default to on for
rotating devices, that'd lessen the impact.

-- 
Jens Axboe