Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755525AbZDWGha (ORCPT ); Thu, 23 Apr 2009 02:37:30 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753756AbZDWGhQ (ORCPT ); Thu, 23 Apr 2009 02:37:16 -0400 Received: from brick.kernel.dk ([93.163.65.50]:58428 "EHLO kernel.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753754AbZDWGhP (ORCPT ); Thu, 23 Apr 2009 02:37:15 -0400 Date: Thu, 23 Apr 2009 08:37:13 +0200 From: Jens Axboe To: Sage Weil Cc: linux-kernel@vger.kernel.org, jeff@garzik.org, neilb@suse.de, yehuda@newdream.net Subject: Re: [PATCH] umem: fix request_queue lock warning Message-ID: <20090423063713.GL4593@kernel.dk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9386 Lines: 169 On Wed, Apr 22 2009, Sage Weil wrote: > Hi, > > The umem driver issues two warnings on boot, due to blk_plug_device() and > blk_remove_plug() being called without q->queue_lock held. Starting with > e48ec690 (block: extend queue_flag bitops), the queue_flag_* functions > warn if q->queue_lock doesn't appear to be locked. In fact, q->queue_lock > is NULL (though that apparently isn't otherwise a problem as the driver is > using card->lock for everything). > > Although blk_init_queue() with take a request_fn_proc and spinlock_t*, > there isn't a corresponding init helper that takes a make_request_fn. > Setting queue_lock to &card->lock explicitly seems to work fine for me. > The warning goes away and the device appears to behave. > > If there is a better solution, I'd be happy to test it out. It looks like > this problem showed up in 2.6.27. There clearly aren't too many people > using these NVRAM cards, but it'd be nice to see this fixed. > > Thanks- > sage > > > [ 1.531881] v2.3 : Micro Memory(tm) PCI memory board block driver > [ 1.538136] umem 0000:02:01.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20 > [ 1.545018] umem 0000:02:01.0: Micro Memory(tm) controller found (PCI Mem Module (Battery Backup)) > [ 1.554176] umem 0000:02:01.0: CSR 0xfc9ffc00 -> 0xffffc200013d0c00 (0x100) > [ 1.561279] umem 0000:02:01.0: Size 1048576 KB, Battery 1 Disabled (FAILURE), Battery 2 Disabled (FAILURE) > [ 1.571114] umem 0000:02:01.0: Window size 16777216 bytes, IRQ 20 > [ 1.577304] umem 0000:02:01.0: memory NOT initialized. Consider over-writing whole device. > [ 1.585989] umema:<4>------------[ cut here ]------------ > [ 1.591775] WARNING: at include/linux/blkdev.h:492 blk_plug_device+0x6d/0x106() > [ 1.592025] Hardware name: H8SSL > [ 1.592025] Modules linked in: > [ 1.592025] Pid: 1, comm: swapper Not tainted 2.6.29 #8 > [ 1.592025] Call Trace: > [ 1.592025] [] warn_slowpath+0xd3/0xf2 > [ 1.592025] [] ? save_trace+0x3f/0x9b > [ 1.592025] [] ? add_lock_to_list+0x7a/0xba > [ 1.592025] [] ? validate_chain+0xb3b/0xce8 > [ 1.592025] [] ? mm_make_request+0x27/0x59 > [ 1.592025] [] ? mm_make_request+0x27/0x59 > [ 1.592025] [] ? __lock_acquire+0x74e/0x7b9 > [ 1.592025] [] ? get_lock_stats+0x34/0x5e > [ 1.592025] [] ? put_lock_stats+0xe/0x27 > [ 1.592025] [] ? mm_make_request+0x27/0x59 > [ 1.592025] [] blk_plug_device+0x6d/0x106 > [ 1.592025] [] mm_make_request+0x46/0x59 > [ 1.592025] [] generic_make_request+0x335/0x3cf > [ 1.592025] [] ? mempool_alloc_slab+0x11/0x13 > [ 1.592025] [] ? mempool_alloc+0x45/0x101 > [ 1.592025] [] ? put_lock_stats+0xe/0x27 > [ 1.592025] [] submit_bio+0x10a/0x119 > [ 1.592025] [] submit_bh+0xe5/0x109 > [ 1.592025] [] block_read_full_page+0x2aa/0x2cb > [ 1.592025] [] ? blkdev_get_block+0x0/0x4c > [ 1.592025] [] ? _spin_unlock_irq+0x36/0x51 > [ 1.592025] [] ? __lru_cache_add+0x92/0xb2 > [ 1.592025] [] blkdev_readpage+0x13/0x15 > [ 1.592025] [] read_cache_page_async+0x90/0x134 > [ 1.592025] [] ? blkdev_readpage+0x0/0x15 > [ 1.592025] [] ? adfspart_check_ICS+0x0/0x16c > [ 1.592025] [] read_cache_page+0xe/0x45 > [ 1.592025] [] read_dev_sector+0x2e/0x93 > [ 1.592025] [] adfspart_check_ICS+0x28/0x16c > [ 1.592025] [] ? trace_hardirqs_on+0xd/0xf > [ 1.592025] [] ? adfspart_check_ICS+0x0/0x16c > [ 1.592025] [] rescan_partitions+0x168/0x2fb > [ 1.592025] [] __blkdev_get+0x259/0x336 > [ 1.592025] [] ? kobject_put+0x47/0x4b > [ 1.592025] [] blkdev_get+0xb/0xd > [ 1.592025] [] register_disk+0xc4/0x12b > [ 1.592025] [] add_disk+0xc3/0x12d > [ 1.592025] [] ? mm_init+0x0/0x1a5 > [ 1.592025] [] mm_init+0x129/0x1a5 > [ 1.592025] [] ? mm_init+0x0/0x1a5 > [ 1.592025] [] _stext+0x56/0x130 > [ 1.592025] [] ? register_irq_proc+0xae/0xca > [ 1.592025] [] ? proc_pid_lookup+0xb4/0x18b > [ 1.592025] [] kernel_init+0x132/0x18b > [ 1.592025] [] child_rip+0xa/0x20 > [ 1.592025] [] ? restore_args+0x0/0x30 > [ 1.592025] [] ? kernel_init+0x0/0x18b > [ 1.592025] [] ? child_rip+0x0/0x20 > [ 1.592025] ---[ end trace 7150b3b86da74e1e ]--- > [ 1.889858] ------------[ cut here ]------------[ve_plug+0x5f/0x91() > [ 1.893848] Hardware name: H8SSL > [ 1.893848] Modules linked in: > [ 1.893848] Pid: 1, comm: swapper Tainted: G W 2.6.29 #8 > [ 1.893848] Call Trace: > [ 1.893848] [] warn_slowpath+0xd3/0xf2 > [ 1.893848] [] ? trace_hardirqs_on_thunk+0x3a/0x3f > [ 1.893848] [] ? restore_args+0x0/0x30 > [ 1.893848] [] ? __atomic_notifier_call_chain+0x0/0xb2 > [ 1.893848] [] ? _spin_unlock_irq+0x31/0x51 > [ 1.893848] [] ? _spin_unlock_irq+0x4d/0x51 > [ 1.893848] [] ? mm_make_request+0x4e/0x59 > [ 1.893848] [] ? get_lock_stats+0x34/0x5e > [ 1.893848] [] ? put_lock_stats+0x25/0x27 > [ 1.893848] [] ? mm_unplug_device+0x25/0x50 > [ 1.893848] [] blk_remove_plug+0x5f/0x91 > [ 1.893848] [] mm_unplug_device+0x30/0x50 > [ 1.893848] [] blk_unplug+0x78/0x7d > [ 1.893848] [] blk_backing_dev_unplug+0xd/0xf > [ 1.893848] [] block_sync_page+0x4a/0x4c > [ 1.893848] [] sync_page+0x44/0x4d > [ 1.893848] [] __wait_on_bit_lock+0x42/0x8a > [ 1.893848] [] ? sync_page+0x0/0x4d > [ 1.893848] [] __lock_page+0x64/0x6b > [ 1.893848] [] ? wake_bit_function+0x0/0x2a > [ 1.893848] [] read_cache_page_async+0xd4/0x134 > [ 1.893848] [] ? blkdev_readpage+0x0/0x15 > [ 1.893848] [] ? adfspart_check_ICS+0x0/0x16c > [ 1.893848] [] read_cache_page+0xe/0x45 > [ 1.893848] [] read_dev_sector+0x2e/0x93 > [ 1.893848] [] adfspart_check_ICS+0x28/0x16c > [ 1.893848] [] ? trace_hardirqs_on+0xd/0xf > [ 1.893848] [] ? adfspart_check_ICS+0x0/0x16c > [ 1.893848] [] rescan_partitions+0x168/0x2fb > [ 1.893848] [] __blkdev_get+0x259/0x336 > [ 1.893848] [] ? kobject_put+0x47/0x4b > [ 1.893848] [] blkdev_get+0xb/0xd > [ 1.893848] [] register_disk+0xc4/0x12b > [ 1.893848] [] add_disk+0xc3/0x12d > [ 1.893848] [] ? mm_init+0x0/0x1a5 > [ 1.893848] [] mm_init+0x129/0x1a5 > [ 1.893848] [] ? mm_init+0x0/0x1a5 > [ 1.893848] [] _stext+0x56/0x130 > [ 1.893848] [] ? register_irq_proc+0xae/0xca > [ 1.893848] [] ? proc_pid_lookup+0xb4/0x18b > [ 1.893848] [] kernel_init+0x132/0x18b > [ 1.893848] [] child_rip+0xa/0x20 > [ 1.893848] [] ? restore_args+0x0/0x30 > [ 1.893848] [] ? kernel_init+0x0/0x18b > [ 1.893848] [] ? child_rip+0x0/0x20 > [ 1.893848] ---[ end trace 7150b3b86da74e1f ]--- > > > Signed-off-by: Sage Weil > --- > drivers/block/umem.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/block/umem.c b/drivers/block/umem.c > index c24e1bd..de669fb 100644 > --- a/drivers/block/umem.c > +++ b/drivers/block/umem.c > @@ -906,6 +906,7 @@ static int __devinit mm_pci_probe(struct pci_dev *dev, > goto failed_alloc; > > blk_queue_make_request(card->queue, mm_make_request); > + card->queue->queue_lock = &card->lock; > card->queue->queuedata = card; > card->queue->unplug_fn = mm_unplug_device; That looks like the correct fix, we are already using that very lock to protect the plugging. I guess the original commit just forgot to inform the block layer that this is the queue lock. I'll queue up your fix. -- Jens Axboe -- 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/