ACK/Cmnt: [PATCH F/Unstable 1/1] UBUNTU: SAUCE: blk/core: Gracefully handle unset make_request_fn

[Andrea Righi]

On Thu, Jan 23, 2020 at 11:24:54AM +0200, Stefan Bader wrote:
> On 23.01.20 11:13, Andrea Righi wrote:
> > On Thu, Jan 23, 2020 at 10:33:03AM +0200, Stefan Bader wrote:
> >> BugLink: https://bugs.launchpad.net/bugs/1860231
> >>
> >> When device-mapper adapted for multi-queue functionality, they
> >> also re-organized the way the make-request function was set.
> >> Before, this happened when the device-mapper logical device was
> >> created. Now it is done once the mapping table gets loaded the
> >> first time (this also decides whether the block device is request
> >> or bio based).
> >>
> >> However in generic_make_request(), the request function gets used
> >> without further checks and this happens if one tries to mount such
> >> a partially set up device.
> >>
> >> This can easily be reproduced with the following steps:
> >>  - dmsetup create -n test
> >>  - mount /dev/dm-<#> /mnt
> >>
> >> This maybe is something which also should be fixed up in device-
> >> mapper. But given there is already a check for an unset queue
> >> pointer and potentially there could be other drivers which do or
> >> might do the same, it sounds like a good move to add another check
> >> to generic_make_request_checks() and to bail out if the request
> >> function has not been set, yet.
> >>
> >> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
> >> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
> >> ---
> >>  block/blk-core.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index 1075aaff606d..adcd042edd2d 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
> >>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
> >>  		goto end_io;
> >>  	}
> >> +	if (unlikely(!q->make_request_fn)) {
> >> +		printk(KERN_ERR
> >> +		       "generic_make_request: Trying to access "
> >> +		       "block-device without request function: %s\n",
> >> +		       bio_devname(bio, b));
> >> +		goto end_io;
> >> +	}
> > 
> > Instead of returning an I/O error, I was wondering if it would be better
> > to give a chance to the caller to repeat the I/O request gracefully,
> > returning -EAGAIN. Something like this:
> 
> This potentially can take some time (potentially).Like in the example given you
> create a device without that mapping function until you load the table. That
> might be much later and maybe by that time there are already a lot of I/O
> requests in flight. Plus I basically repeated the handling which is done when
> you submit I/O to a device which has no request queue set up at all.

Hm... yeah, makes sense, then failing here is probably a safer action.

Thanks,
-Andrea