* [PATCH 12/12] cciss: fix for iostat
@ 2006-11-06 20:32 Mike Miller (OS Dev)
2006-11-06 20:45 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Mike Miller (OS Dev) @ 2006-11-06 20:32 UTC (permalink / raw)
To: akpm, jens.axboe; +Cc: linux-kernel, linux-scsi
Patch 12 of 12
This patch replaces complete_buffers with end_that_request_first to fix
programs like iostat. This has been broken for the last few kernel releases.
Please consider this for inclusion.
Thanks,
mikem
Signed-off-by: Mike Miller <mike.miller@hp.com>
---
---
drivers/block/cciss.c | 20 ++++----------------
1 files changed, 4 insertions(+), 16 deletions(-)
diff -puN drivers/block/cciss.c~cciss_update_diskstats_fix drivers/block/cciss.c
--- linux-2.6/drivers/block/cciss.c~cciss_update_diskstats_fix 2006-11-06 13:28:53.000000000 -0600
+++ linux-2.6-root/drivers/block/cciss.c 2006-11-06 13:28:53.000000000 -0600
@@ -1156,18 +1156,6 @@ static int cciss_ioctl(struct inode *ino
}
}
-static inline void complete_buffers(struct bio *bio, int status)
-{
- while (bio) {
- struct bio *xbh = bio->bi_next;
- int nr_sectors = bio_sectors(bio);
-
- bio->bi_next = NULL;
- bio_endio(bio, nr_sectors << 9, status ? 0 : -EIO);
- bio = xbh;
- }
-}
-
static void cciss_check_queues(ctlr_info_t *h)
{
int start_queue = h->next_to_run;
@@ -1236,15 +1224,15 @@ static void cciss_softirq_done(struct re
pci_unmap_page(h->pdev, temp64.val, cmd->SG[i].Len, ddir);
}
- complete_buffers(rq->bio, rq->errors);
-
#ifdef CCISS_DEBUG
printk("Done with %p\n", rq);
#endif /* CCISS_DEBUG */
- add_disk_randomness(rq->rq_disk);
spin_lock_irqsave(&h->lock, flags);
- end_that_request_last(rq, rq->errors);
+ if (!end_that_request_first(rq, rq->errors, rq->nr_sectors)) {
+ add_disk_randomness(rq->rq_disk);
+ end_that_request_last(rq, rq->errors);
+ }
cmd_free(h, cmd, 1);
cciss_check_queues(h);
spin_unlock_irqrestore(&h->lock, flags);
_
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 12/12] cciss: fix for iostat
2006-11-06 20:32 [PATCH 12/12] cciss: fix for iostat Mike Miller (OS Dev)
@ 2006-11-06 20:45 ` Jens Axboe
2006-11-06 21:05 ` Jens Axboe
2006-11-07 19:24 ` Mike Miller (OS Dev)
0 siblings, 2 replies; 4+ messages in thread
From: Jens Axboe @ 2006-11-06 20:45 UTC (permalink / raw)
To: Mike Miller (OS Dev); +Cc: akpm, linux-kernel, linux-scsi
On Mon, Nov 06 2006, Mike Miller (OS Dev) wrote:
> Patch 12 of 12
>
> This patch replaces complete_buffers with end_that_request_first to fix
> programs like iostat. This has been broken for the last few kernel releases.
> Please consider this for inclusion.
>
> Thanks,
> mikem
>
> Signed-off-by: Mike Miller <mike.miller@hp.com>
>
>
> ---
>
>
> ---
>
> drivers/block/cciss.c | 20 ++++----------------
> 1 files changed, 4 insertions(+), 16 deletions(-)
>
> diff -puN drivers/block/cciss.c~cciss_update_diskstats_fix drivers/block/cciss.c
> --- linux-2.6/drivers/block/cciss.c~cciss_update_diskstats_fix 2006-11-06 13:28:53.000000000 -0600
> +++ linux-2.6-root/drivers/block/cciss.c 2006-11-06 13:28:53.000000000 -0600
> @@ -1156,18 +1156,6 @@ static int cciss_ioctl(struct inode *ino
> }
> }
>
> -static inline void complete_buffers(struct bio *bio, int status)
> -{
> - while (bio) {
> - struct bio *xbh = bio->bi_next;
> - int nr_sectors = bio_sectors(bio);
> -
> - bio->bi_next = NULL;
> - bio_endio(bio, nr_sectors << 9, status ? 0 : -EIO);
> - bio = xbh;
> - }
> -}
> -
> static void cciss_check_queues(ctlr_info_t *h)
> {
> int start_queue = h->next_to_run;
> @@ -1236,15 +1224,15 @@ static void cciss_softirq_done(struct re
> pci_unmap_page(h->pdev, temp64.val, cmd->SG[i].Len, ddir);
> }
>
> - complete_buffers(rq->bio, rq->errors);
> -
> #ifdef CCISS_DEBUG
> printk("Done with %p\n", rq);
> #endif /* CCISS_DEBUG */
>
> - add_disk_randomness(rq->rq_disk);
> spin_lock_irqsave(&h->lock, flags);
> - end_that_request_last(rq, rq->errors);
> + if (!end_that_request_first(rq, rq->errors, rq->nr_sectors)) {
> + add_disk_randomness(rq->rq_disk);
> + end_that_request_last(rq, rq->errors);
> + }
> cmd_free(h, cmd, 1);
> cciss_check_queues(h);
> spin_unlock_irqrestore(&h->lock, flags);
Ah, so there's where that went. Your code isn't clear, though -
end_that_request_first() _must_ return 0, so the above looks confusing.
It would look cleaner and more informative like:
if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
BUG();
add_disk_randomness(rq->rq_disk);
end_that_request_last(rq, rq->errors);
...
and so on. Additionally you don't need the lock for
end_that_request_first(), so it's a lot more optimal to rearrange it
again.
add_disk_randomness(rq->rq_disk);
if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
BUG();
spin_lock_irqsave(&h->lock, flags);
end_that_request_last(rq, rq->errors);
cmd_free(h, cmd, 1);
...
Not only cleaner to read since it's obvious what will happen, also moves
the heavy path (end_that_request_first()) outside of the controller
lock.
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 12/12] cciss: fix for iostat
2006-11-06 20:45 ` Jens Axboe
@ 2006-11-06 21:05 ` Jens Axboe
2006-11-07 19:24 ` Mike Miller (OS Dev)
1 sibling, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2006-11-06 21:05 UTC (permalink / raw)
To: Mike Miller (OS Dev); +Cc: akpm, linux-kernel, linux-scsi
On Mon, Nov 06 2006, Jens Axboe wrote:
> On Mon, Nov 06 2006, Mike Miller (OS Dev) wrote:
> > Patch 12 of 12
> >
> > This patch replaces complete_buffers with end_that_request_first to fix
> > programs like iostat. This has been broken for the last few kernel releases.
> > Please consider this for inclusion.
> >
> > Thanks,
> > mikem
> >
> > Signed-off-by: Mike Miller <mike.miller@hp.com>
> >
> >
> > ---
> >
> >
> > ---
> >
> > drivers/block/cciss.c | 20 ++++----------------
> > 1 files changed, 4 insertions(+), 16 deletions(-)
> >
> > diff -puN drivers/block/cciss.c~cciss_update_diskstats_fix drivers/block/cciss.c
> > --- linux-2.6/drivers/block/cciss.c~cciss_update_diskstats_fix 2006-11-06 13:28:53.000000000 -0600
> > +++ linux-2.6-root/drivers/block/cciss.c 2006-11-06 13:28:53.000000000 -0600
> > @@ -1156,18 +1156,6 @@ static int cciss_ioctl(struct inode *ino
> > }
> > }
> >
> > -static inline void complete_buffers(struct bio *bio, int status)
> > -{
> > - while (bio) {
> > - struct bio *xbh = bio->bi_next;
> > - int nr_sectors = bio_sectors(bio);
> > -
> > - bio->bi_next = NULL;
> > - bio_endio(bio, nr_sectors << 9, status ? 0 : -EIO);
> > - bio = xbh;
> > - }
> > -}
> > -
> > static void cciss_check_queues(ctlr_info_t *h)
> > {
> > int start_queue = h->next_to_run;
> > @@ -1236,15 +1224,15 @@ static void cciss_softirq_done(struct re
> > pci_unmap_page(h->pdev, temp64.val, cmd->SG[i].Len, ddir);
> > }
> >
> > - complete_buffers(rq->bio, rq->errors);
> > -
> > #ifdef CCISS_DEBUG
> > printk("Done with %p\n", rq);
> > #endif /* CCISS_DEBUG */
> >
> > - add_disk_randomness(rq->rq_disk);
> > spin_lock_irqsave(&h->lock, flags);
> > - end_that_request_last(rq, rq->errors);
> > + if (!end_that_request_first(rq, rq->errors, rq->nr_sectors)) {
> > + add_disk_randomness(rq->rq_disk);
> > + end_that_request_last(rq, rq->errors);
> > + }
> > cmd_free(h, cmd, 1);
> > cciss_check_queues(h);
> > spin_unlock_irqrestore(&h->lock, flags);
>
> Ah, so there's where that went. Your code isn't clear, though -
> end_that_request_first() _must_ return 0, so the above looks confusing.
> It would look cleaner and more informative like:
>
> if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
> BUG();
>
> add_disk_randomness(rq->rq_disk);
> end_that_request_last(rq, rq->errors);
> ...
>
> and so on. Additionally you don't need the lock for
> end_that_request_first(), so it's a lot more optimal to rearrange it
> again.
>
> add_disk_randomness(rq->rq_disk);
> if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
> BUG();
>
> spin_lock_irqsave(&h->lock, flags);
> end_that_request_last(rq, rq->errors);
> cmd_free(h, cmd, 1);
> ...
>
> Not only cleaner to read since it's obvious what will happen, also moves
> the heavy path (end_that_request_first()) outside of the controller
> lock.
IOW, simple one-liner fix:
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 6ffe2b2..f9f128a 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1299,6 +1299,7 @@ static void cciss_softirq_done(struct re
}
complete_buffers(rq->bio, rq->errors);
+ disk_stat_add(rq->rq_disk, sectors[rq_data_dir(rq)], rq->nr_sectors);
#ifdef CCISS_DEBUG
printk("Done with %p\n", rq);
--
Jens Axboe
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 12/12] cciss: fix for iostat
2006-11-06 20:45 ` Jens Axboe
2006-11-06 21:05 ` Jens Axboe
@ 2006-11-07 19:24 ` Mike Miller (OS Dev)
1 sibling, 0 replies; 4+ messages in thread
From: Mike Miller (OS Dev) @ 2006-11-07 19:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: akpm, linux-kernel, linux-scsi
On Mon, Nov 06, 2006 at 09:45:51PM +0100, Jens Axboe wrote:
> On Mon, Nov 06 2006, Mike Miller (OS Dev) wrote:
> > Patch 12 of 12
> >
> > This patch replaces complete_buffers with end_that_request_first to fix
> > programs like iostat. This has been broken for the last few kernel releases.
> > Please consider this for inclusion.
> >
> > Thanks,
> > mikem
> >
> > Signed-off-by: Mike Miller <mike.miller@hp.com>
> >
> >
> > ---
> >
> >
> > ---
> >
> > drivers/block/cciss.c | 20 ++++----------------
> > 1 files changed, 4 insertions(+), 16 deletions(-)
> >
> > diff -puN drivers/block/cciss.c~cciss_update_diskstats_fix drivers/block/cciss.c
> > --- linux-2.6/drivers/block/cciss.c~cciss_update_diskstats_fix 2006-11-06 13:28:53.000000000 -0600
> > +++ linux-2.6-root/drivers/block/cciss.c 2006-11-06 13:28:53.000000000 -0600
> > @@ -1156,18 +1156,6 @@ static int cciss_ioctl(struct inode *ino
> > }
> > }
> >
> > -static inline void complete_buffers(struct bio *bio, int status)
> > -{
> > - while (bio) {
> > - struct bio *xbh = bio->bi_next;
> > - int nr_sectors = bio_sectors(bio);
> > -
> > - bio->bi_next = NULL;
> > - bio_endio(bio, nr_sectors << 9, status ? 0 : -EIO);
> > - bio = xbh;
> > - }
> > -}
> > -
> > static void cciss_check_queues(ctlr_info_t *h)
> > {
> > int start_queue = h->next_to_run;
> > @@ -1236,15 +1224,15 @@ static void cciss_softirq_done(struct re
> > pci_unmap_page(h->pdev, temp64.val, cmd->SG[i].Len, ddir);
> > }
> >
> > - complete_buffers(rq->bio, rq->errors);
> > -
> > #ifdef CCISS_DEBUG
> > printk("Done with %p\n", rq);
> > #endif /* CCISS_DEBUG */
> >
> > - add_disk_randomness(rq->rq_disk);
> > spin_lock_irqsave(&h->lock, flags);
> > - end_that_request_last(rq, rq->errors);
> > + if (!end_that_request_first(rq, rq->errors, rq->nr_sectors)) {
> > + add_disk_randomness(rq->rq_disk);
> > + end_that_request_last(rq, rq->errors);
> > + }
> > cmd_free(h, cmd, 1);
> > cciss_check_queues(h);
> > spin_unlock_irqrestore(&h->lock, flags);
>
> Ah, so there's where that went. Your code isn't clear, though -
> end_that_request_first() _must_ return 0, so the above looks confusing.
> It would look cleaner and more informative like:
>
> if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
> BUG();
>
> add_disk_randomness(rq->rq_disk);
> end_that_request_last(rq, rq->errors);
> ...
>
> and so on. Additionally you don't need the lock for
> end_that_request_first(), so it's a lot more optimal to rearrange it
> again.
>
> add_disk_randomness(rq->rq_disk);
> if (end_that_request_first(rq, rq->errors, rq->nr_sectors))
> BUG();
>
> spin_lock_irqsave(&h->lock, flags);
> end_that_request_last(rq, rq->errors);
> cmd_free(h, cmd, 1);
> ...
>
> Not only cleaner to read since it's obvious what will happen, also moves
> the heavy path (end_that_request_first()) outside of the controller
> lock.
>
> --
> Jens Axboe
>
Thanks Jens. I'm fighting several fires right now so the cleanup will
be done in a day or 2.
mikem
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-11-07 19:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-11-06 20:32 [PATCH 12/12] cciss: fix for iostat Mike Miller (OS Dev)
2006-11-06 20:45 ` Jens Axboe
2006-11-06 21:05 ` Jens Axboe
2006-11-07 19:24 ` Mike Miller (OS Dev)
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.