Fix race condition between cciss_init_one(), cciss_update_drive_info(), and cciss_check_queues(). cciss_softirq_done would try to start queues which were not quite ready to be started, as its checks for readiness were not sufficiently synchronized with the queue initializing code in cciss_init_one and cciss_update_drive_info. Slow cpu and large numbers of logical drives seem to make the race more likely to cause a problem.
Signed-off-by: Stephen M. Cameron <scame...@beardog.cca.cpqcorp.net>
diff -puN linux-2.6.25-rc9/drivers/block/cciss.c~cciss_init_one_race linux-2.6.25-rc9/drivers/block/cciss.c --- linux-2.6.25-rc9/linux-2.6.25-rc9/drivers/block/cciss.c~cciss_init_one_race 2008-04-14 08:21:03.000000000 -0500 +++ linux-2.6.25-rc9-scameron/linux-2.6.25-rc9/drivers/block/cciss.c 2008-04-14 08:22:04.000000000 -0500 @@ -1270,7 +1270,9 @@ static void cciss_check_queues(ctlr_info /* make sure the disk has been added and the drive is real * because this can be called from the middle of init_one. */ - if (!(h->drv[curr_queue].queue) || !(h->drv[curr_queue].heads)) + if (!(h->drv[curr_queue].queue) || + !(h->drv[curr_queue].heads) || + !h->drv[curr_queue].queue_ready) continue; blk_start_queue(h->gendisk[curr_queue]->queue);
On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> Fix race condition between cciss_init_one(), cciss_update_drive_info(), > and cciss_check_queues(). cciss_softirq_done would try to start > queues which were not quite ready to be started, as its checks for > readiness were not sufficiently synchronized with the queue initializing > code in cciss_init_one and cciss_update_drive_info. Slow cpu and > large numbers of logical drives seem to make the race more likely > to cause a problem.
Hmm, this seems backwards to me. cciss_softirq_done() isn't going to start the queues, until an irq has triggered for instance. Why isn't the init properly ordered instead of band-aiding around this with a 'queue_ready' variable?
> On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> > Fix race condition between cciss_init_one(), cciss_update_drive_info(), > > and cciss_check_queues(). cciss_softirq_done would try to start > > queues which were not quite ready to be started, as its checks for > > readiness were not sufficiently synchronized with the queue initializing > > code in cciss_init_one and cciss_update_drive_info. Slow cpu and > > large numbers of logical drives seem to make the race more likely > > to cause a problem.
> Hmm, this seems backwards to me. cciss_softirq_done() isn't going to > start the queues, until an irq has triggered for instance. Why isn't the > init properly ordered instead of band-aiding around this with a > 'queue_ready' variable?
Each call to add_disk() will trigger some interrupts, and earlier added disks may cause the queues of later, not-yet-completely added disks to be started.
I suppose the init routine might be reorganized to initialize all the queues, then have second loop call add_disk() for all of them. Is that what you had in mind by "properly ordered?"
Disks may be added at run time though, and I think this tears down all but the first disk, and re-adds them all, if I remember right, so there is some complication there to think about.
-- steve
On Mon, Apr 14, 2008 at 07:05:05PM +0200, Jens Axboe wrote: > -- > Jens Axboe
On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> > On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> > > Fix race condition between cciss_init_one(), cciss_update_drive_info(), > > > and cciss_check_queues(). cciss_softirq_done would try to start > > > queues which were not quite ready to be started, as its checks for > > > readiness were not sufficiently synchronized with the queue initializing > > > code in cciss_init_one and cciss_update_drive_info. Slow cpu and > > > large numbers of logical drives seem to make the race more likely > > > to cause a problem.
> > Hmm, this seems backwards to me. cciss_softirq_done() isn't going to > > start the queues, until an irq has triggered for instance. Why isn't the > > init properly ordered instead of band-aiding around this with a > > 'queue_ready' variable?
> Each call to add_disk() will trigger some interrupts, > and earlier added disks may cause the queues of later, > not-yet-completely added disks to be started.
> I suppose the init routine might be reorganized to initialize all > the queues, then have second loop call add_disk() for all > of them. Is that what you had in mind by "properly ordered?"
Yep precisely, don't call add_disk() until everything is set up.
> Disks may be added at run time though, and I think this tears > down all but the first disk, and re-adds them all, if I remember > right, so there is some complication there to think about.
Well, other drivers manage quite fine without resorting to work-arounds :-)
On Mon, Apr 14, 2008 at 07:37:20PM +0200, Jens Axboe wrote: > On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> > > On Mon, Apr 14 2008, scame...@beardog.cca.cpqcorp.net wrote:
> > > > Fix race condition between cciss_init_one(), cciss_update_drive_info(), > > > > and cciss_check_queues(). cciss_softirq_done would try to start > > > > queues which were not quite ready to be started, as its checks for > > > > readiness were not sufficiently synchronized with the queue initializing > > > > code in cciss_init_one and cciss_update_drive_info. Slow cpu and > > > > large numbers of logical drives seem to make the race more likely > > > > to cause a problem.
> > > Hmm, this seems backwards to me. cciss_softirq_done() isn't going to > > > start the queues, until an irq has triggered for instance. Why isn't the > > > init properly ordered instead of band-aiding around this with a > > > 'queue_ready' variable?
> > Each call to add_disk() will trigger some interrupts, > > and earlier added disks may cause the queues of later, > > not-yet-completely added disks to be started.
> > I suppose the init routine might be reorganized to initialize all > > the queues, then have second loop call add_disk() for all > > of them. Is that what you had in mind by "properly ordered?"
> Yep precisely, don't call add_disk() until everything is set up.
> > Disks may be added at run time though, and I think this tears > > down all but the first disk, and re-adds them all, if I remember > > right, so there is some complication there to think about.
> Well, other drivers manage quite fine without resorting to work-arounds > :-)
Ok. Thanks for the constructive criticism. I'll rethink it.
Fortunately, (or unfortunately) the race is apparently pretty hard to trigger, it's been in there for ages, and we've only just seen it manifest as a problem recently and only in one particular configuration.
> > > > > Fix race condition between cciss_init_one(), cciss_update_drive_info(), > > > > > and cciss_check_queues(). cciss_softirq_done would try to start > > > > > queues which were not quite ready to be started, as its checks for > > > > > readiness were not sufficiently synchronized with the queue initializing > > > > > code in cciss_init_one and cciss_update_drive_info. Slow cpu and > > > > > large numbers of logical drives seem to make the race more likely > > > > > to cause a problem.
> > > > Hmm, this seems backwards to me. cciss_softirq_done() isn't going to > > > > start the queues, until an irq has triggered for instance. Why isn't the > > > > init properly ordered instead of band-aiding around this with a > > > > 'queue_ready' variable?
> > > Each call to add_disk() will trigger some interrupts, > > > and earlier added disks may cause the queues of later, > > > not-yet-completely added disks to be started.
> > > I suppose the init routine might be reorganized to initialize all > > > the queues, then have second loop call add_disk() for all > > > of them. Is that what you had in mind by "properly ordered?"
> > Yep precisely, don't call add_disk() until everything is set up.
> > > Disks may be added at run time though, and I think this tears > > > down all but the first disk, and re-adds them all, if I remember > > > right, so there is some complication there to think about.
> > Well, other drivers manage quite fine without resorting to work-arounds > > :-)
> Ok. Thanks for the constructive criticism. I'll rethink it.
> Fortunately, (or unfortunately) the race is apparently pretty hard > to trigger, it's been in there for ages, and we've only just seen it > manifest as a problem recently and only in one particular configuration.
Hopefully that will not matter. If you rework the init code so that everything is up and running before you allow any IO going on, then it'll be easier to 'prove' that you can't hit such races. If you can have disk added at runtime, make them go through the same init process/function.
+ /* Make sure all queue data is written out before */ + /* setting h->drv[drv_index].queue, as setting this */ + /* allows the interrupt handler to start the queue */ + wmb(); h->drv[drv_index].queue = disk->queue; add_disk(disk); } @@ -3520,10 +3528,17 @@ static int __devinit cciss_init_one(stru continue; blk_queue_hardsect_size(q, drv->block_size); set_capacity(disk, drv->nr_blocks); - add_disk(disk); j++; } while (j <= hba[i]->highest_lun);
+ /* Make sure all queue data is written out before */ + /* interrupt handler, triggered by add_disk, */ + /* is allowed to start them. */ + wmb(); + + for (j = 0; j <= hba[i]->highest_lun; j++) + add_disk(hba[i]->gendisk[j]); + return 1;
> + /* Make sure all queue data is written out before */ > + /* setting h->drv[drv_index].queue, as setting this */ > + /* allows the interrupt handler to start the queue */ > + wmb(); > h->drv[drv_index].queue = disk->queue; > add_disk(disk); > } > @@ -3520,10 +3528,17 @@ static int __devinit cciss_init_one(stru > continue; > blk_queue_hardsect_size(q, drv->block_size); > set_capacity(disk, drv->nr_blocks); > - add_disk(disk); > j++; > } while (j <= hba[i]->highest_lun);
> + /* Make sure all queue data is written out before */ > + /* interrupt handler, triggered by add_disk, */ > + /* is allowed to start them. */ > + wmb(); > + > + for (j = 0; j <= hba[i]->highest_lun; j++) > + add_disk(hba[i]->gendisk[j]); > + > return 1;
