Re: [for-4.16 PATCH v2 2/3] block: cope with gendisk's 'queue' being added later

[Mike Snitzer <snitzer@redhat.com>]

On Wed, Jan 10 2018 at  2:55am -0500,
Ming Lei <tom.leiming@gmail.com> wrote:

> On Wed, Jan 10, 2018 at 12:21 PM, Mike Snitzer <snitzer@redhat.com> wrote:
> > On Tue, Jan 09 2018 at 10:46pm -0500,
> > Ming Lei <tom.leiming@gmail.com> wrote:
> >
> >> Another related issue is that blk-mq debugfs can't work yet for dm-mpath.
> >
> > Not sure how that is a related issue to this thread.
> > But can you expand on that?  I'm not familiar with "blk-mq debugfs".
> > Any pointer to code that activates it?  Could be that my changes make it
> > work now...
> 
> Hi,
> 
> blk_mq_debugfs_register() is called in blk_register_queue() from add_disk(),
> and blk_mq_debugfs_register() need queue's information to setup mq debugfs
> stuff.
> 
> But your patch does fix this issue, cool!

Great, thought it might.
 
> >> So I am just wondering if it is possible to follow the usual way to add disk
> >> after setting up queue for DM? If it is possible, it may avoid lots of trouble
> >> for us.
> >
> > As I explained in the header (quoted above actually) it is _not_
> > possible.  It is the gendisk that enables the device stack to be
> > constructed (at least how DM's stacking is currently implemented with
> > bd_link_disk_holder() and taking references on devices found in a DM
> > device's table).  And from that gendisk I can then walk the devices to
> > establish the request_queue as needed, its stacked limits, etc.
> >
> > The approach I've taken in these patches is the closest I've gotten to
> > make DM _much_ more sane about how its request_queue is established.
> > But its still off the beaten path due to needing "block: cope with
> > gendisk's 'queue' being added later".
> 
> OK, thanks for your explanation.
> 
> After taking close look at your patches, I think this approach is clean.
> But there are still corner cases which need to be addressed:
> 
> 1) in the following disk attributes, queue is referenced, so we have
> to add check in their show/store operations since the attributes
> are shown up just after disk is added.
> 
>      disk_alignment_offset_show
>      disk_discard_alignment_show
>      part_timeout_show/part_timeout_store
> 
> 2) same issue on /proc/diskstats: see diskstats_show()
> 
> 3) in IO path, looks we already checked disk->queue in
> generic_make_request_checks(), so it should be fine, and you set
> disk->queue after the queue is fully initialized in the 3rd patch.

I'll work through these and see what I can do.

Will likely deal with each independent of the 2/3 patch (otherwise
if I just keep folding changes in to the original patch review will get
too hard).

So hopefully I'll have a v3 to share later today.

Thanks,
Mike