From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: dm: fix false alarm in free_rq_clone() for non blk-mq target Date: Thu, 28 May 2015 23:18:42 -0400 Message-ID: <20150529031842.GA26268@redhat.com> References: <5566CAE8.4070404@ce.jp.nec.com> <20150528191457.GA26218@redhat.com> <5567B2D2.5080907@ce.jp.nec.com> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <5567B2D2.5080907@ce.jp.nec.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Junichi Nomura Cc: device-mapper development List-Id: dm-devel.ids On Thu, May 28 2015 at 8:29pm -0400, Junichi Nomura wrote: > On 05/29/15 04:14, Mike Snitzer wrote: > > On Thu, May 28 2015 at 3:59am -0400, > > Junichi Nomura 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: > >> [] dump_stack+0x45/0x57 > >> [] warn_slowpath_common+0x8a/0xc0 > >> [] warn_slowpath_null+0x1a/0x20 > >> [] free_rq_clone+0x7a/0xf0 [dm_mod] > >> [] dm_softirq_done+0x13c/0x250 [dm_mod] > >> [] blk_done_softirq+0x80/0xa0 > >> [] __do_softirq+0xf4/0x2d0 > >> [] irq_exit+0x125/0x130 > >> [] smp_call_function_single_interrupt+0x35/0x40 > >> [] call_function_single_interrupt+0x6e/0x80 > >> [] ? native_safe_halt+0x6/0x10 > >> [] ? rcu_eqs_enter+0x68/0x90 > >> [] default_idle+0x1e/0xc0 > >> [] arch_cpu_idle+0xf/0x20 > >> [] cpu_startup_entry+0x2fc/0x3a0 > >> [] 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?