From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: dm: Refuse to load an sq table on an mq device Date: Wed, 7 Dec 2016 21:11:59 -0500 Message-ID: <20161208021159.GA5944@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Bart Van Assche Cc: device-mapper development List-Id: dm-devel.ids On Wed, Dec 07 2016 at 7:56P -0500, Bart Van Assche wrote: > This patch avoids that the following call trace can be triggered: > > ------------[ cut here ]------------ > WARNING: CPU: 10 PID: 19102 at drivers/md/dm-mpath.c:543 __multipath_map.isra.17+0x23a/0x370 [dm_multipath] > CPU: 10 PID: 19102 Comm: mkfs.ext4 Not tainted 4.9.0-rc8-dbg+ #4 > Call Trace: > [] dump_stack+0x68/0x93 > [] __warn+0xc6/0xe0 > [] warn_slowpath_null+0x18/0x20 > [] __multipath_map.isra.17+0x23a/0x370 [dm_multipath] > [] multipath_clone_and_map+0x18/0x20 [dm_multipath] > [] map_request+0xed/0x300 [dm_mod] > [] dm_mq_queue_rq+0x77/0x110 [dm_mod] > [] blk_mq_process_rq_list+0x23f/0x340 > [] __blk_mq_run_hw_queue+0x122/0x1c0 > [] blk_mq_run_hw_queue+0x9f/0xc0 > [] blk_mq_insert_requests+0x245/0x320 > [] blk_mq_flush_plug_list+0x125/0x140 > [] blk_flush_plug_list+0xa2/0x210 > [] blk_finish_plug+0x27/0x40 > [] __do_page_cache_readahead+0x279/0x2f0 > [] force_page_cache_readahead+0xa8/0x100 > [] page_cache_sync_readahead+0x39/0x40 > [] generic_file_read_iter+0x234/0x780 > [] blkdev_read_iter+0x30/0x40 > [] __vfs_read+0xbb/0x130 > [] vfs_read+0x91/0x130 > [] SyS_read+0x44/0xa0 > [] entry_SYSCALL_64_fastpath+0x18/0xad > ---[ end trace c5c6591c32eae6dd ]--- > > after having inserted the following code in __multipath_map(): > > WARN_ON_ONCE(clone && q->mq_ops); > WARN_ON_ONCE(!clone && !q->mq_ops); > > Signed-off-by: Bart Van Assche > Cc: > --- > drivers/md/dm-ioctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index c72a77048b73..918e8b363015 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -1322,6 +1322,11 @@ static int table_load(struct dm_ioctl *param, size_t param_size) > DMWARN("can't change device type after initial table load."); > r = -EINVAL; > goto err_unlock_md_type; > + } else if (dm_get_md_type(md) == DM_TYPE_MQ_REQUEST_BASED && > + !dm_table_all_blk_mq_devices(t)) { > + DMWARN("can't load a sq table on a blk-mq dm device."); > + r = -EINVAL; > + goto err_unlock_md_type; > } > > dm_unlock_md_type(md); Hi Bart, Thanks for catching this lack of table type verification. I'm not a fan of a patch header including a stack trace for debugging was temporarily introduced loclaly for debugging purposes; especially not for a patch that will cc stable. As such I'll be re-writing the patch header. Also, I prefer the following variant of your fix (results in failing the table load a bit sooner and doesn't allow a table->type inconsistency to escape dm_table_determine_type()). Should I still attribute authorship of this change to you? Or would you prefer I switch to you being the Reporter (via Reported-by)? diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 8ce81d0..0a427de 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -976,6 +976,11 @@ static int dm_table_determine_type(struct dm_table *t) } t->all_blk_mq = mq_count > 0; + if (t->type == DM_TYPE_MQ_REQUEST_BASED && !t->all_blk_mq) { + DMERR("table load rejected: all devices are not blk-mq request-stackable"); + return -EINVAL; + } + return 0; }