Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752648AbbL1SxK (ORCPT ); Mon, 28 Dec 2015 13:53:10 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:15135 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751396AbbL1SxI (ORCPT ); Mon, 28 Dec 2015 13:53:08 -0500 Subject: Re: [PATCH] null_blk: don't enable irqs when in irq To: Rabin Vincent References: <1451053594-12188-1-git-send-email-rabin@rab.in> <567E1180.8070004@fb.com> CC: From: Jens Axboe Message-ID: <568184E8.1080801@fb.com> Date: Mon, 28 Dec 2015 11:52:24 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <567E1180.8070004@fb.com> Content-Type: multipart/mixed; boundary="------------010103070701000500030602" X-Originating-IP: [192.168.54.13] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2015-12-28_12:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4081 Lines: 111 --------------010103070701000500030602 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit On 12/25/2015 09:03 PM, Jens Axboe wrote: > On 12/25/2015 07:26 AM, Rabin Vincent wrote: >> When using irq_mode=NULL_IRQ_TIMER, blk_start_queue() is called from the >> hrtimer interrupt. null_request_fn() calls spin_unlock_irq() and this >> causes the following splat from lockdep since interrupts are being >> enabled while we're in an interrupt handler. >> >> When we're in null_request_fn() we can't know the status of the flags >> saved before blk_start_queue() was called, but we can use in_irq() to >> conditionally enable interrupts only if we're not in a hard interrupt in >> order to handle this case. > > Using in_irq() to check for this is... nasty. You have two choices here. > Either you don't enable interrupts ever. That's safe from the > perspective of the driver, since we don't block in handling the command. > That means just unconditionally using spin_unlock() in the request_fn. > Or you punt queue running to process context, by manually clearing the > queue stopped flag and using blk_run_queue_async() instead. Something like this should work, can you test it? -- Jens Axboe --------------010103070701000500030602 Content-Type: text/x-patch; name="null-restart.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="null-restart.patch" diff --git a/block/blk-core.c b/block/blk-core.c index c487b94c59e3..33e2f62d5062 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -207,6 +207,22 @@ void blk_delay_queue(struct request_queue *q, unsigned long msecs) EXPORT_SYMBOL(blk_delay_queue); /** + * blk_start_queue_async - asynchronously restart a previously stopped queue + * @q: The &struct request_queue in question + * + * Description: + * blk_start_queue_async() will clear the stop flag on the queue, and + * ensure that the request_fn for the queue is run from an async + * context. + **/ +void blk_start_queue_async(struct request_queue *q) +{ + queue_flag_clear(QUEUE_FLAG_STOPPED, q); + blk_run_queue_async(q); +} +EXPORT_SYMBOL(blk_start_queue_async); + +/** * blk_start_queue - restart a previously stopped queue * @q: The &struct request_queue in question * diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c index a428e4ef71fd..69064e1f70b0 100644 --- a/drivers/block/null_blk.c +++ b/drivers/block/null_blk.c @@ -232,20 +232,14 @@ static void end_cmd(struct nullb_cmd *cmd) break; case NULL_Q_BIO: bio_endio(cmd->bio); - goto free_cmd; + break; } - /* Restart queue if needed, as we are freeing a tag */ - if (q && !q->mq_ops && blk_queue_stopped(q)) { - unsigned long flags; - - spin_lock_irqsave(q->queue_lock, flags); - if (blk_queue_stopped(q)) - blk_start_queue(q); - spin_unlock_irqrestore(q->queue_lock, flags); - } -free_cmd: free_cmd(cmd); + + /* Restart queue if needed, as we are freeing a tag */ + if (queue_mode == NULL_Q_RQ && blk_queue_stopped(q)) + blk_start_queue_async(q); } static enum hrtimer_restart null_cmd_timer_expired(struct hrtimer *timer) diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 0169ba2e2e64..c70e3588a48c 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -797,6 +797,7 @@ extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t, extern int blk_queue_enter(struct request_queue *q, gfp_t gfp); extern void blk_queue_exit(struct request_queue *q); extern void blk_start_queue(struct request_queue *q); +extern void blk_start_queue_async(struct request_queue *q); extern void blk_stop_queue(struct request_queue *q); extern void blk_sync_queue(struct request_queue *q); extern void __blk_stop_queue(struct request_queue *q); --------------010103070701000500030602-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/