lkml.org 
[lkml]   [2011]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/5] CFQ: use proper locking for cache of last hit cic
    On 2011-06-05 18:26, Paul Bolle wrote:
    > io_context.last_cic is a (single entry) cache of the last hit
    > cfq_io_context ("cic").
    >
    > It turns out last_cic wasn't always accessed with io_context.lock held
    > and under the correct RCU semantics. That meant that last_cic could be
    > out of sync with the hlist it was supposed to cache, leading to hard to
    > reproduce and hard to debug issues. Using proper locking makes those
    > issues go away.
    >
    > Many thanks to Vivek Goyal, Paul McKenney, and Jens Axboe, in suggesting
    > various options, looking at all the debug output I generated, etc. If we
    > hadn't done all that I would have never concluded that the best way to
    > solve this issue was to, yet again, read the code looking for
    > problematic sections.
    >
    > This should finally resolve bugzilla.redhat.com/show_bug.cgi?id=577968

    A few comments inline.

    > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
    > ---
    > block/cfq-iosched.c | 27 +++++++++++++++++++--------
    > 1 files changed, 19 insertions(+), 8 deletions(-)
    >
    > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
    > index 39e4d01..9206ee3 100644
    > --- a/block/cfq-iosched.c
    > +++ b/block/cfq-iosched.c
    > @@ -2695,6 +2695,8 @@ static void __cfq_exit_single_io_context(struct cfq_data *cfqd,
    > struct cfq_io_context *cic)
    > {
    > struct io_context *ioc = cic->ioc;
    > + struct cfq_io_context *last_cic;
    > + unsigned long flags;
    >
    > list_del_init(&cic->queue_list);
    >
    > @@ -2704,8 +2706,13 @@ static void __cfq_exit_single_io_context(struct cfq_data *cfqd,
    > smp_wmb();
    > cic->key = cfqd_dead_key(cfqd);
    >
    > - if (ioc->last_cic == cic)
    > + spin_lock_irqsave(&ioc->lock, flags);
    > + rcu_read_lock();
    > + last_cic = rcu_dereference(ioc->last_cic);
    > + rcu_read_unlock();
    > + if (last_cic == cic)
    > rcu_assign_pointer(ioc->last_cic, NULL);
    > + spin_unlock_irqrestore(&ioc->lock, flags);

    We don't need the ioc->lock for checking the cache, it would in fact
    defeat the purpose of using RCU. But this hunk will clash with the
    merged part anyway.

    > @@ -3000,23 +3007,25 @@ cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct io_context *ioc,
    >
    > /*
    > * We drop cfq io contexts lazily, so we may find a dead one.
    > + *
    > + * Called with ioc->lock held.
    > */
    > static void
    > cfq_drop_dead_cic(struct cfq_data *cfqd, struct io_context *ioc,
    > struct cfq_io_context *cic)
    > {
    > - unsigned long flags;
    > + struct cfq_io_context *last_cic;
    >
    > WARN_ON(!list_empty(&cic->queue_list));
    > BUG_ON(cic->key != cfqd_dead_key(cfqd));
    >
    > - spin_lock_irqsave(&ioc->lock, flags);
    > -
    > - BUG_ON(ioc->last_cic == cic);
    > + rcu_read_lock();
    > + last_cic = rcu_dereference(ioc->last_cic);
    > + rcu_read_unlock();
    > + BUG_ON(last_cic == cic);
    >
    > radix_tree_delete(&ioc->radix_root, cfqd->cic_index);
    > hlist_del_rcu(&cic->cic_node);
    > - spin_unlock_irqrestore(&ioc->lock, flags);
    >
    > cfq_cic_free(cic);

    See Pauls comment on this part.

    --
    Jens Axboe



    \
     
     \ /
      Last update: 2011-06-06 05:09    [W:0.023 / U:36.452 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site