diff for duplicates of <200812140015.23853.bzolnier@gmail.com> diff --git a/a/1.txt b/N1/1.txt index 5ac2090..7d8e959 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,319 +1,21 @@ [ Once again, sorry for the long delay. ] - -On Monday 24 November 2008, Elias Oltmanns wrote: -> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: -> > * Move hack for flush requests from choose_drive() to do_ide_request(). -> > -> > * Add ide_plug_device() helper and convert core IDE code from using -> > per-hwgroup lock as a request lock to use the ->queue_lock instead. -> > -> > * Remove no longer needed: -> > - choose_drive() function -> > - WAKEUP() macro -> > - 'sleeping' flag from ide_hwif_t -> > - 'service_{start,time}' fields from ide_drive_t -> > -> > This patch results in much simpler and more maintainable code -> > (besides being a scalability improvement). -> > -> > Cc: Elias Oltmanns <eo@nebensachen.de> -> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> -> > --- -> > newer version -> -> Eventually, I got round to having a look at this one (I reviewed the two -> small ones at the time). Apologies for the delay. -> -> > -> > drivers/ide/ide-io.c | 213 +++++++++++++++--------------------------------- -> > drivers/ide/ide-park.c | 13 +- -> > drivers/ide/ide-probe.c | 3 -> > include/linux/ide.h | 4 -> > 4 files changed, 79 insertions(+), 154 deletions(-) -> > -> > Index: b/drivers/ide/ide-io.c -> > =================================================================== -> > --- a/drivers/ide/ide-io.c -> > +++ b/drivers/ide/ide-io.c -> [...] -> > @@ -780,61 +704,40 @@ repeat: -> > */ -> > void do_ide_request(struct request_queue *q) -> > { -> > - ide_drive_t *orig_drive = q->queuedata; -> > - ide_hwgroup_t *hwgroup = orig_drive->hwif->hwgroup; -> > - ide_drive_t *drive; -> > - ide_hwif_t *hwif; -> > + ide_drive_t *drive = q->queuedata; -> > + ide_hwif_t *hwif = drive->hwif; -> > + ide_hwgroup_t *hwgroup = hwif->hwgroup; -> > struct request *rq; -> > ide_startstop_t startstop; -> > -> > - /* caller must own hwgroup->lock */ -> > - BUG_ON(!irqs_disabled()); -> > - -> > - while (!ide_lock_hwgroup(hwgroup)) { -> > - drive = choose_drive(hwgroup); -> > - if (drive == NULL) { -> > - int sleeping = 0; -> > - unsigned long sleep = 0; /* shut up, gcc */ -> > - hwgroup->rq = NULL; -> > - drive = hwgroup->drive; -> > - do { -> > - if ((drive->dev_flags & IDE_DFLAG_SLEEPING) && -> > - (sleeping == 0 || -> > - time_before(drive->sleep, sleep))) { -> > - sleeping = 1; -> > - sleep = drive->sleep; -> > - } -> > - } while ((drive = drive->next) != hwgroup->drive); -> > - if (sleeping) { -> > + /* -> > + * drive is doing pre-flush, ordered write, post-flush sequence. even -> > + * though that is 3 requests, it must be seen as a single transaction. -> > + * we must not preempt this drive until that is complete -> > + */ -> > + if (blk_queue_flushing(q)) -> > /* -> > - * Take a short snooze, and then wake up this hwgroup again. -> > - * This gives other hwgroups on the same a chance to -> > - * play fairly with us, just in case there are big differences -> > - * in relative throughputs.. don't want to hog the cpu too much. -> > + * small race where queue could get replugged during -> > + * the 3-request flush cycle, just yank the plug since -> > + * we want it to finish asap -> > */ -> > - if (time_before(sleep, jiffies + WAIT_MIN_SLEEP)) -> > - sleep = jiffies + WAIT_MIN_SLEEP; -> > -#if 1 -> > - if (timer_pending(&hwgroup->timer)) -> > - printk(KERN_CRIT "ide_set_handler: timer already active\n"); -> > -#endif -> > - /* so that ide_timer_expiry knows what to do */ -> > - hwgroup->sleeping = 1; -> > - hwgroup->req_gen_timer = hwgroup->req_gen; -> > - mod_timer(&hwgroup->timer, sleep); -> > - /* we purposely leave hwgroup locked -> > - * while sleeping */ -> > - } else -> > - ide_unlock_hwgroup(hwgroup); -> > + blk_remove_plug(q); -> -> I'm not at all convinced that this works as expected. First of all, I -> think we can safely assume that the plug is removed when block layer -> calls into the ->request_fn(). Secondly, since the ide layer doesn't -> call the ->request_fn() on it's own accord, I rather suspect that this -> check can be dropped altogether. On the other hand, I'm not sure I agree - -I suspect that this is leftover from the old code and I also think that -it can be removed completely. However mixing too many real code changes -in a single patch is not a good practice and the removal can be handled -independently of the discussed patch. - -If you would send me a patch with the above change I will be happy to -integrate it into pata tree (patch can be against current Linus' tree or -linux-next, either one is completely fine with me). - -> with the rest of the patch anyway, see below. -> -> > -> > - /* no more work for this hwgroup (for now) */ -> > - goto plug_device; -> > - } -> > + spin_unlock_irq(q->queue_lock); -> > + spin_lock_irq(&hwgroup->lock); -> > -> > - if (drive != orig_drive) -> > - goto plug_device; -> > + /* caller must own hwgroup->lock */ -> > + BUG_ON(!irqs_disabled()); -> -> Does this check really make any sense? The lock is being taken just two -> lines before, after all. - +On Monday 24 November 2008, Elias Oltmanns wrote:> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:> > * Move hack for flush requests from choose_drive() to do_ide_request().> >> > * Add ide_plug_device() helper and convert core IDE code from using> > per-hwgroup lock as a request lock to use the ->queue_lock instead.> >> > * Remove no longer needed:> > - choose_drive() function> > - WAKEUP() macro> > - 'sleeping' flag from ide_hwif_t> > - 'service_{start,time}' fields from ide_drive_t> >> > This patch results in much simpler and more maintainable code> > (besides being a scalability improvement).> >> > Cc: Elias Oltmanns <eo@nebensachen.de>> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>> > ---> > newer version> > Eventually, I got round to having a look at this one (I reviewed the two> small ones at the time). Apologies for the delay.> > >> > drivers/ide/ide-io.c | 213 +++++++++++++++---------------------------------> > drivers/ide/ide-park.c | 13 +-> > drivers/ide/ide-probe.c | 3 > > include/linux/ide.h | 4 > > 4 files changed, 79 insertions(+), 154 deletions(-)> >> > Index: b/drivers/ide/ide-io.c> > ===================================================================> > --- a/drivers/ide/ide-io.c> > +++ b/drivers/ide/ide-io.c> [...]> > @@ -780,61 +704,40 @@ repeat: > > */> > void do_ide_request(struct request_queue *q)> > {> > - ide_drive_t *orig_drive = q->queuedata;> > - ide_hwgroup_t *hwgroup = orig_drive->hwif->hwgroup;> > - ide_drive_t *drive;> > - ide_hwif_t *hwif;> > + ide_drive_t *drive = q->queuedata;> > + ide_hwif_t *hwif = drive->hwif;> > + ide_hwgroup_t *hwgroup = hwif->hwgroup;> > struct request *rq;> > ide_startstop_t startstop;> > > > - /* caller must own hwgroup->lock */> > - BUG_ON(!irqs_disabled());> > -> > - while (!ide_lock_hwgroup(hwgroup)) {> > - drive = choose_drive(hwgroup);> > - if (drive == NULL) {> > - int sleeping = 0;> > - unsigned long sleep = 0; /* shut up, gcc */> > - hwgroup->rq = NULL;> > - drive = hwgroup->drive;> > - do {> > - if ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&> > - (sleeping == 0 ||> > - time_before(drive->sleep, sleep))) {> > - sleeping = 1;> > - sleep = drive->sleep;> > - }> > - } while ((drive = drive->next) != hwgroup->drive);> > - if (sleeping) {> > + /*> > + * drive is doing pre-flush, ordered write, post-flush sequence. even> > + * though that is 3 requests, it must be seen as a single transaction.> > + * we must not preempt this drive until that is complete> > + */> > + if (blk_queue_flushing(q))> > /*> > - * Take a short snooze, and then wake up this hwgroup again.> > - * This gives other hwgroups on the same a chance to> > - * play fairly with us, just in case there are big differences> > - * in relative throughputs.. don't want to hog the cpu too much.> > + * small race where queue could get replugged during> > + * the 3-request flush cycle, just yank the plug since> > + * we want it to finish asap> > */> > - if (time_before(sleep, jiffies + WAIT_MIN_SLEEP))> > - sleep = jiffies + WAIT_MIN_SLEEP;> > -#if 1> > - if (timer_pending(&hwgroup->timer))> > - printk(KERN_CRIT "ide_set_handler: timer already active\n");> > -#endif> > - /* so that ide_timer_expiry knows what to do */> > - hwgroup->sleeping = 1;> > - hwgroup->req_gen_timer = hwgroup->req_gen;> > - mod_timer(&hwgroup->timer, sleep);> > - /* we purposely leave hwgroup locked> > - * while sleeping */> > - } else> > - ide_unlock_hwgroup(hwgroup);> > + blk_remove_plug(q);> > I'm not at all convinced that this works as expected. First of all, I> think we can safely assume that the plug is removed when block layer> calls into the ->request_fn(). Secondly, since the ide layer doesn't> call the ->request_fn() on it's own accord, I rather suspect that this> check can be dropped altogether. On the other hand, I'm not sure I agree +I suspect that this is leftover from the old code and I also think thatit can be removed completely. However mixing too many real code changesin a single patch is not a good practice and the removal can be handledindependently of the discussed patch. +If you would send me a patch with the above change I will be happy tointegrate it into pata tree (patch can be against current Linus' tree orlinux-next, either one is completely fine with me). +> with the rest of the patch anyway, see below.> > > > > - /* no more work for this hwgroup (for now) */> > - goto plug_device;> > - }> > + spin_unlock_irq(q->queue_lock);> > + spin_lock_irq(&hwgroup->lock);> > > > - if (drive != orig_drive)> > - goto plug_device;> > + /* caller must own hwgroup->lock */> > + BUG_ON(!irqs_disabled());> > Does this check really make any sense? The lock is being taken just two> lines before, after all. Indeed, removed. - -> > - hwif = drive->hwif; -> > + if (!ide_lock_hwgroup(hwgroup)) { -> > + hwgroup->rq = NULL; -> > + -> > + if (drive->dev_flags & IDE_DFLAG_SLEEPING) { -> > + if (time_before(drive->sleep, jiffies)) { -> > + ide_unlock_hwgroup(hwgroup); -> > + goto plug_device; -> > + } -> > + } -> > -> > if (hwif != hwgroup->hwif) { -> > /* -> > @@ -847,16 +750,20 @@ void do_ide_request(struct request_queue -> > hwgroup->hwif = hwif; -> > hwgroup->drive = drive; -> > drive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED); -> > - drive->service_start = jiffies; -> > -> > + spin_unlock_irq(&hwgroup->lock); -> > + spin_lock_irq(q->queue_lock); -> > /* -> > * we know that the queue isn't empty, but this can happen -> > * if the q->prep_rq_fn() decides to kill a request -> > */ -> > rq = elv_next_request(drive->queue); -> > + spin_unlock_irq(q->queue_lock); -> > + spin_lock_irq(&hwgroup->lock); -> > + -> > if (!rq) { -> > ide_unlock_hwgroup(hwgroup); -> > - break; -> > + goto out; -> > } -> > -> > /* -> > @@ -888,15 +795,22 @@ void do_ide_request(struct request_queue -> > -> > if (startstop == ide_stopped) { -> > ide_unlock_hwgroup(hwgroup); -> > - if (!elv_queue_empty(orig_drive->queue)) -> > - blk_plug_device(orig_drive->queue); -> > + /* give other devices a chance */ -> > + goto plug_device; -> -> This, I think, is very wrong. The rule of thumb for a ->requst_fn() -> should be to take as many requests off the queue as possible. Besides, -> this may easily preempt an ordered sequence as mentioned earlier. In my -> opinion, we really need a loop of sorts (while or goto). - +> > - hwif = drive->hwif;> > + if (!ide_lock_hwgroup(hwgroup)) {> > + hwgroup->rq = NULL;> > +> > + if (drive->dev_flags & IDE_DFLAG_SLEEPING) {> > + if (time_before(drive->sleep, jiffies)) {> > + ide_unlock_hwgroup(hwgroup);> > + goto plug_device;> > + }> > + }> > > > if (hwif != hwgroup->hwif) {> > /*> > @@ -847,16 +750,20 @@ void do_ide_request(struct request_queue> > hwgroup->hwif = hwif;> > hwgroup->drive = drive;> > drive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);> > - drive->service_start = jiffies;> > > > + spin_unlock_irq(&hwgroup->lock);> > + spin_lock_irq(q->queue_lock);> > /*> > * we know that the queue isn't empty, but this can happen> > * if the q->prep_rq_fn() decides to kill a request> > */> > rq = elv_next_request(drive->queue);> > + spin_unlock_irq(q->queue_lock);> > + spin_lock_irq(&hwgroup->lock);> > +> > if (!rq) {> > ide_unlock_hwgroup(hwgroup);> > - break;> > + goto out;> > }> > > > /*> > @@ -888,15 +795,22 @@ void do_ide_request(struct request_queue> > > > if (startstop == ide_stopped) {> > ide_unlock_hwgroup(hwgroup);> > - if (!elv_queue_empty(orig_drive->queue))> > - blk_plug_device(orig_drive->queue);> > + /* give other devices a chance */> > + goto plug_device;> > This, I think, is very wrong. The rule of thumb for a ->requst_fn()> should be to take as many requests off the queue as possible. Besides,> this may easily preempt an ordered sequence as mentioned earlier. In my> opinion, we really need a loop of sorts (while or goto). Thanks for noticing this. Fixed. - -[ The original idea behind "goto plug_device" is in the comment but as - I read the code again it doesn't make any sense now... ] - -> > } -> > - } -> > + } else -> > + goto plug_device; -> > +out: -> > + spin_unlock_irq(&hwgroup->lock); -> > + spin_lock_irq(q->queue_lock); -> > return; -> > -> > plug_device: -> > - if (!elv_queue_empty(orig_drive->queue)) -> > - blk_plug_device(orig_drive->queue); -> > + spin_unlock_irq(&hwgroup->lock); -> > + spin_lock_irq(q->queue_lock); -> > + -> > + if (!elv_queue_empty(q)) -> > + blk_plug_device(q); -> > } -> > -> > /* -> [...] -> > @@ -974,10 +899,12 @@ out: -> > void ide_timer_expiry (unsigned long data) -> > { -> > ide_hwgroup_t *hwgroup = (ide_hwgroup_t *) data; -> > + ide_drive_t *uninitialized_var(drive); -> > ide_handler_t *handler; -> > ide_expiry_t *expiry; -> > unsigned long flags; -> > unsigned long wait = -1; -> > + int plug_device = 0; -> > -> > spin_lock_irqsave(&hwgroup->lock, flags); -> > -> > @@ -989,12 +916,8 @@ void ide_timer_expiry (unsigned long dat -> > * or we were "sleeping" to give other devices a chance. -> > * Either way, we don't really want to complain about anything. -> > */ -> -> Not that important, I suppose, but you might want to update that comment -> while you're at it. - -I think that despite code changes it stays valid so there is no urgent need -to touch it.. - -> > - if (hwgroup->sleeping) { -> > - hwgroup->sleeping = 0; -> > - ide_unlock_hwgroup(hwgroup); -> > - } -> > } else { -> > - ide_drive_t *drive = hwgroup->drive; -> > + drive = hwgroup->drive; -> > if (!drive) { -> > printk(KERN_ERR "ide_timer_expiry: hwgroup->drive was NULL\n"); -> > hwgroup->handler = NULL; -> -> Finally, some more general blathering on the topic at hand: A while ago, -> I spent some thought on the possibilities of giving the block layer a -> notion of linked device queues as an equivalent hwgroups in ide, -> scsi_hosts or ata_ports and let it take care of time / bandwidth -> distribution among the queues belonging to one group. This is, as I -> understand, pretty much what your code is relying on since you have -> chucked out choose_drive(). However, this turned out not to be too easy - -This is the right way to go and I has always advocated for it. However -after seeing how libata got away with ignoring the issue altogether -I'm no longer sure of this. I haven't seen any bug reports which would -indicate that simplified approach has any really negative consequences. - -[ Still would be great to have the control over bandwitch of "queue-group" - at the block layer level since we could also use it for distributing the - available PCI[-E] bus bandwitch. ] - -> and I'm not quite sure whether we really want to go down that road. One -> major problem is that there is no straight forward way for the block -> layer to know, whether a ->request_fn() has actually taken a request off -> the queue and if not (or less than queue_depth anyway), whether it's -> just the device that couldn't take any more or the controller instead. -> On the whole, it seems not exactly trivial to get it right and it would -> probably be a good idea to consult Jens and perhaps James before - -I think that having more information returned by ->request_fn() could be -beneficial to the block layer (independently whether we end up adding -support for "queue-groups" to the block layer or not) but this definitely -needs to be verified with Jens & James. - -> embarking on such a venture. Short of that, I think that ide layer has -> to keep an appropriate equivalent of choose_drive() and also the while -> loop in the do_ide_request() function. - +[ The original idea behind "goto plug_device" is in the comment but as I read the code again it doesn't make any sense now... ] +> > }> > - }> > + } else> > + goto plug_device;> > +out:> > + spin_unlock_irq(&hwgroup->lock);> > + spin_lock_irq(q->queue_lock);> > return;> > > > plug_device:> > - if (!elv_queue_empty(orig_drive->queue))> > - blk_plug_device(orig_drive->queue);> > + spin_unlock_irq(&hwgroup->lock);> > + spin_lock_irq(q->queue_lock);> > +> > + if (!elv_queue_empty(q))> > + blk_plug_device(q);> > }> > > > /*> [...]> > @@ -974,10 +899,12 @@ out:> > void ide_timer_expiry (unsigned long data)> > {> > ide_hwgroup_t *hwgroup = (ide_hwgroup_t *) data;> > + ide_drive_t *uninitialized_var(drive);> > ide_handler_t *handler;> > ide_expiry_t *expiry;> > unsigned long flags;> > unsigned long wait = -1;> > + int plug_device = 0;> > > > spin_lock_irqsave(&hwgroup->lock, flags);> > > > @@ -989,12 +916,8 @@ void ide_timer_expiry (unsigned long dat> > * or we were "sleeping" to give other devices a chance.> > * Either way, we don't really want to complain about anything.> > */> > Not that important, I suppose, but you might want to update that comment> while you're at it. +I think that despite code changes it stays valid so there is no urgent needto touch it.. +> > - if (hwgroup->sleeping) {> > - hwgroup->sleeping = 0;> > - ide_unlock_hwgroup(hwgroup);> > - }> > } else {> > - ide_drive_t *drive = hwgroup->drive;> > + drive = hwgroup->drive;> > if (!drive) {> > printk(KERN_ERR "ide_timer_expiry: hwgroup->drive was NULL\n");> > hwgroup->handler = NULL;> > Finally, some more general blathering on the topic at hand: A while ago,> I spent some thought on the possibilities of giving the block layer a> notion of linked device queues as an equivalent hwgroups in ide,> scsi_hosts or ata_ports and let it take care of time / bandwidth> distribution among the queues belonging to one group. This is, as I> understand, pretty much what your code is relying on since you have> chucked out choose_drive(). However, this turned out not to be too easy +This is the right way to go and I has always advocated for it. Howeverafter seeing how libata got away with ignoring the issue altogetherI'm no longer sure of this. I haven't seen any bug reports which wouldindicate that simplified approach has any really negative consequences. +[ Still would be great to have the control over bandwitch of "queue-group" at the block layer level since we could also use it for distributing the available PCI[-E] bus bandwitch. ] +> and I'm not quite sure whether we really want to go down that road. One> major problem is that there is no straight forward way for the block> layer to know, whether a ->request_fn() has actually taken a request off> the queue and if not (or less than queue_depth anyway), whether it's> just the device that couldn't take any more or the controller instead.> On the whole, it seems not exactly trivial to get it right and it would> probably be a good idea to consult Jens and perhaps James before +I think that having more information returned by ->request_fn() could bebeneficial to the block layer (independently whether we end up addingsupport for "queue-groups" to the block layer or not) but this definitelyneeds to be verified with Jens & James. +> embarking on such a venture. Short of that, I think that ide layer has> to keep an appropriate equivalent of choose_drive() and also the while> loop in the do_ide_request() function. Thank you for your review. v1->v2 interdiff below. - -v2: -* Fixes/improvements based on review from Elias: - - take as many requests off the queue as possible - - remove now redundant BUG_ON() - -diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c ---- b/drivers/ide/ide-io.c -+++ b/drivers/ide/ide-io.c -@@ -726,10 +726,8 @@ - spin_unlock_irq(q->queue_lock); - spin_lock_irq(&hwgroup->lock); - -- /* caller must own hwgroup->lock */ -- BUG_ON(!irqs_disabled()); -- - if (!ide_lock_hwgroup(hwgroup)) { -+repeat: - hwgroup->rq = NULL; - - if (drive->dev_flags & IDE_DFLAG_SLEEPING) { -@@ -793,11 +791,8 @@ - startstop = start_request(drive, rq); - spin_lock_irq(&hwgroup->lock); - -- if (startstop == ide_stopped) { -- ide_unlock_hwgroup(hwgroup); -- /* give other devices a chance */ -- goto plug_device; -- } -+ if (startstop == ide_stopped) -+ goto repeat; - } else - goto plug_device; - out: -\0 +v2:* Fixes/improvements based on review from Elias: - take as many requests off the queue as possible - remove now redundant BUG_ON() +diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c--- b/drivers/ide/ide-io.c+++ b/drivers/ide/ide-io.c@@ -726,10 +726,8 @@ spin_unlock_irq(q->queue_lock); spin_lock_irq(&hwgroup->lock); - /* caller must own hwgroup->lock */- BUG_ON(!irqs_disabled());- if (!ide_lock_hwgroup(hwgroup)) {+repeat: hwgroup->rq = NULL; if (drive->dev_flags & IDE_DFLAG_SLEEPING) {@@ -793,11 +791,8 @@ startstop = start_request(drive, rq); spin_lock_irq(&hwgroup->lock); - if (startstop == ide_stopped) {- ide_unlock_hwgroup(hwgroup);- /* give other devices a chance */- goto plug_device;- }+ if (startstop == ide_stopped)+ goto repeat; } else goto plug_device; out:\0ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ diff --git a/a/content_digest b/N1/content_digest index 3d3f924..9489d0c 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -10,322 +10,24 @@ "b\0" "\n" "[ Once again, sorry for the long delay. ]\n" - "\n" - "On Monday 24 November 2008, Elias Oltmanns wrote:\n" - "> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:\n" - "> > * Move hack for flush requests from choose_drive() to do_ide_request().\n" - "> >\n" - "> > * Add ide_plug_device() helper and convert core IDE code from using\n" - "> > per-hwgroup lock as a request lock to use the ->queue_lock instead.\n" - "> >\n" - "> > * Remove no longer needed:\n" - "> > - choose_drive() function\n" - "> > - WAKEUP() macro\n" - "> > - 'sleeping' flag from ide_hwif_t\n" - "> > - 'service_{start,time}' fields from ide_drive_t\n" - "> >\n" - "> > This patch results in much simpler and more maintainable code\n" - "> > (besides being a scalability improvement).\n" - "> >\n" - "> > Cc: Elias Oltmanns <eo@nebensachen.de>\n" - "> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>\n" - "> > ---\n" - "> > newer version\n" - "> \n" - "> Eventually, I got round to having a look at this one (I reviewed the two\n" - "> small ones at the time). Apologies for the delay.\n" - "> \n" - "> >\n" - "> > drivers/ide/ide-io.c | 213 +++++++++++++++---------------------------------\n" - "> > drivers/ide/ide-park.c | 13 +-\n" - "> > drivers/ide/ide-probe.c | 3 \n" - "> > include/linux/ide.h | 4 \n" - "> > 4 files changed, 79 insertions(+), 154 deletions(-)\n" - "> >\n" - "> > Index: b/drivers/ide/ide-io.c\n" - "> > ===================================================================\n" - "> > --- a/drivers/ide/ide-io.c\n" - "> > +++ b/drivers/ide/ide-io.c\n" - "> [...]\n" - "> > @@ -780,61 +704,40 @@ repeat:\t\n" - "> > */\n" - "> > void do_ide_request(struct request_queue *q)\n" - "> > {\n" - "> > -\tide_drive_t\t*orig_drive = q->queuedata;\n" - "> > -\tide_hwgroup_t\t*hwgroup = orig_drive->hwif->hwgroup;\n" - "> > -\tide_drive_t\t*drive;\n" - "> > -\tide_hwif_t\t*hwif;\n" - "> > +\tide_drive_t\t*drive = q->queuedata;\n" - "> > +\tide_hwif_t\t*hwif = drive->hwif;\n" - "> > +\tide_hwgroup_t\t*hwgroup = hwif->hwgroup;\n" - "> > \tstruct request\t*rq;\n" - "> > \tide_startstop_t\tstartstop;\n" - "> > \n" - "> > -\t/* caller must own hwgroup->lock */\n" - "> > -\tBUG_ON(!irqs_disabled());\n" - "> > -\n" - "> > -\twhile (!ide_lock_hwgroup(hwgroup)) {\n" - "> > -\t\tdrive = choose_drive(hwgroup);\n" - "> > -\t\tif (drive == NULL) {\n" - "> > -\t\t\tint sleeping = 0;\n" - "> > -\t\t\tunsigned long sleep = 0; /* shut up, gcc */\n" - "> > -\t\t\thwgroup->rq = NULL;\n" - "> > -\t\t\tdrive = hwgroup->drive;\n" - "> > -\t\t\tdo {\n" - "> > -\t\t\t\tif ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&\n" - "> > -\t\t\t\t (sleeping == 0 ||\n" - "> > -\t\t\t\t time_before(drive->sleep, sleep))) {\n" - "> > -\t\t\t\t\tsleeping = 1;\n" - "> > -\t\t\t\t\tsleep = drive->sleep;\n" - "> > -\t\t\t\t}\n" - "> > -\t\t\t} while ((drive = drive->next) != hwgroup->drive);\n" - "> > -\t\t\tif (sleeping) {\n" - "> > +\t/*\n" - "> > +\t * drive is doing pre-flush, ordered write, post-flush sequence. even\n" - "> > +\t * though that is 3 requests, it must be seen as a single transaction.\n" - "> > +\t * we must not preempt this drive until that is complete\n" - "> > +\t */\n" - "> > +\tif (blk_queue_flushing(q))\n" - "> > \t\t/*\n" - "> > -\t\t * Take a short snooze, and then wake up this hwgroup again.\n" - "> > -\t\t * This gives other hwgroups on the same a chance to\n" - "> > -\t\t * play fairly with us, just in case there are big differences\n" - "> > -\t\t * in relative throughputs.. don't want to hog the cpu too much.\n" - "> > +\t\t * small race where queue could get replugged during\n" - "> > +\t\t * the 3-request flush cycle, just yank the plug since\n" - "> > +\t\t * we want it to finish asap\n" - "> > \t\t */\n" - "> > -\t\t\t\tif (time_before(sleep, jiffies + WAIT_MIN_SLEEP))\n" - "> > -\t\t\t\t\tsleep = jiffies + WAIT_MIN_SLEEP;\n" - "> > -#if 1\n" - "> > -\t\t\t\tif (timer_pending(&hwgroup->timer))\n" - "> > -\t\t\t\t\tprintk(KERN_CRIT \"ide_set_handler: timer already active\\n\");\n" - "> > -#endif\n" - "> > -\t\t\t\t/* so that ide_timer_expiry knows what to do */\n" - "> > -\t\t\t\thwgroup->sleeping = 1;\n" - "> > -\t\t\t\thwgroup->req_gen_timer = hwgroup->req_gen;\n" - "> > -\t\t\t\tmod_timer(&hwgroup->timer, sleep);\n" - "> > -\t\t\t\t/* we purposely leave hwgroup locked\n" - "> > -\t\t\t\t * while sleeping */\n" - "> > -\t\t\t} else\n" - "> > -\t\t\t\tide_unlock_hwgroup(hwgroup);\n" - "> > +\t\tblk_remove_plug(q);\n" - "> \n" - "> I'm not at all convinced that this works as expected. First of all, I\n" - "> think we can safely assume that the plug is removed when block layer\n" - "> calls into the ->request_fn(). Secondly, since the ide layer doesn't\n" - "> call the ->request_fn() on it's own accord, I rather suspect that this\n" - "> check can be dropped altogether. On the other hand, I'm not sure I agree\n" - "\n" - "I suspect that this is leftover from the old code and I also think that\n" - "it can be removed completely. However mixing too many real code changes\n" - "in a single patch is not a good practice and the removal can be handled\n" - "independently of the discussed patch.\n" - "\n" - "If you would send me a patch with the above change I will be happy to\n" - "integrate it into pata tree (patch can be against current Linus' tree or\n" - "linux-next, either one is completely fine with me).\n" - "\n" - "> with the rest of the patch anyway, see below.\n" - "> \n" - "> > \n" - "> > -\t\t\t/* no more work for this hwgroup (for now) */\n" - "> > -\t\t\tgoto plug_device;\n" - "> > -\t\t}\n" - "> > +\tspin_unlock_irq(q->queue_lock);\n" - "> > +\tspin_lock_irq(&hwgroup->lock);\n" - "> > \n" - "> > -\t\tif (drive != orig_drive)\n" - "> > -\t\t\tgoto plug_device;\n" - "> > +\t/* caller must own hwgroup->lock */\n" - "> > +\tBUG_ON(!irqs_disabled());\n" - "> \n" - "> Does this check really make any sense? The lock is being taken just two\n" - "> lines before, after all.\n" - "\n" + "On Monday 24 November 2008, Elias Oltmanns wrote:> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:> > * Move hack for flush requests from choose_drive() to do_ide_request().> >> > * Add ide_plug_device() helper and convert core IDE code from using> > per-hwgroup lock as a request lock to use the ->queue_lock instead.> >> > * Remove no longer needed:> > - choose_drive() function> > - WAKEUP() macro> > - 'sleeping' flag from ide_hwif_t> > - 'service_{start,time}' fields from ide_drive_t> >> > This patch results in much simpler and more maintainable code> > (besides being a scalability improvement).> >> > Cc: Elias Oltmanns <eo@nebensachen.de>> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>> > ---> > newer version> > Eventually, I got round to having a look at this one (I reviewed the two> small ones at the time). Apologies for the delay.> > >> > drivers/ide/ide-io.c | 213 +++++++++++++++---------------------------------> > drivers/ide/ide-park.c | 13 +-> > drivers/ide/ide-probe.c | 3 > > include/linux/ide.h | 4 > > 4 files changed, 79 insertions(+), 154 deletions(-)> >> > Index: b/drivers/ide/ide-io.c> > ===================================================================> > --- a/drivers/ide/ide-io.c> > +++ b/drivers/ide/ide-io.c> [...]> > @@ -780,61 +704,40 @@ repeat:\t> > */> > void do_ide_request(struct request_queue *q)> > {> > -\tide_drive_t\t*orig_drive = q->queuedata;> > -\tide_hwgroup_t\t*hwgroup = orig_drive->hwif->hwgroup;> > -\tide_drive_t\t*drive;> > -\tide_hwif_t\t*hwif;> > +\tide_drive_t\t*drive = q->queuedata;> > +\tide_hwif_t\t*hwif = drive->hwif;> > +\tide_hwgroup_t\t*hwgroup = hwif->hwgroup;> > \tstruct request\t*rq;> > \tide_startstop_t\tstartstop;> > > > -\t/* caller must own hwgroup->lock */> > -\tBUG_ON(!irqs_disabled());> > -> > -\twhile (!ide_lock_hwgroup(hwgroup)) {> > -\t\tdrive = choose_drive(hwgroup);> > -\t\tif (drive == NULL) {> > -\t\t\tint sleeping = 0;> > -\t\t\tunsigned long sleep = 0; /* shut up, gcc */> > -\t\t\thwgroup->rq = NULL;> > -\t\t\tdrive = hwgroup->drive;> > -\t\t\tdo {> > -\t\t\t\tif ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&> > -\t\t\t\t (sleeping == 0 ||> > -\t\t\t\t time_before(drive->sleep, sleep))) {> > -\t\t\t\t\tsleeping = 1;> > -\t\t\t\t\tsleep = drive->sleep;> > -\t\t\t\t}> > -\t\t\t} while ((drive = drive->next) != hwgroup->drive);> > -\t\t\tif (sleeping) {> > +\t/*> > +\t * drive is doing pre-flush, ordered write, post-flush sequence. even> > +\t * though that is 3 requests, it must be seen as a single transaction.> > +\t * we must not preempt this drive until that is complete> > +\t */> > +\tif (blk_queue_flushing(q))> > \t\t/*> > -\t\t * Take a short snooze, and then wake up this hwgroup again.> > -\t\t * This gives other hwgroups on the same a chance to> > -\t\t * play fairly with us, just in case there are big differences> > -\t\t * in relative throughputs.. don't want to hog the cpu too much.> > +\t\t * small race where queue could get replugged during> > +\t\t * the 3-request flush cycle, just yank the plug since> > +\t\t * we want it to finish asap> > \t\t */> > -\t\t\t\tif (time_before(sleep, jiffies + WAIT_MIN_SLEEP))> > -\t\t\t\t\tsleep = jiffies + WAIT_MIN_SLEEP;> > -#if 1> > -\t\t\t\tif (timer_pending(&hwgroup->timer))> > -\t\t\t\t\tprintk(KERN_CRIT \"ide_set_handler: timer already active\\n\");> > -#endif> > -\t\t\t\t/* so that ide_timer_expiry knows what to do */> > -\t\t\t\thwgroup->sleeping = 1;> > -\t\t\t\thwgroup->req_gen_timer = hwgroup->req_gen;> > -\t\t\t\tmod_timer(&hwgroup->timer, sleep);> > -\t\t\t\t/* we purposely leave hwgroup locked> > -\t\t\t\t * while sleeping */> > -\t\t\t} else> > -\t\t\t\tide_unlock_hwgroup(hwgroup);> > +\t\tblk_remove_plug(q);> > I'm not at all convinced that this works as expected. First of all, I> think we can safely assume that the plug is removed when block layer> calls into the ->request_fn(). Secondly, since the ide layer doesn't> call the ->request_fn() on it's own accord, I rather suspect that this> check can be dropped altogether. On the other hand, I'm not sure I agree\n" + "I suspect that this is leftover from the old code and I also think thatit can be removed completely. However mixing too many real code changesin a single patch is not a good practice and the removal can be handledindependently of the discussed patch.\n" + "If you would send me a patch with the above change I will be happy tointegrate it into pata tree (patch can be against current Linus' tree orlinux-next, either one is completely fine with me).\n" + "> with the rest of the patch anyway, see below.> > > > > -\t\t\t/* no more work for this hwgroup (for now) */> > -\t\t\tgoto plug_device;> > -\t\t}> > +\tspin_unlock_irq(q->queue_lock);> > +\tspin_lock_irq(&hwgroup->lock);> > > > -\t\tif (drive != orig_drive)> > -\t\t\tgoto plug_device;> > +\t/* caller must own hwgroup->lock */> > +\tBUG_ON(!irqs_disabled());> > Does this check really make any sense? The lock is being taken just two> lines before, after all.\n" "Indeed, removed.\n" - "\n" - "> > -\t\thwif = drive->hwif;\n" - "> > +\tif (!ide_lock_hwgroup(hwgroup)) {\n" - "> > +\t\thwgroup->rq = NULL;\n" - "> > +\n" - "> > +\t\tif (drive->dev_flags & IDE_DFLAG_SLEEPING) {\n" - "> > +\t\t\tif (time_before(drive->sleep, jiffies)) {\n" - "> > +\t\t\t\tide_unlock_hwgroup(hwgroup);\n" - "> > +\t\t\t\tgoto plug_device;\n" - "> > +\t\t\t}\n" - "> > +\t\t}\n" - "> > \n" - "> > \t\tif (hwif != hwgroup->hwif) {\n" - "> > \t\t\t/*\n" - "> > @@ -847,16 +750,20 @@ void do_ide_request(struct request_queue\n" - "> > \t\thwgroup->hwif = hwif;\n" - "> > \t\thwgroup->drive = drive;\n" - "> > \t\tdrive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);\n" - "> > -\t\tdrive->service_start = jiffies;\n" - "> > \n" - "> > +\t\tspin_unlock_irq(&hwgroup->lock);\n" - "> > +\t\tspin_lock_irq(q->queue_lock);\n" - "> > \t\t/*\n" - "> > \t\t * we know that the queue isn't empty, but this can happen\n" - "> > \t\t * if the q->prep_rq_fn() decides to kill a request\n" - "> > \t\t */\n" - "> > \t\trq = elv_next_request(drive->queue);\n" - "> > +\t\tspin_unlock_irq(q->queue_lock);\n" - "> > +\t\tspin_lock_irq(&hwgroup->lock);\n" - "> > +\n" - "> > \t\tif (!rq) {\n" - "> > \t\t\tide_unlock_hwgroup(hwgroup);\n" - "> > -\t\t\tbreak;\n" - "> > +\t\t\tgoto out;\n" - "> > \t\t}\n" - "> > \n" - "> > \t\t/*\n" - "> > @@ -888,15 +795,22 @@ void do_ide_request(struct request_queue\n" - "> > \n" - "> > \t\tif (startstop == ide_stopped) {\n" - "> > \t\t\tide_unlock_hwgroup(hwgroup);\n" - "> > -\t\t\tif (!elv_queue_empty(orig_drive->queue))\n" - "> > -\t\t\t\tblk_plug_device(orig_drive->queue);\n" - "> > +\t\t\t/* give other devices a chance */\n" - "> > +\t\t\tgoto plug_device;\n" - "> \n" - "> This, I think, is very wrong. The rule of thumb for a ->requst_fn()\n" - "> should be to take as many requests off the queue as possible. Besides,\n" - "> this may easily preempt an ordered sequence as mentioned earlier. In my\n" - "> opinion, we really need a loop of sorts (while or goto).\n" - "\n" + "> > -\t\thwif = drive->hwif;> > +\tif (!ide_lock_hwgroup(hwgroup)) {> > +\t\thwgroup->rq = NULL;> > +> > +\t\tif (drive->dev_flags & IDE_DFLAG_SLEEPING) {> > +\t\t\tif (time_before(drive->sleep, jiffies)) {> > +\t\t\t\tide_unlock_hwgroup(hwgroup);> > +\t\t\t\tgoto plug_device;> > +\t\t\t}> > +\t\t}> > > > \t\tif (hwif != hwgroup->hwif) {> > \t\t\t/*> > @@ -847,16 +750,20 @@ void do_ide_request(struct request_queue> > \t\thwgroup->hwif = hwif;> > \t\thwgroup->drive = drive;> > \t\tdrive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);> > -\t\tdrive->service_start = jiffies;> > > > +\t\tspin_unlock_irq(&hwgroup->lock);> > +\t\tspin_lock_irq(q->queue_lock);> > \t\t/*> > \t\t * we know that the queue isn't empty, but this can happen> > \t\t * if the q->prep_rq_fn() decides to kill a request> > \t\t */> > \t\trq = elv_next_request(drive->queue);> > +\t\tspin_unlock_irq(q->queue_lock);> > +\t\tspin_lock_irq(&hwgroup->lock);> > +> > \t\tif (!rq) {> > \t\t\tide_unlock_hwgroup(hwgroup);> > -\t\t\tbreak;> > +\t\t\tgoto out;> > \t\t}> > > > \t\t/*> > @@ -888,15 +795,22 @@ void do_ide_request(struct request_queue> > > > \t\tif (startstop == ide_stopped) {> > \t\t\tide_unlock_hwgroup(hwgroup);> > -\t\t\tif (!elv_queue_empty(orig_drive->queue))> > -\t\t\t\tblk_plug_device(orig_drive->queue);> > +\t\t\t/* give other devices a chance */> > +\t\t\tgoto plug_device;> > This, I think, is very wrong. The rule of thumb for a ->requst_fn()> should be to take as many requests off the queue as possible. Besides,> this may easily preempt an ordered sequence as mentioned earlier. In my> opinion, we really need a loop of sorts (while or goto).\n" "Thanks for noticing this. Fixed.\n" - "\n" - "[ The original idea behind \"goto plug_device\" is in the comment but as\n" - " I read the code again it doesn't make any sense now... ]\n" - "\n" - "> > \t\t}\n" - "> > -\t}\n" - "> > +\t} else\n" - "> > +\t\tgoto plug_device;\n" - "> > +out:\n" - "> > +\tspin_unlock_irq(&hwgroup->lock);\n" - "> > +\tspin_lock_irq(q->queue_lock);\n" - "> > \treturn;\n" - "> > \n" - "> > plug_device:\n" - "> > -\tif (!elv_queue_empty(orig_drive->queue))\n" - "> > -\t\tblk_plug_device(orig_drive->queue);\n" - "> > +\tspin_unlock_irq(&hwgroup->lock);\n" - "> > +\tspin_lock_irq(q->queue_lock);\n" - "> > +\n" - "> > +\tif (!elv_queue_empty(q))\n" - "> > +\t\tblk_plug_device(q);\n" - "> > }\n" - "> > \n" - "> > /*\n" - "> [...]\n" - "> > @@ -974,10 +899,12 @@ out:\n" - "> > void ide_timer_expiry (unsigned long data)\n" - "> > {\n" - "> > \tide_hwgroup_t\t*hwgroup = (ide_hwgroup_t *) data;\n" - "> > +\tide_drive_t\t*uninitialized_var(drive);\n" - "> > \tide_handler_t\t*handler;\n" - "> > \tide_expiry_t\t*expiry;\n" - "> > \tunsigned long\tflags;\n" - "> > \tunsigned long\twait = -1;\n" - "> > +\tint\t\tplug_device = 0;\n" - "> > \n" - "> > \tspin_lock_irqsave(&hwgroup->lock, flags);\n" - "> > \n" - "> > @@ -989,12 +916,8 @@ void ide_timer_expiry (unsigned long dat\n" - "> > \t\t * or we were \"sleeping\" to give other devices a chance.\n" - "> > \t\t * Either way, we don't really want to complain about anything.\n" - "> > \t\t */\n" - "> \n" - "> Not that important, I suppose, but you might want to update that comment\n" - "> while you're at it.\n" - "\n" - "I think that despite code changes it stays valid so there is no urgent need\n" - "to touch it..\n" - "\n" - "> > -\t\tif (hwgroup->sleeping) {\n" - "> > -\t\t\thwgroup->sleeping = 0;\n" - "> > -\t\t\tide_unlock_hwgroup(hwgroup);\n" - "> > -\t\t}\n" - "> > \t} else {\n" - "> > -\t\tide_drive_t *drive = hwgroup->drive;\n" - "> > +\t\tdrive = hwgroup->drive;\n" - "> > \t\tif (!drive) {\n" - "> > \t\t\tprintk(KERN_ERR \"ide_timer_expiry: hwgroup->drive was NULL\\n\");\n" - "> > \t\t\thwgroup->handler = NULL;\n" - "> \n" - "> Finally, some more general blathering on the topic at hand: A while ago,\n" - "> I spent some thought on the possibilities of giving the block layer a\n" - "> notion of linked device queues as an equivalent hwgroups in ide,\n" - "> scsi_hosts or ata_ports and let it take care of time / bandwidth\n" - "> distribution among the queues belonging to one group. This is, as I\n" - "> understand, pretty much what your code is relying on since you have\n" - "> chucked out choose_drive(). However, this turned out not to be too easy\n" - "\n" - "This is the right way to go and I has always advocated for it. However\n" - "after seeing how libata got away with ignoring the issue altogether\n" - "I'm no longer sure of this. I haven't seen any bug reports which would\n" - "indicate that simplified approach has any really negative consequences.\n" - "\n" - "[ Still would be great to have the control over bandwitch of \"queue-group\"\n" - " at the block layer level since we could also use it for distributing the\n" - " available PCI[-E] bus bandwitch. ]\n" - "\n" - "> and I'm not quite sure whether we really want to go down that road. One\n" - "> major problem is that there is no straight forward way for the block\n" - "> layer to know, whether a ->request_fn() has actually taken a request off\n" - "> the queue and if not (or less than queue_depth anyway), whether it's\n" - "> just the device that couldn't take any more or the controller instead.\n" - "> On the whole, it seems not exactly trivial to get it right and it would\n" - "> probably be a good idea to consult Jens and perhaps James before\n" - "\n" - "I think that having more information returned by ->request_fn() could be\n" - "beneficial to the block layer (independently whether we end up adding\n" - "support for \"queue-groups\" to the block layer or not) but this definitely\n" - "needs to be verified with Jens & James.\n" - "\n" - "> embarking on such a venture. Short of that, I think that ide layer has\n" - "> to keep an appropriate equivalent of choose_drive() and also the while\n" - "> loop in the do_ide_request() function.\n" - "\n" + "[ The original idea behind \"goto plug_device\" is in the comment but as I read the code again it doesn't make any sense now... ]\n" + "> > \t\t}> > -\t}> > +\t} else> > +\t\tgoto plug_device;> > +out:> > +\tspin_unlock_irq(&hwgroup->lock);> > +\tspin_lock_irq(q->queue_lock);> > \treturn;> > > > plug_device:> > -\tif (!elv_queue_empty(orig_drive->queue))> > -\t\tblk_plug_device(orig_drive->queue);> > +\tspin_unlock_irq(&hwgroup->lock);> > +\tspin_lock_irq(q->queue_lock);> > +> > +\tif (!elv_queue_empty(q))> > +\t\tblk_plug_device(q);> > }> > > > /*> [...]> > @@ -974,10 +899,12 @@ out:> > void ide_timer_expiry (unsigned long data)> > {> > \tide_hwgroup_t\t*hwgroup = (ide_hwgroup_t *) data;> > +\tide_drive_t\t*uninitialized_var(drive);> > \tide_handler_t\t*handler;> > \tide_expiry_t\t*expiry;> > \tunsigned long\tflags;> > \tunsigned long\twait = -1;> > +\tint\t\tplug_device = 0;> > > > \tspin_lock_irqsave(&hwgroup->lock, flags);> > > > @@ -989,12 +916,8 @@ void ide_timer_expiry (unsigned long dat> > \t\t * or we were \"sleeping\" to give other devices a chance.> > \t\t * Either way, we don't really want to complain about anything.> > \t\t */> > Not that important, I suppose, but you might want to update that comment> while you're at it.\n" + "I think that despite code changes it stays valid so there is no urgent needto touch it..\n" + "> > -\t\tif (hwgroup->sleeping) {> > -\t\t\thwgroup->sleeping = 0;> > -\t\t\tide_unlock_hwgroup(hwgroup);> > -\t\t}> > \t} else {> > -\t\tide_drive_t *drive = hwgroup->drive;> > +\t\tdrive = hwgroup->drive;> > \t\tif (!drive) {> > \t\t\tprintk(KERN_ERR \"ide_timer_expiry: hwgroup->drive was NULL\\n\");> > \t\t\thwgroup->handler = NULL;> > Finally, some more general blathering on the topic at hand: A while ago,> I spent some thought on the possibilities of giving the block layer a> notion of linked device queues as an equivalent hwgroups in ide,> scsi_hosts or ata_ports and let it take care of time / bandwidth> distribution among the queues belonging to one group. This is, as I> understand, pretty much what your code is relying on since you have> chucked out choose_drive(). However, this turned out not to be too easy\n" + "This is the right way to go and I has always advocated for it. Howeverafter seeing how libata got away with ignoring the issue altogetherI'm no longer sure of this. I haven't seen any bug reports which wouldindicate that simplified approach has any really negative consequences.\n" + "[ Still would be great to have the control over bandwitch of \"queue-group\" at the block layer level since we could also use it for distributing the available PCI[-E] bus bandwitch. ]\n" + "> and I'm not quite sure whether we really want to go down that road. One> major problem is that there is no straight forward way for the block> layer to know, whether a ->request_fn() has actually taken a request off> the queue and if not (or less than queue_depth anyway), whether it's> just the device that couldn't take any more or the controller instead.> On the whole, it seems not exactly trivial to get it right and it would> probably be a good idea to consult Jens and perhaps James before\n" + "I think that having more information returned by ->request_fn() could bebeneficial to the block layer (independently whether we end up addingsupport for \"queue-groups\" to the block layer or not) but this definitelyneeds to be verified with Jens & James.\n" + "> embarking on such a venture. Short of that, I think that ide layer has> to keep an appropriate equivalent of choose_drive() and also the while> loop in the do_ide_request() function.\n" "Thank you for your review. v1->v2 interdiff below.\n" - "\n" - "v2:\n" - "* Fixes/improvements based on review from Elias:\n" - " - take as many requests off the queue as possible\n" - " - remove now redundant BUG_ON()\n" - "\n" - "diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c\n" - "--- b/drivers/ide/ide-io.c\n" - "+++ b/drivers/ide/ide-io.c\n" - "@@ -726,10 +726,8 @@\n" - " \tspin_unlock_irq(q->queue_lock);\n" - " \tspin_lock_irq(&hwgroup->lock);\n" - " \n" - "-\t/* caller must own hwgroup->lock */\n" - "-\tBUG_ON(!irqs_disabled());\n" - "-\n" - " \tif (!ide_lock_hwgroup(hwgroup)) {\n" - "+repeat:\n" - " \t\thwgroup->rq = NULL;\n" - " \n" - " \t\tif (drive->dev_flags & IDE_DFLAG_SLEEPING) {\n" - "@@ -793,11 +791,8 @@\n" - " \t\tstartstop = start_request(drive, rq);\n" - " \t\tspin_lock_irq(&hwgroup->lock);\n" - " \n" - "-\t\tif (startstop == ide_stopped) {\n" - "-\t\t\tide_unlock_hwgroup(hwgroup);\n" - "-\t\t\t/* give other devices a chance */\n" - "-\t\t\tgoto plug_device;\n" - "-\t\t}\n" - "+\t\tif (startstop == ide_stopped)\n" - "+\t\t\tgoto repeat;\n" - " \t} else\n" - " \t\tgoto plug_device;\n" - " out:\n" - "\0" + "v2:* Fixes/improvements based on review from Elias: - take as many requests off the queue as possible - remove now redundant BUG_ON()\n" + "diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c--- b/drivers/ide/ide-io.c+++ b/drivers/ide/ide-io.c@@ -726,10 +726,8 @@ \tspin_unlock_irq(q->queue_lock); \tspin_lock_irq(&hwgroup->lock); -\t/* caller must own hwgroup->lock */-\tBUG_ON(!irqs_disabled());- \tif (!ide_lock_hwgroup(hwgroup)) {+repeat: \t\thwgroup->rq = NULL; \t\tif (drive->dev_flags & IDE_DFLAG_SLEEPING) {@@ -793,11 +791,8 @@ \t\tstartstop = start_request(drive, rq); \t\tspin_lock_irq(&hwgroup->lock); -\t\tif (startstop == ide_stopped) {-\t\t\tide_unlock_hwgroup(hwgroup);-\t\t\t/* give other devices a chance */-\t\t\tgoto plug_device;-\t\t}+\t\tif (startstop == ide_stopped)+\t\t\tgoto repeat; \t} else \t\tgoto plug_device; out:\0\303\277\303\264\303\250\302\272{.n\303\207+\302\211\302\267\302\237\302\256\302\211\302\255\302\206+%\302\212\303\213\303\277\302\261\303\251\303\235\302\266\027\302\245\302\212w\303\277\302\272{.n\303\207+\302\211\302\267\302\245\302\212{\302\261\303\276G\302\253\302\235\303\251\303\277\302\212{ay\302\272\035\303\212\302\207\303\232\302\231\303\253,j\a\302\255\302\242f\302\243\302\242\302\267h\302\232\302\217\303\257\302\201\303\252\303\277\302\221\303\252\303\247z_\303\250\302\256\003(\302\255\303\251\302\232\302\216\302\212\303\235\302\242j\"\302\235\303\272\032\302\266\033m\302\247\303\277\303\277\302\276\a\302\253\303\276G\302\253\302\235\303\251\303\277\302\242\302\270?\302\231\302\250\303\250\302\255\303\232&\302\243\303\270\302\247~\302\217\303\241\302\266iO\302\225\303\246\302\254z\302\267\302\232v\303\230^\024\004\032\302\266\033m\302\247\303\277\303\277\303\203\f\303\277\302\266\303\254\303\277\302\242\302\270?\302\226I\302\245" -217507cd22d923c2eb7b9c6c8ca064019875b794a7915a7612242de2ad7014d6 +f5a8a1dabec7ef47d773793a0580b0b5d80ff153bf165da3ca27957c9b54570e
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.