Messages in this thread | | | From | Nikanth Karthikesan <> | Subject | Re: [PATCH] [RFC] make hd_struct->in_flight atomic to avoid diskstat corruption | Date | Thu, 16 Apr 2009 14:45:03 +0530 |
| |
On Thursday 16 April 2009 13:05:57 Jens Axboe wrote: > On Thu, Apr 16 2009, Nikanth Karthikesan wrote: > > The disk statistics exported to userspace through proc and sysfs are > > not protected by locks to avoid performance overhead. Since most of > > the statistics are maintained in the per_cpu struct disk_stats, the > > chances of them getting corrupted is negligible. But the in_flight > > counter, that records the no of requests currently in progress is not > > per-cpu. This increases the chance of it getting corrupted. And > > corruption of this value would result in visibly distorted statistics > > such as negative in_flight. This can be avoided by making this field > > atomic. > > Hmm. Did you observe this behaviour?
Sorry, not on current kernels. But on a very old 2.6.5 kernel.
Reading Documentation/iostats.txt and the changelog of commit e71bf0d0ee89e51b92776391c5634938236977d5 made me assume that this could be a problem even today.
> A quick glance at the code reveals > that the callers of part_inc_in_flight() and part_dec_in_flight() in the > block layer are always done under the queue lock. Ditto > part_round_stats(), which calls part_round_stats_single() and also needs > protection for in_flight. > > That basically just leaves the code reading this out and reporting, and > driver calls to part_round_stats(). I'd suggest looking there instead, > we're not going to make ->in_flight an atomic just because of some > silliness there that could be fixed.
Isn't this also true for the stats protected by the part_stat_lock()? Only places where we are only reading seems to be called without the queue lock.
Thanks Nikanth
| |