Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Junichi Nomura]

On 01/25/17 01:39, Mike Snitzer wrote:
> On Tue, Jan 24 2017 at  9:20am -0500, Christoph Hellwig  wrote:
>> On Tue, Jan 24, 2017 at 05:05:39AM -0500, Mike Snitzer wrote:
>>> possible and is welcomed cleanup.  The only concern I have is that using
>>> get_request() for the old request_fn request_queue eliminates the
>>> guaranteed availability of requests to allow for forward progress (on
>>> path failure or for the purposes of swap over mpath, etc).  This isn't a
>>> concern for blk-mq because as you know we have a fixed set of tags (and
>>> associated preallocated requests).
>>>
>>> So I'm left unconvinced old request_fn request-based DM multipath isn't
>>> regressing in how request resubmission can be assured a request will be
>>> available when needed on retry in the face of path failure.
>>
>> Mempool only need a size where we can make guaranteed requests, so for
>> get_request based drivers under dm the theoretical minimum size would be
>> one as we never rely on a second request to finish the first one,
>> and each request_queue has it's own mempool(s) to start with.
> 
> Fair enough.  Cc'ing Junichi just in case he sees anything we're
> missing.

DM multipath could not use blk_get_request() because the function
was not callable from interrupt-disabled context. E.g. request_fn.

However, since the current code no longer calls blk_get_request()
from such a context, the change should be ok.

-- 
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 11:39:51AM -0500, Mike Snitzer wrote:
> Fair enough.  Cc'ing Junichi just in case he sees anything we're
> missing.

Thanks, I'll wait for his repsonse before reposting with the few accumulated
changes (including the dm cleanup).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Mike Snitzer]

On Tue, Jan 24 2017 at  9:20am -0500,
Christoph Hellwig  wrote:

> On Tue, Jan 24, 2017 at 05:05:39AM -0500, Mike Snitzer wrote:
> > possible and is welcomed cleanup.  The only concern I have is that using
> > get_request() for the old request_fn request_queue eliminates the
> > guaranteed availability of requests to allow for forward progress (on
> > path failure or for the purposes of swap over mpath, etc).  This isn't a
> > concern for blk-mq because as you know we have a fixed set of tags (and
> > associated preallocated requests).
> > 
> > So I'm left unconvinced old request_fn request-based DM multipath isn't
> > regressing in how request resubmission can be assured a request will be
> > available when needed on retry in the face of path failure.
> 
> Mempool only need a size where we can make guaranteed requests, so for
> get_request based drivers under dm the theoretical minimum size would be
> one as we never rely on a second request to finish the first one,
> and each request_queue has it's own mempool(s) to start with.

Fair enough.  Cc'ing Junichi just in case he sees anything we're
missing.
 
> > dm_mod's 'reserved_rq_based_ios' module_param governs the minimum number
> > of requests in the md->rq_pool (and defaults to 256 requests per
> > request-based DM request_queue).  Whereas blk_init_rl()'s
> > mempool_create_node() uses BLKDEV_MIN_RQ (4) yet q->nr_requests =
> > BLKDEV_MAX_RQ (128).  Also, this patch eliminates the utility of
> > 'reserved_rq_based_ios' module_param without actually removing it.
> 
> It's still used for the bio pool, so I couldn't simply remove it.

Ah, yeah, you're talking about:
pools->bs = bioset_create_nobvec(pool_size, front_pad);

Makes sense.

> > Anyway, should blk-core evolve to allow drivers to specify a custom
> > min_nr of requests in the old request_fn request_queue's mempool?  Or is
> > my concern overblown?
> 
> Thanks to mempool_resize we could add that functionality.  I just don't
> see an actual need for it.
> 
> > p.s. dm.c:dm_alloc_md_mempools() could be cleaned up a bit more since
> > only bio-based DM will have a pools->io_pool moving forward; but I can
> > circle back to that cleanup after.
> 
> If you're fine with doing more than the necessary changes in a patch
> that needs to got into the block tree I'd be happy to apply my usual
> cleanup magic to it.

I think this one example illustrates cleanup that makes sense even if
going through block.  While in there it is best to not leave extra
branching that just doesn't make sense to have anymore.  So if you do
another version of this patchset feel free to move this direct into
dm_alloc_md_mempools()'s BIO_BASED case:
pools->io_pool = mempool_create_slab_pool(pool_size, _io_cache);
And eliminate the cachep pointer.

But if not that is fine too.

Also:

Reviewed-by: Mike Snitzer 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 05:05:39AM -0500, Mike Snitzer wrote:
> possible and is welcomed cleanup.  The only concern I have is that using
> get_request() for the old request_fn request_queue eliminates the
> guaranteed availability of requests to allow for forward progress (on
> path failure or for the purposes of swap over mpath, etc).  This isn't a
> concern for blk-mq because as you know we have a fixed set of tags (and
> associated preallocated requests).
> 
> So I'm left unconvinced old request_fn request-based DM multipath isn't
> regressing in how request resubmission can be assured a request will be
> available when needed on retry in the face of path failure.

