On Sat Aug 29 09:43:54 EDT 2009, [email protected] wrote:
...
> I finished bisecting the NULL object and ended up at this commit:
> cd43e26f071524647e660706b784ebcbefbd2e44
>
> block: Expose stacked device queues in sysfs
>
> Currently stacking devices do not have a queue directory in sysfs.
> However, many of the I/O characteristics like sector size, maximum
> request size, etc. are queue properties.
>
> This patch enables the queue directory for MD/DM devices. The elevator
> code has been modified to deal with queues that do not have an I/O
> scheduler.
>
> Signed-off-by: Martin K. Petersen <[email protected]>
> Signed-off-by: Jens Axboe <[email protected]>
>
>
> This seems to generate /sys/block/$device/queue and its contents for
> everyone who is using queues, not just for those queues that have a
> non-NULL queue->request_fn.
Thanks much for doing that. It makes sense that this change would
have caused it to suddenly matter whether the unused queue is
initialized.
The patch looks fine to me.
I don't think it should go in my aoe tree for linux-next, since the
patch addresses a regression.
Based on the series file in mmotm, I don't think this patch is in mm
at the moment. Andrew Morton, do you think this patch should go
through your mm tree?
--
Ed
On Tue, 01 September 2009 Ed Cashin <[email protected]> wrote:
> On Sat Aug 29 09:43:54 EDT 2009, [email protected] wrote:
> ...
> > I finished bisecting the NULL object and ended up at this commit:
> > cd43e26f071524647e660706b784ebcbefbd2e44
> >
> > block: Expose stacked device queues in sysfs
> >
> > Currently stacking devices do not have a queue directory in sysfs.
> > However, many of the I/O characteristics like sector size, maximum
> > request size, etc. are queue properties.
> >
> > This patch enables the queue directory for MD/DM devices. The
> > elevator code has been modified to deal with queues that do not
> > have an I/O scheduler.
> >
> > Signed-off-by: Martin K. Petersen <[email protected]>
> > Signed-off-by: Jens Axboe <[email protected]>
> >
> >
> > This seems to generate /sys/block/$device/queue and its contents for
> > everyone who is using queues, not just for those queues that have a
> > non-NULL queue->request_fn.
>
> Thanks much for doing that. It makes sense that this change would
> have caused it to suddenly matter whether the unused queue is
> initialized.
>
> The patch looks fine to me.
Would it make sense to fine-tune the values reported by sys-fs in the
queue details so they match with the AoE device (as a separate
enhancement patch)?
Sure the queue itself won't use them, but some user-space tools might
be interested in this data.
I've not checked what information is provided by AoE protocol or can
be deducted from interface MTU.
Bruno
On Tue Sep 1 16:31:58 EDT 2009, [email protected] wrote:
> On Tue, 01 September 2009 Ed Cashin <[email protected]> wrote:
...
> > Thanks much for doing that. It makes sense that this change would
> > have caused it to suddenly matter whether the unused queue is
> > initialized.
> >
> > The patch looks fine to me.
>
> Would it make sense to fine-tune the values reported by sys-fs in the
> queue details so they match with the AoE device (as a separate
> enhancement patch)?
>
> Sure the queue itself won't use them, but some user-space tools might
> be interested in this data.
>
> I've not checked what information is provided by AoE protocol or can
> be deducted from interface MTU.
The AoE protocol does allow a storage target to say, "I can handle
this certain number of sectors in a single read or write command." But
the user space tools cannot really benefit from that information,
since they can send larger read/write operations with no loss of
efficiency.
So I suspect that it would not make much sense, but if you decide to
look into it further, see whether you can identify an extant user
space tool that benefits from the information.
--
Ed Cashin
http://noserose.net/e/
On Tue, 1 Sep 2009 15:15:20 -0400
Ed Cashin <[email protected]> wrote:
> The patch looks fine to me.
I translated that into Acked-by:.
> I don't think it should go in my aoe tree for linux-next, since the
> patch addresses a regression.
>
> Based on the series file in mmotm, I don't think this patch is in mm
> at the moment. Andrew Morton, do you think this patch should go
> through your mm tree?
I have it now. Please check that the below is the right patch and that
my cobbled-together changelog makes sense (if not, please send
replacement changelog and/or patch).
From: Andy Whitcroft <[email protected]>
When using the aoe driver we see an oops triggered by use of an
incorrectly initialised request_queue object:
[ 2645.959090] kobject '<NULL>' (ffff880059ca22c0): tried to add
an uninitialized object, something is seriously wrong.
[ 2645.959104] Pid: 6, comm: events/0 Not tainted 2.6.31-5-generic #24-Ubuntu
[ 2645.959107] Call Trace:
[ 2645.959139] [<ffffffff8126ca2f>] kobject_add+0x5f/0x70
[ 2645.959151] [<ffffffff8125b4ab>] blk_register_queue+0x8b/0xf0
[ 2645.959155] [<ffffffff8126043f>] add_disk+0x8f/0x160
[ 2645.959161] [<ffffffffa01673c4>] aoeblk_gdalloc+0x164/0x1c0 [aoe]
It seems this driver mearly embeds a request_queue object in its main
control structure and does not attempt to initialise it. Pull this out
and use blk_init_queue/blk_cleanup_queue to handle initialisation and
teardown of this object.
Bruno bisected this regression down to
cd43e26f071524647e660706b784ebcbefbd2e44
block: Expose stacked device queues in sysfs
"This seems to generate /sys/block/$device/queue and its contents for
everyone who is using queues, not just for those queues that have a
non-NULL queue->request_fn."
Addresses http://bugs.launchpad.net/bugs/410198
Addresses http://bugzilla.kernel.org/show_bug.cgi?id=13942
Signed-off-by: Andy Whitcroft <[email protected]>
Acked-by: Ed Cashin <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Bruno Premont <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Cc: Jens Axboe <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/block/aoe/aoe.h | 2 +-
drivers/block/aoe/aoeblk.c | 6 +++---
drivers/block/aoe/aoedev.c | 11 ++++++++++-
3 files changed, 14 insertions(+), 5 deletions(-)
diff -puN drivers/block/aoe/aoe.h~aoe-ensure-we-initialise-the-request_queue-correctly drivers/block/aoe/aoe.h
--- a/drivers/block/aoe/aoe.h~aoe-ensure-we-initialise-the-request_queue-correctly
+++ a/drivers/block/aoe/aoe.h
@@ -155,7 +155,7 @@ struct aoedev {
u16 fw_ver; /* version of blade's firmware */
struct work_struct work;/* disk create work struct */
struct gendisk *gd;
- struct request_queue blkq;
+ struct request_queue *blkq;
struct hd_geometry geo;
sector_t ssize;
struct timer_list timer;
diff -puN drivers/block/aoe/aoeblk.c~aoe-ensure-we-initialise-the-request_queue-correctly drivers/block/aoe/aoeblk.c
--- a/drivers/block/aoe/aoeblk.c~aoe-ensure-we-initialise-the-request_queue-correctly
+++ a/drivers/block/aoe/aoeblk.c
@@ -264,8 +264,8 @@ aoeblk_gdalloc(void *vp)
goto err_disk;
}
- blk_queue_make_request(&d->blkq, aoeblk_make_request);
- if (bdi_init(&d->blkq.backing_dev_info))
+ blk_queue_make_request(d->blkq, aoeblk_make_request);
+ if (bdi_init(&d->blkq->backing_dev_info))
goto err_mempool;
spin_lock_irqsave(&d->lock, flags);
gd->major = AOE_MAJOR;
@@ -276,7 +276,7 @@ aoeblk_gdalloc(void *vp)
snprintf(gd->disk_name, sizeof gd->disk_name, "etherd/e%ld.%d",
d->aoemajor, d->aoeminor);
- gd->queue = &d->blkq;
+ gd->queue = d->blkq;
d->gd = gd;
d->flags &= ~DEVFL_GDALLOC;
d->flags |= DEVFL_UP;
diff -puN drivers/block/aoe/aoedev.c~aoe-ensure-we-initialise-the-request_queue-correctly drivers/block/aoe/aoedev.c
--- a/drivers/block/aoe/aoedev.c~aoe-ensure-we-initialise-the-request_queue-correctly
+++ a/drivers/block/aoe/aoedev.c
@@ -113,6 +113,7 @@ aoedev_freedev(struct aoedev *d)
if (d->bufpool)
mempool_destroy(d->bufpool);
skbpoolfree(d);
+ blk_cleanup_queue(d->blkq);
kfree(d);
}
@@ -202,6 +203,7 @@ aoedev_by_sysminor_m(ulong sysminor)
{
struct aoedev *d;
ulong flags;
+ struct request_queue *rq;
spin_lock_irqsave(&devlist_lock, flags);
@@ -210,9 +212,13 @@ aoedev_by_sysminor_m(ulong sysminor)
break;
if (d)
goto out;
+ rq = blk_init_queue(NULL, NULL);
+ if (!rq)
+ goto out;
d = kcalloc(1, sizeof *d, GFP_ATOMIC);
if (!d)
- goto out;
+ goto out_rq;
+ d->blkq = rq;
INIT_WORK(&d->work, aoecmd_sleepwork);
spin_lock_init(&d->lock);
skb_queue_head_init(&d->sendq);
@@ -234,6 +240,9 @@ aoedev_by_sysminor_m(ulong sysminor)
out:
spin_unlock_irqrestore(&devlist_lock, flags);
return d;
+ out_rq:
+ blk_cleanup_queue(rq);
+ goto out;
}
static void
_
On Wed Sep 2 15:55:47 EDT 2009, [email protected] wrote:
> On Tue, 1 Sep 2009 15:15:20 -0400
> Ed Cashin <[email protected]> wrote:
>
> > The patch looks fine to me.
>
> I translated that into Acked-by:.
>
> > I don't think it should go in my aoe tree for linux-next, since the
> > patch addresses a regression.
> >
> > Based on the series file in mmotm, I don't think this patch is in mm
> > at the moment. Andrew Morton, do you think this patch should go
> > through your mm tree?
>
> I have it now. Please check that the below is the right patch and that
> my cobbled-together changelog makes sense (if not, please send
> replacement changelog and/or patch).
Thanks. It looks good to me.
--
Ed