[lkml]   [2009]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] [RFC] make hd_struct->in_flight atomic to avoid diskstat corruption
    On Thu, Apr 16 2009, Tejun Heo wrote:
    > Hello, Nikanth, Jens.
    > Nikanth Karthikesan wrote:
    > >> 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.
    > The only problem we can run into there is if a request doesn't get
    > attributed to a partition on issue but gets attributed to a partition
    > on completion, which seems to be possible if a new partition is added
    > while IO on the whole device which fell into the new partition area is
    > already in progress, which, on the first glance, seems possible if the
    > admin tries really hard. I think we can get around the problem by
    > doing part->in_flight = min(max(new_val, part0->in_flight), 0) in
    > dec_in_flight(). This is pretty extreme corner case tho.

    Heh, that is pretty extreme. I'd prefer just quiescing the queue,
    perhaps we should do that for partition map swaps.

    > >> 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.
    > part_stat_lock() doesn't protect against simultaneous access. I don't
    > think we have any place where in_flight is updated without queuelock
    > and the counters being equal to or smaller then ulong, reading
    > shouldn't be a problem.
    > I don't think the bug you saw in 2.6.5 kernel applies to upstream
    > kernel. The minus in_flight value was seen on the diskstats of the
    > whole device which can't be affected by partition coming up while IOs
    > are in progress.

    Plus at least early versions of the SLES9 kernel had a missing lock
    around io stat updates for SCSI. But that has been plugged for a long
    time, so probably unrelated to this case as well.

    Jens Axboe

     \ /
      Last update: 2009-04-16 18:35    [W:0.021 / U:2.556 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site