Mempool only need a size where we can make guaranteed requests, so for
get_request based drivers under dm the theoretical minimum size would be
one as we never rely on a second request to finish the first one,
and each request_queue has it's own mempool(s) to start with.

> dm_mod's 'reserved_rq_based_ios' module_param governs the minimum number
> of requests in the md->rq_pool (and defaults to 256 requests per
> request-based DM request_queue).  Whereas blk_init_rl()'s
> mempool_create_node() uses BLKDEV_MIN_RQ (4) yet q->nr_requests =
> BLKDEV_MAX_RQ (128).  Also, this patch eliminates the utility of
> 'reserved_rq_based_ios' module_param without actually removing it.

It's still used for the bio pool, so I couldn't simply remove it.

> Anyway, should blk-core evolve to allow drivers to specify a custom
> min_nr of requests in the old request_fn request_queue's mempool?  Or is
> my concern overblown?

Thanks to mempool_resize we could add that functionality.  I just don't
see an actual need for it.

> p.s. dm.c:dm_alloc_md_mempools() could be cleaned up a bit more since
> only bio-based DM will have a pools->io_pool moving forward; but I can
> circle back to that cleanup after.

If you're fine with doing more than the necessary changes in a patch
that needs to got into the block tree I'd be happy to apply my usual
cleanup magic to it.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Mike Snitzer]

On Mon, Jan 23 2017 at 10:29am -0500,
Christoph Hellwig  wrote:

> DM already calls blk_mq_alloc_request on the request_queue of the
> underlying device if it is a blk-mq device.  But now that we allow drivers
> to allocate additional data and initialize it ahead of time we need to do
> the same for all drivers.   Doing so and using the new cmd_size
> infrastructure in the block layer greatly simplifies the dm-rq and mpath
> code, and should also make arbitrary combinations of SQ and MQ devices
> with SQ or MQ device mapper tables easily possible as a further step.

Thanks for working (and suffering) through all of this request-based DM
code.  Nice to have someone else be painfully aware of the complexity in
request-based DM's old request_fn support.

The queue->cmd_size (per request data) definitely makes this more
possible and is welcomed cleanup.  The only concern I have is that using
get_request() for the old request_fn request_queue eliminates the
guaranteed availability of requests to allow for forward progress (on
path failure or for the purposes of swap over mpath, etc).  This isn't a
concern for blk-mq because as you know we have a fixed set of tags (and
associated preallocated requests).

So I'm left unconvinced old request_fn request-based DM multipath isn't
regressing in how request resubmission can be assured a request will be
available when needed on retry in the face of path failure.

dm_mod's 'reserved_rq_based_ios' module_param governs the minimum number
of requests in the md->rq_pool (and defaults to 256 requests per
request-based DM request_queue).  Whereas blk_init_rl()'s
mempool_create_node() uses BLKDEV_MIN_RQ (4) yet q->nr_requests =
BLKDEV_MAX_RQ (128).  Also, this patch eliminates the utility of
'reserved_rq_based_ios' module_param without actually removing it.

Anyway, should blk-core evolve to allow drivers to specify a custom
min_nr of requests in the old request_fn request_queue's mempool?  Or is
my concern overblown?

Seems we're very close to making this request-based DM cleanup doable.
Just would like some extra eyes and care/thought/guidance from yourself
and likely Jens.

Thanks,
Mike

p.s. dm.c:dm_alloc_md_mempools() could be cleaned up a bit more since
only bio-based DM will have a pools->io_pool moving forward; but I can
circle back to that cleanup after.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/16] dm: always defer request allocation to the owner of the request_queue

[Hannes Reinecke]

On 01/23/2017 04:29 PM, Christoph Hellwig wrote:
> DM already calls blk_mq_alloc_request on the request_queue of the
> underlying device if it is a blk-mq device.  But now that we allow drivers
> to allocate additional data and initialize it ahead of time we need to do
> the same for all drivers.   Doing so and using the new cmd_size
> infrastructure in the block layer greatly simplifies the dm-rq and mpath
> code, and should also make arbitrary combinations of SQ and MQ devices
> with SQ or MQ device mapper tables easily possible as a further step.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/md/dm-core.h  |   1 -
>  drivers/md/dm-mpath.c | 132 --
>  drivers/md/dm-rq.c| 251 
> ++
>  drivers/md/dm-rq.h|   2 +-
>  drivers/md/dm-target.c|   7 --
>  drivers/md/dm.c   |  18 +--
>  drivers/md/dm.h   |   3 +-
>  include/linux/device-mapper.h |   3 -
>  8 files changed, 81 insertions(+), 336 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel