[PATCH] ide-io.c, kernel 2.4.22 Fix for IO stats in /proc/partitions, was Re: sard/iostat disk I/O statistics/accounting for 2.5.8-pre3

[PATCH] ide-io.c, kernel 2.4.22 Fix for IO stats in /proc/partitions, was Re: sard/iostat disk I/O statistics/accounting for 2.5.8-pre3

[Chad Talbott]

I found the cause of ide disks' ios_in_flight going negative in
/proc/partitions.

It's due to unbalanced calls to up_ios and down_ios.  After an
explicit drive command, ide-io.c's ide_end_drive_cmd calls
end_that_request_last which eventually calls down_ios to decrement
ios_in_flight, however there is no corresponding call to up_ios when
the command is initiated.

My guess is that ios_in_flight goes negative when the drive is idle
because many people run hdparm in an init script, and this decrements
ios_in_flight early on.  It stays off center from there.

The following hack to ide_end_drive_cmd is a workaround, I would
rather call up_ios appropriately, so that explicit ide commands are
properly accounted.  However I'm having a hard time identifying all
the places that initiate a low-level drive command.  I'll look into a
proper fix, but someone else probably knows the ide layer better than
me.

Chad

--- linux-2.4.18-old/drivers/ide/ide-io.c	15 Sep 2003 17:41:32 -0000
+++ linux-2.4.18-new/drivers/ide/ide-io.c	15 Sep 2003 20:11:12 -0000
@@ -148,6 +148,7 @@
 	ide_hwif_t *hwif = HWIF(drive);
 	unsigned long flags;
 	struct request *rq;
+	struct completion *waiting;
 
 	spin_lock_irqsave(&io_request_lock, flags);
 	rq = HWGROUP(drive)->rq;
@@ -221,7 +222,13 @@
 	spin_lock_irqsave(&io_request_lock, flags);
 	blkdev_dequeue_request(rq);
 	HWGROUP(drive)->rq = NULL;
-	end_that_request_last(rq);
+
+	waiting = req->waiting;
+	req_finished_io(req);
+	blkdev_release_request(req);
+	if (waiting)
+		complete(waiting);
+
 	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 

Re: [PATCH] ide-io.c, kernel 2.4.22 Fix for IO stats in /proc/partitions, was Re: sard/iostat disk I/O statistics/accounting for 2.5.8-pre3

[J.A. Magallon]

On 09.15, Chad Talbott wrote:
> I found the cause of ide disks' ios_in_flight going negative in
> /proc/partitions.
[...]
> 
> --- linux-2.4.18-old/drivers/ide/ide-io.c	15 Sep 2003 17:41:32 -0000
> +++ linux-2.4.18-new/drivers/ide/ide-io.c	15 Sep 2003 20:11:12 -0000
> @@ -148,6 +148,7 @@
>  	ide_hwif_t *hwif = HWIF(drive);
>  	unsigned long flags;
>  	struct request *rq;
> +	struct completion *waiting;
>  
>  	spin_lock_irqsave(&io_request_lock, flags);
>  	rq = HWGROUP(drive)->rq;
> @@ -221,7 +222,13 @@
>  	spin_lock_irqsave(&io_request_lock, flags);
>  	blkdev_dequeue_request(rq);
>  	HWGROUP(drive)->rq = NULL;
> -	end_that_request_last(rq);
> +
> +	waiting = req->waiting;
> +	req_finished_io(req);
> +	blkdev_release_request(req);
> +	if (waiting)
> +		complete(waiting);
> +
>  	spin_unlock_irqrestore(&io_request_lock, flags);
>  }
>  

Did you ever built this ? req -> rq ?

-- 
J.A. Magallon <jamagallon@able.es>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.2 (Cooker) for i586
Linux 2.4.23-pre4-jam1 (gcc 3.3.1 (Mandrake Linux 9.2 3.3.1-2mdk))

Re: [PATCH] ide-io.c, kernel 2.4.22 Fix for IO stats in /proc/partitions, was Re: sard/iostat disk I/O statistics/accounting for 2.5.8-pre3

[Chad Talbott]

"J.A. Magallon" <jamagallon@able.es> writes:

> On 09.15, Chad Talbott wrote:
> > I found the cause of ide disks' ios_in_flight going negative in
> > /proc/partitions.
> [...]

[broken patch elided...]

> Did you ever built this ? req -> rq ?

Good catch.  Not that patch, obviously.  :)  I don't know where I came
up with that one; even with the req -> rq change it includes the call
to req_finished_io, so it's exactly a no-op.

Here's the one that I compiled and tested. It makes /proc/partitions
report correctly.

Thanks,
Chad

--- linux-2.4.22-old/drivers/ide/ide-io.c	15 Sep 2003 17:41:32 -0000
+++ linux-2.4.22-new/drivers/ide/ide-io.c	23 Sep 2003 17:40:27 -0000
@@ -148,6 +148,7 @@
 	ide_hwif_t *hwif = HWIF(drive);
 	unsigned long flags;
 	struct request *rq;
+	struct completion *waiting;
 
 	spin_lock_irqsave(&io_request_lock, flags);
 	rq = HWGROUP(drive)->rq;
@@ -221,7 +222,12 @@
 	spin_lock_irqsave(&io_request_lock, flags);
 	blkdev_dequeue_request(rq);
 	HWGROUP(drive)->rq = NULL;
-	end_that_request_last(rq);
+
+	waiting = rq->waiting;
+	blkdev_release_request(rq);
+	if (waiting)
+		complete(waiting);
+
 	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 

Re: [PATCH] ide-io.c, kernel 2.4.22 Fix for IO stats in /proc/partitions, was Re: sard/iostat disk I/O statistics/accounting for 2.5.8-pre3

[Stephen C. Tweedie]

Hi,

On Wed, 2003-09-24, Chad Talbott wrote:


> Here's the one that I compiled and tested. It makes /proc/partitions
> report correctly.

Looks OK, but I'd defer to an IDE expert on the question of whether this
is the best fix or not.

Basically, you need to pair accounting start points with accounting end
points.  This patch removes the end point from the IDE special-command
postprocessing, but only by manually replacing end_that_request_last()
with a variant which doesn't do accounting.

I'd really rather see a patch which accounted for the start of the
special command in the first place (via "req_new_io()" when the special
request is first queued), as I suspect that might be a more robust way
of doing things.  But as I said, it would take somebody more familiar
with the IDE code to know whether that's really going to be better or
not.

Cheers,
 Stephen