| Message ID | 4b4da27f-1540-db69-dce7-431b61b164ef@sandisk.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Tue, Nov 15 2016 at 6:33pm -0500, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > Use a single loop instead of two loops to determine whether or not > all_blk_mq has to be set. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > --- > drivers/md/dm-table.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 49893fdc..fff4979 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -871,7 +871,7 @@ static int dm_table_determine_type(struct dm_table *t) > { > unsigned i; > unsigned bio_based = 0, request_based = 0, hybrid = 0; > - bool verify_blk_mq = false; > + unsigned sq_count = 0, mq_count = 0; > struct dm_target *tgt; > struct dm_dev_internal *dd; > struct list_head *devices = dm_table_get_devices(t); > @@ -959,20 +959,15 @@ static int dm_table_determine_type(struct dm_table *t) > } > > if (q->mq_ops) > - verify_blk_mq = true; > + mq_count++; > + else > + sq_count++; > } > - > - if (verify_blk_mq) { > - /* verify _all_ devices in the table are blk-mq devices */ > - list_for_each_entry(dd, devices, list) > - if (!bdev_get_queue(dd->dm_dev->bdev)->mq_ops) { > - DMERR("table load rejected: not all devices" > - " are blk-mq request-stackable"); > - return -EINVAL; > - } > - > - t->all_blk_mq = true; > + if (sq_count && mq_count) { > + DMERR("table load rejected: not all devices are blk-mq request-stackable"); > + return -EINVAL; > } > + t->all_blk_mq = mq_count > 0; > > return 0; > } > @@ -1021,6 +1016,10 @@ bool dm_table_request_based(struct dm_table *t) > return __table_type_request_based(dm_table_get_type(t)); > } > > +/* > + * Returns true if all paths are blk-mq devices. Returns false if all paths > + * are single queue block devices or if there are no paths. > + */ This code isn't specific to multipath. So "paths" is misplaced. "devices" is more appropriate. Not seeing the need for the comment to be honest. The function name is pretty descriptive. > bool dm_table_all_blk_mq_devices(struct dm_table *t) > { > return t->all_blk_mq; > -- > 2.10.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 11/16/2016 06:54 AM, Mike Snitzer wrote: > On Tue, Nov 15 2016 at 6:33pm -0500, > Bart Van Assche <bart.vanassche@sandisk.com> wrote: >> +/* >> + * Returns true if all paths are blk-mq devices. Returns false if all paths >> + * are single queue block devices or if there are no paths. >> + */ > > This code isn't specific to multipath. So "paths" is misplaced. > "devices" is more appropriate. Not seeing the need for the comment to > be honest. The function name is pretty descriptive. > >> bool dm_table_all_blk_mq_devices(struct dm_table *t) >> { >> return t->all_blk_mq; Hello Mike, After having added WARN_ON_ONCE(sq_count == 0 && mq_count == 0) close to the end of dm_table_determine_type() the following output appeared: WARNING: CPU: 0 PID: 2458 at drivers/md/dm-table.c:966 dm_table_complete+0x60e/0x6b0 [dm_mod] [ ... ] Call Trace: [<ffffffff81329b45>] dump_stack+0x68/0x93 [<ffffffff810650e6>] __warn+0xc6/0xe0 [<ffffffff810651b8>] warn_slowpath_null+0x18/0x20 [<ffffffffa0021c8e>] dm_table_complete+0x60e/0x6b0 [dm_mod] [<ffffffffa00245cd>] table_load+0x13d/0x330 [dm_mod] [<ffffffffa0024490>] ? retrieve_status+0x1b0/0x1b0 [dm_mod] [<ffffffffa0025145>] ctl_ioctl+0x1f5/0x520 [dm_mod] [<ffffffffa002547e>] dm_ctl_ioctl+0xe/0x20 [dm_mod] [<ffffffff811f26cf>] do_vfs_ioctl+0x8f/0x6b0 [<ffffffff811ff41c>] ? __fget+0x10c/0x200 [<ffffffff811ff310>] ? expand_files+0x2a0/0x2a0 [<ffffffff811f2d2c>] SyS_ioctl+0x3c/0x70 [<ffffffff8163f9aa>] entry_SYSCALL_64_fastpath+0x18/0xad Is it intentional that dm_table_determine_type() can get called for a dm device with an empty table? Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Nov 16 2016 at 3:14pm -0500, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > On 11/16/2016 06:54 AM, Mike Snitzer wrote: > >On Tue, Nov 15 2016 at 6:33pm -0500, > >Bart Van Assche <bart.vanassche@sandisk.com> wrote: > >>+/* > >>+ * Returns true if all paths are blk-mq devices. Returns false if all paths > >>+ * are single queue block devices or if there are no paths. > >>+ */ > > > >This code isn't specific to multipath. So "paths" is misplaced. > >"devices" is more appropriate. Not seeing the need for the comment to > >be honest. The function name is pretty descriptive. > > > >> bool dm_table_all_blk_mq_devices(struct dm_table *t) > >> { > >> return t->all_blk_mq; > > Hello Mike, > > After having added WARN_ON_ONCE(sq_count == 0 && mq_count == 0) > close to the end of dm_table_determine_type() the following output > appeared: > > WARNING: CPU: 0 PID: 2458 at drivers/md/dm-table.c:966 > dm_table_complete+0x60e/0x6b0 [dm_mod] > [ ... ] > Call Trace: > [<ffffffff81329b45>] dump_stack+0x68/0x93 > [<ffffffff810650e6>] __warn+0xc6/0xe0 > [<ffffffff810651b8>] warn_slowpath_null+0x18/0x20 > [<ffffffffa0021c8e>] dm_table_complete+0x60e/0x6b0 [dm_mod] > [<ffffffffa00245cd>] table_load+0x13d/0x330 [dm_mod] > [<ffffffffa0024490>] ? retrieve_status+0x1b0/0x1b0 [dm_mod] > [<ffffffffa0025145>] ctl_ioctl+0x1f5/0x520 [dm_mod] > [<ffffffffa002547e>] dm_ctl_ioctl+0xe/0x20 [dm_mod] > [<ffffffff811f26cf>] do_vfs_ioctl+0x8f/0x6b0 > [<ffffffff811ff41c>] ? __fget+0x10c/0x200 > [<ffffffff811ff310>] ? expand_files+0x2a0/0x2a0 > [<ffffffff811f2d2c>] SyS_ioctl+0x3c/0x70 > [<ffffffff8163f9aa>] entry_SYSCALL_64_fastpath+0x18/0xad > > Is it intentional that dm_table_determine_type() can get called for > a dm device with an empty table? What table were you trying to load when this occurred? Targets like the 'error' target don't have any devices. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 11/16/2016 01:11 PM, Mike Snitzer wrote: > On Wed, Nov 16 2016 at 3:14pm -0500, > Bart Van Assche <bart.vanassche@sandisk.com> wrote: >> After having added WARN_ON_ONCE(sq_count == 0 && mq_count == 0) >> close to the end of dm_table_determine_type() the following output >> appeared: >> >> WARNING: CPU: 0 PID: 2458 at drivers/md/dm-table.c:966 >> dm_table_complete+0x60e/0x6b0 [dm_mod] >> [ ... ] >> Call Trace: >> [<ffffffff81329b45>] dump_stack+0x68/0x93 >> [<ffffffff810650e6>] __warn+0xc6/0xe0 >> [<ffffffff810651b8>] warn_slowpath_null+0x18/0x20 >> [<ffffffffa0021c8e>] dm_table_complete+0x60e/0x6b0 [dm_mod] >> [<ffffffffa00245cd>] table_load+0x13d/0x330 [dm_mod] >> [<ffffffffa0024490>] ? retrieve_status+0x1b0/0x1b0 [dm_mod] >> [<ffffffffa0025145>] ctl_ioctl+0x1f5/0x520 [dm_mod] >> [<ffffffffa002547e>] dm_ctl_ioctl+0xe/0x20 [dm_mod] >> [<ffffffff811f26cf>] do_vfs_ioctl+0x8f/0x6b0 >> [<ffffffff811ff41c>] ? __fget+0x10c/0x200 >> [<ffffffff811ff310>] ? expand_files+0x2a0/0x2a0 >> [<ffffffff811f2d2c>] SyS_ioctl+0x3c/0x70 >> [<ffffffff8163f9aa>] entry_SYSCALL_64_fastpath+0x18/0xad >> >> Is it intentional that dm_table_determine_type() can get called for >> a dm device with an empty table? > > What table were you trying to load when this occurred? > > Targets like the 'error' target don't have any devices. Hello Mike, This was the result of a table load issued by multipathd. Do you want me to add more debug code such that the details of the table load request are logged? Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Nov 16 2016 at 4:53pm -0500, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > On 11/16/2016 01:11 PM, Mike Snitzer wrote: > >On Wed, Nov 16 2016 at 3:14pm -0500, > >Bart Van Assche <bart.vanassche@sandisk.com> wrote: > >>After having added WARN_ON_ONCE(sq_count == 0 && mq_count == 0) > >>close to the end of dm_table_determine_type() the following output > >>appeared: > >> > >>WARNING: CPU: 0 PID: 2458 at drivers/md/dm-table.c:966 > >>dm_table_complete+0x60e/0x6b0 [dm_mod] > >>[ ... ] > >>Call Trace: > >> [<ffffffff81329b45>] dump_stack+0x68/0x93 > >> [<ffffffff810650e6>] __warn+0xc6/0xe0 > >> [<ffffffff810651b8>] warn_slowpath_null+0x18/0x20 > >> [<ffffffffa0021c8e>] dm_table_complete+0x60e/0x6b0 [dm_mod] > >> [<ffffffffa00245cd>] table_load+0x13d/0x330 [dm_mod] > >> [<ffffffffa0024490>] ? retrieve_status+0x1b0/0x1b0 [dm_mod] > >> [<ffffffffa0025145>] ctl_ioctl+0x1f5/0x520 [dm_mod] > >> [<ffffffffa002547e>] dm_ctl_ioctl+0xe/0x20 [dm_mod] > >> [<ffffffff811f26cf>] do_vfs_ioctl+0x8f/0x6b0 > >> [<ffffffff811ff41c>] ? __fget+0x10c/0x200 > >> [<ffffffff811ff310>] ? expand_files+0x2a0/0x2a0 > >> [<ffffffff811f2d2c>] SyS_ioctl+0x3c/0x70 > >> [<ffffffff8163f9aa>] entry_SYSCALL_64_fastpath+0x18/0xad > >> > >>Is it intentional that dm_table_determine_type() can get called for > >>a dm device with an empty table? > > > >What table were you trying to load when this occurred? > > > >Targets like the 'error' target don't have any devices. > > Hello Mike, > > This was the result of a table load issued by multipathd. Do you > want me to add more debug code such that the details of the table > load request are logged? No, I think it's fine. You likely have 'queue_if_no_path' configured and multipathd is loading a 'multipath' table that doesn't have any paths. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 49893fdc..fff4979 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -871,7 +871,7 @@ static int dm_table_determine_type(struct dm_table *t) { unsigned i; unsigned bio_based = 0, request_based = 0, hybrid = 0; - bool verify_blk_mq = false; + unsigned sq_count = 0, mq_count = 0; struct dm_target *tgt; struct dm_dev_internal *dd; struct list_head *devices = dm_table_get_devices(t); @@ -959,20 +959,15 @@ static int dm_table_determine_type(struct dm_table *t) } if (q->mq_ops) - verify_blk_mq = true; + mq_count++; + else + sq_count++; } - - if (verify_blk_mq) { - /* verify _all_ devices in the table are blk-mq devices */ - list_for_each_entry(dd, devices, list) - if (!bdev_get_queue(dd->dm_dev->bdev)->mq_ops) { - DMERR("table load rejected: not all devices" - " are blk-mq request-stackable"); - return -EINVAL; - } - - t->all_blk_mq = true; + if (sq_count && mq_count) { + DMERR("table load rejected: not all devices are blk-mq request-stackable"); + return -EINVAL; } + t->all_blk_mq = mq_count > 0; return 0; } @@ -1021,6 +1016,10 @@ bool dm_table_request_based(struct dm_table *t) return __table_type_request_based(dm_table_get_type(t)); } +/* + * Returns true if all paths are blk-mq devices. Returns false if all paths + * are single queue block devices or if there are no paths. + */ bool dm_table_all_blk_mq_devices(struct dm_table *t) { return t->all_blk_mq;
Use a single loop instead of two loops to determine whether or not all_blk_mq has to be set. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> --- drivers/md/dm-table.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)