On Mon, Apr 15 2002, Martin Dalecki wrote:
> Jens Axboe wrote:
> >Hi,
> >
> >Updated the patch to 2.5.8 (painful). Changes:
>
> S*it - I did just offer you assistance... well you where faster.
> Anyway thank you for going into this trouble.
:-)
> >
> >diff -urN -X /home/axboe/cdrom/exclude
> >/opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c
> >linux/drivers/block/ll_rw_blk.c
> >--- /opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c 2002-04-15
> >07:59:53.000000000 +0200
> >+++ linux/drivers/block/ll_rw_blk.c 2002-04-15 08:42:23.000000000 +0200
>
> I recognize the following makes sense but since
> it's affecting every single type of device out there
> it should go separetely to Linus I think...
They have.
> >+ drive as a /\/\/\/\ queue size balance, where we could instead try and
> >+ maintain a minimum queue size and get a /---------\ graph instead.
>
> Nice drawings :-). But the help text shouldn't be
> that encouraging right now IMHO... (At least
> we should mark the options as experimental. (OK I will do that...)
It's no more experiemental than the rest of it, in fact all my testing
has been with _FULL option enabled right now. So I'd basically say it's
the recommended setting :-)
> >diff -urN -X /home/axboe/cdrom/exclude
> >/opt/kernel/linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
> >--- /opt/kernel/linux-2.5.8/drivers/ide/ide.c 2002-04-15
> >10:05:03.000000000 +0200
> >+++ linux/drivers/ide/ide.c 2002-04-15 14:31:08.000000000 +0200
> >@@ -381,8 +381,23 @@
> > add_blkdev_randomness(major(rq->rq_dev));
> >
> > spin_lock_irqsave(&ide_lock, flags);
> >+
> >+ if ((jiffies - ar->ar_time > ATA_AR_MAX_TURNAROUND) &&
> >drive->queue_depth > 1) {
> >+ printk("%s: exceeded max command turn-around time
> >(%d seconds)\n", drive->name, ATA_AR_MAX_TURNAROUND / HZ);
> >+ drive->queue_depth >>= 1;
> >+ }
> >+
> >+ if (jiffies - ar->ar_time > drive->tcq->oldest_command)
> >+ drive->tcq->oldest_command = jiffies - ar->ar_time;
> >+
>
> time_before() and timer_after() should be used here.
Not necessarily, the above is wrap safe.
> > while ((read_timer() - hwif->last_time) < DISK_RECOVERY_TIME);
>
> Same here.
Ditto, at least for this one I agree that readibility would be better
with the macros.
> >-#ifdef CONFIG_BLK_DEV_IDEPCI
> > if (hwif->pci_dev && !hwif->pci_dev->vendor)
> >-#endif
>
> Here I'm still not convinced whatever this should be implemented for
> all architectures...
Agree, we can leave this the way it was for now and just do the nasty
ifdefs.
> >- memset(ar, 0, sizeof(*ar));
> >+ memset(ar, 0, sizeof(ar));
>
> Please look closer - I'm quite convinced that sizeof(ar) == sizeof(void *)
> which gives not the desired effect. I have fixed this during the last
> merge...
Irk, where did that come from?! Had you done the incremental patches
this would not have slipped through! My mistake.
> > static int ide_check_media_change (kdev_t i_rdev)
> >diff -urN -X /home/axboe/cdrom/exclude
> >/opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
> >--- /opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c 2002-04-15
> >07:59:53.000000000 +0200
> >+++ linux/drivers/ide/ide-disk.c 2002-04-15 10:20:33.000000000 +0200
> >@@ -107,10 +107,7 @@
> > * 48-bit commands are pretty sanely laid out
> > */
> > if (lba48bit) {
> >- if (cmd == READ)
> >- command = WIN_READ_EXT;
> >- else
> >- command = WIN_WRITE_EXT;
> >+ command = cmd == READ ? WIN_READ_EXT : WIN_WRITE_EXT;
>
> Checking with GCC will reveal that the former version doesn't
> generate worser code...
I had already rewritten it by the time your changed version was out, so
it was simply unchanged.
> > return command;
> >@@ -895,24 +894,24 @@
> >
> > __set_bit(i, &tag_mask);
> > len += sprintf(out+len, "%d, ", i);
> >- if (ar->ar_time > max_jif)
> >- max_jif = ar->ar_time;
> >+ if (cur_jif - ar->ar_time > max_jif)
> >+ max_jif = cur_jif - ar->ar_time;
>
> I disgust timer calculartions...- will have to look at a way
> how to nap in a similar way eth drivers do.
? This is just statistics. There absolutely nothing wrong with the
above.
> >diff -urN -X /home/axboe/cdrom/exclude
> >/opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
> >--- /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c 2002-04-15
> >07:59:53.000000000 +0200
> >+++ linux/drivers/ide/ide-dma.c 2002-04-15 09:17:55.000000000 +0200
> >
> >+#endif /* CONFIG_BLK_DEV_IDE_TCQ */
> >
> > case ide_dma_read:
> > reading = 1 << 3;
> > case ide_dma_write:
> >- ar = HWGROUP(drive)->rq->special;
> >+ ar = IDE_CUR_AR(drive);
>
> Ahhh!!! I'm gald to see this.
:-)
> > static inline void drive_ctl_nien(ide_drive_t *drive, int clear)
> > {
> > #ifdef IDE_TCQ_NIEN
> >- int mask = clear ? 0 : 2;
> >+ int mask = clear ? 0 : 1 << 1;
> ????
Just more readable that we are setting bit 2.
> >-#define IDE_CUR_AR(drive) \
> >- ((drive)->using_tcq ? IDE_CUR_TAG((drive)) :
> >HWGROUP((drive))->rq->special)
> >+#define IDE_CUR_AR(drive) (HWGROUP((drive))->rq->special)
>
> Ahh that's nice :-). Let's look further down how this coexists with
> ide-cd.c...
>
>
> >-extern inline int ide_get_tag(ide_drive_t *drive)
> >+static inline int ide_get_tag(ide_drive_t *drive)
>
> OK. I have missed this one apparently.
>
> ar_timer will make it at least esier to finish the ide-cd.c transition
> to this data transport base.
>
> Should I just quickly remerge this, so we can work further from
> the same code base to ease the merging pain?
Sure, go ahead. And do the changes they way we discussed earlier today,
ok? :)
-- Jens Axboe- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Mon Apr 15 2002 - 22:00:24 EST