From patchwork Thu Aug 11 03:01:05 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Junichi Nomura X-Patchwork-Id: 1055632 Received: from mx3-phx2.redhat.com (mx3-phx2.redhat.com [209.132.183.24]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p7B3EgDR003680 for ; Thu, 11 Aug 2011 03:15:03 GMT Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p7B3CHu2020019; Wed, 10 Aug 2011 23:12:20 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p7B3CGqb008349 for ; Wed, 10 Aug 2011 23:12:16 -0400 Received: from mx1.redhat.com (ext-mx11.extmail.prod.ext.phx2.redhat.com [10.5.110.16]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7B3CAj2027290 for ; Wed, 10 Aug 2011 23:12:10 -0400 Received: from tyo201.gate.nec.co.jp (TYO201.gate.nec.co.jp [202.32.8.193]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7B3C9Ya021397 for ; Wed, 10 Aug 2011 23:12:09 -0400 Received: from mailgate3.nec.co.jp ([10.7.69.192]) by tyo201.gate.nec.co.jp (8.13.8/8.13.4) with ESMTP id p7B3C0q9021262; Thu, 11 Aug 2011 12:12:00 +0900 (JST) Received: (from root@localhost) by mailgate3.nec.co.jp (8.11.7/3.7W-MAILGATE-NEC) id p7B3C0r10408; Thu, 11 Aug 2011 12:12:00 +0900 (JST) Received: from mail03.kamome.nec.co.jp (mail03.kamome.nec.co.jp [10.25.43.7]) by mailsv.nec.co.jp (8.13.8/8.13.4) with ESMTP id p7B3C0PT017698; Thu, 11 Aug 2011 12:12:00 +0900 (JST) Received: from shoin.jp.nec.com ([10.26.220.3] [10.26.220.3]) by mail01b.kamome.nec.co.jp with ESMTP id BT-MMP-320794; Thu, 11 Aug 2011 12:01:06 +0900 Received: from xzibit.linux.bs1.fc.nec.co.jp ([10.34.125.164] [10.34.125.164]) by mail.jp.nec.com with ESMTP; Thu, 11 Aug 2011 12:01:05 +0900 Message-ID: <4E4345F1.9040501@ce.jp.nec.com> Date: Thu, 11 Aug 2011 12:01:05 +0900 From: "Jun'ichi Nomura" User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc14 Thunderbird/3.1.10 MIME-Version: 1.0 To: James Bottomley References: <4E420941.3080601@ce.jp.nec.com> <1313005933.2785.12.camel@mulgrave> <4E432122.9050101@ce.jp.nec.com> In-Reply-To: <4E432122.9050101@ce.jp.nec.com> X-RedHat-Spam-Score: 0 () X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-Scanned-By: MIMEDefang 2.68 on 10.5.110.16 X-loop: dm-devel@redhat.com Cc: Kiyoshi Ueda , linux-scsi@vger.kernel.org, jaxboe@fusionio.com, "linux-kernel@vger.kernel.org" , roland@purestorage.com, device-mapper development , stern@rowland.harvard.edu Subject: Re: [dm-devel] [BUG] Oops when SCSI device under multipath is removed X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Thu, 11 Aug 2011 03:15:03 +0000 (UTC) Hi James, On 08/11/11 09:24, Jun'ichi Nomura wrote: > On 08/11/11 04:52, James Bottomley wrote: >> On Wed, 2011-08-10 at 13:29 +0900, Jun'ichi Nomura wrote: >>> 2) SCSI to call blk_cleanup_queue() from device's ->release() callback >>> (before 2.6.39, it used to work like this) >>> https://lkml.org/lkml/2011/7/2/106 >> >> Well, they both have documented objections. I asked why we destroy the >> elevator in the del case and didn't get any traction, so let me show the >> actual patch which should fix all of these issues. >> >> Is there a good reason for not doing this as a bug fix now? ... > I think it doesn't work because elevator_exit() and > blk_throtl_exit() take &q->queue_lock, which may be freed > by LLD after blk_cleanup_queue, before blk_release_queue. If the reason you moved scsi_free_queue into scsi_remove_device is marking the queue dead, how about the following patch? Do you think it's acceptable? Jun'ichi Nomura, NEC Corporation Add blk_kill_queue() for drivers which want to mark the queue dead early. blk_cleanup_queue() is an interface for LLD to notify block layer that LLD no longer needs the queue. Since q->queue_lock may point to a structure in LLD which is freed after blk_cleanup_queue, blk_cleanup_queue() frees subordinate structures like elevator, which uses q->queue_lock, to avoid invalid reference. OTOH, LLD like SCSI wants to just mark the queue dead earlier in tear down phase. So this patch factors out the early part of blk_cleanup_queue into blk_kill_queue for such drivers. --- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel --- linux-3.1-rc1/include/linux/blkdev.h.orig 2011-08-11 11:19:52.585280223 +0900 +++ linux-3.1-rc1/include/linux/blkdev.h 2011-08-11 11:20:09.482279763 +0900 @@ -804,6 +804,7 @@ extern struct request_queue *blk_init_al extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *); extern struct request_queue *blk_init_allocated_queue(struct request_queue *, request_fn_proc *, spinlock_t *); +extern void blk_kill_queue(struct request_queue *); extern void blk_cleanup_queue(struct request_queue *); extern void blk_queue_make_request(struct request_queue *, make_request_fn *); extern void blk_queue_bounce_limit(struct request_queue *, u64); --- linux-3.1-rc1/block/blk-core.c.orig 2011-08-10 09:46:06.014043123 +0900 +++ linux-3.1-rc1/block/blk-core.c 2011-08-11 11:19:34.551280697 +0900 @@ -347,6 +347,17 @@ void blk_put_queue(struct request_queue } EXPORT_SYMBOL(blk_put_queue); +void blk_kill_queue(struct request_queue *q) +{ + blk_sync_queue(q); + + del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer); + mutex_lock(&q->sysfs_lock); + queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q); + mutex_unlock(&q->sysfs_lock); +} +EXPORT_SYMBOL(blk_kill_queue); + /* * Note: If a driver supplied the queue lock, it should not zap that lock * unexpectedly as some queue cleanup components like elevator_exit() and @@ -360,12 +371,7 @@ void blk_cleanup_queue(struct request_qu * are done before moving on. Going into this function, we should * not have processes doing IO to this device. */ - blk_sync_queue(q); - - del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer); - mutex_lock(&q->sysfs_lock); - queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q); - mutex_unlock(&q->sysfs_lock); + blk_kill_queue(q); if (q->elevator) elevator_exit(q->elevator); --- linux-3.1-rc1/drivers/scsi/scsi_sysfs.c.orig 2011-08-09 18:48:13.676485115 +0900 +++ linux-3.1-rc1/drivers/scsi/scsi_sysfs.c 2011-08-11 11:21:07.923277456 +0900 @@ -322,6 +322,7 @@ static void scsi_device_dev_release_user kfree(evt); } + scsi_free_queue(sdev->request_queue); blk_put_queue(sdev->request_queue); /* NULL queue means the device can't be used */ sdev->request_queue = NULL; @@ -937,7 +938,7 @@ void __scsi_remove_device(struct scsi_de sdev->request_queue->queuedata = NULL; /* Freeing the queue signals to block that we're done */ - scsi_free_queue(sdev->request_queue); + blk_kill_queue(sdev->request_queue); put_device(dev); }