On Mon, Feb 26 2018 at 7:13pm -0500, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > On Mon, Feb 26 2018 at 5:56pm -0500, > Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote: > > > The block layer provides a function direct_make_request - but it doesn't > > provide any test whether this function may be used or not. > > > > Device mapper currently uses a dirty trick - it compares the result of > > bdevname against the string "nvme" to test if direct_make_request can be > > used. > > dm's DM_TYPE_NVME_BIO_BASED is backed by that crude "nvme" check _but_ > it is a means to an end. Alternative would be to have NVMe (and other > devices) set something like QUEUE_FLAG_NO_PARTIAL_COMPLETION or > something -- and then DM would need to verify all underlying devices set > that flag. > > > The information whether the driver will or won't recurse can be easily > > obtained in generic_make_request (by comparing make_request_fn - or > > alternatively, we could introduce a queue flag for this), so my suggestion > > is to just delete direct_make_request and fold this "norecurse" > > optimization directly into generic_make_request This will allow us to > > simplify device mapper paths and get rid of device name matching. > > Sorry, Nack from me as implemented. Please see below. As we discussed, but for the benefit of others: I was mistaken.. I reviewed your proposal too quickly. > > Index: linux-2.6/block/blk-core.c > > =================================================================== > > --- linux-2.6.orig/block/blk-core.c 2018-02-26 20:37:19.088499000 +0100 > > +++ linux-2.6/block/blk-core.c 2018-02-26 20:43:57.839999000 +0100 > > @@ -2265,6 +2265,8 @@ end_io: > > return false; > > } > > > > +blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio); > > + > > /** > > * generic_make_request - hand a buffer to its device driver for I/O > > * @bio: The bio describing the location in memory and on the device. > > @@ -2300,10 +2302,14 @@ blk_qc_t generic_make_request(struct bio > > */ > > struct bio_list bio_list_on_stack[2]; > > blk_qc_t ret = BLK_QC_T_NONE; > > + bool may_recurse; > > > > if (!generic_make_request_checks(bio)) > > goto out; > > > > + may_recurse = bio->bi_disk->queue->make_request_fn != blk_queue_bio && > > + bio->bi_disk->queue->make_request_fn != blk_mq_make_request; > > + > > This does _not_ allow dm's DM_TYPE_NVME_BIO_BASED to make use of your > direct_make_request() equivalent that you've encoded into > generic_make_request(). > > There is no easy way to _know_, at runtime, that an arbitrary queue > (e.g. stacked DM device) does _not_ split IO (therefore not needing to > recurse). > > Same can be said of NVMe given NVMe multipath sets its make_request_fn > to nvme_ns_head_make_request(). > > So for both cases that don't need to recurse you're setting @may_recurse > to true. Both DM multipath and NVMe native multipath will change the bio->bi_disk to the underlying NVMe device (which has make_request_fn set to blk_mq_make_request). SO: @may_recurse is set to false for both cases. Sorry for tainting your work, I retract my Nack. Avoiding the need for drivers to reason about whether it best to call direct_make_request() vs generic_make_request() is a welcome advance. I think you plan on submitting a v2 that avoids the make_request_fn check. I'll review that much more carefully before commenting ;) Thanks, Mike -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
