Re: dm: fix false alarm in free_rq_clone() for non blk-mq target

[Mike Snitzer <snitzer@redhat.com>]

On Thu, May 28 2015 at  8:29pm -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> On 05/29/15 04:14, Mike Snitzer wrote:
> > On Thu, May 28 2015 at  3:59am -0400,
> > Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
> > 
> >> When stacking request-based dm device on non blk-mq device
> >> and device-mapper target could not map the request
> >> (error target is used, multipath target with all paths down, etc),
> >> following warning will show up once:
> >>
> >>   WARNING: CPU: 7 PID: 0 at drivers/md/dm.c:1090 free_rq_clone+0x7a/0xf0
> >>   CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.1.0-rc5+ #1
> >>   ..
> >>   Call Trace:
> >>    <IRQ>  [<ffffffff81558095>] dump_stack+0x45/0x57
> >>    [<ffffffff8105ef3a>] warn_slowpath_common+0x8a/0xc0
> >>    [<ffffffff8105f02a>] warn_slowpath_null+0x1a/0x20
> >>    [<ffffffffa020bf3a>] free_rq_clone+0x7a/0xf0 [dm_mod]
> >>    [<ffffffffa020c7cc>] dm_softirq_done+0x13c/0x250 [dm_mod]
> >>    [<ffffffff812b87a0>] blk_done_softirq+0x80/0xa0
> >>    [<ffffffff81062f44>] __do_softirq+0xf4/0x2d0
> >>    [<ffffffff81063425>] irq_exit+0x125/0x130
> >>    [<ffffffff81037f05>] smp_call_function_single_interrupt+0x35/0x40
> >>    [<ffffffff8156027e>] call_function_single_interrupt+0x6e/0x80
> >>    <EOI>  [<ffffffff8104b576>] ? native_safe_halt+0x6/0x10
> >>    [<ffffffff810b6348>] ? rcu_eqs_enter+0x68/0x90
> >>    [<ffffffff8100d6fe>] default_idle+0x1e/0xc0
> >>    [<ffffffff8100e1df>] arch_cpu_idle+0xf/0x20
> >>    [<ffffffff810a269c>] cpu_startup_entry+0x2fc/0x3a0
> >>    [<ffffffff81038892>] start_secondary+0x182/0x1b0
> >>
> >> The warning was added by commit aa6df8dd28c0 ("dm: fix free_rq_clone()
> >> NULL pointer when requeueing unmapped request").
> >>
> >> But free_rq_clone() with clone->q == NULL is valid usage for non blk-mq
> >> underlying device. Such a call can happen via dm_kill_unmapped_request().
> > 
> > dm_kill_unmapped_request() is called from the blk-mq error path too
> 
> Yes, but:
> 
> > (e.g. if clone_and_map_rq fails with an error).  So this isn't non
> > blk-mq specific.
> 
> unmapped request does not have clone in the case of blk-mq?
> (as the clone_and_map_rq() API suggests)
> 
> Then dm_softirq_done(orig) for dm_kill_unmapped_request() will fall
> into 'if (!clone)' path.
> Thus neither of dm_done(clone), dm_end_request(clone) nor free_rq_clone()
> will be called.

Very true, not sure how I overlooked that.

> > dm_kill_unmapped_request() sets REQ_FAILED, which dm_softirq_done()
> > checks for and will set 'mapped' to false.  I think a proper fix is to
> > pass 'mapped' into dm_end_request() and then pass it to free_rq_clone().
> > Like so, what do you think?
> 
> So I don't think it's necessary to extend dm_end_request() with 'mapped'
> parameter.

I'm starting to question the need for 'must_be_mapped' param to
free_rq_clone().  It was motivated by strange reports from Bart's
testing but in reality I don't think it ever actually helped.

It was to act as a canary in the coal mine for the future but I'm now
more inclined to just remove the parameter entirely.  Or do you think
'must_be_mapped' is still useful?