lkml.org 
[lkml]   [2011]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 5/5] CFQ: use proper locking for cache of last hit cic
    On Sun, Jun 05, 2011 at 06:26:40PM +0200, 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

    Good stuff! A few minor comments below.

    Thanx, Paul

    > 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);

    Because we are holding ioc->lock, no one else is permitted to change
    the value of ioc->last_cic, correct?

    If so, I suggest the following replacement for the above code,
    starting at the rcu_read_lock():

    last_cic = rcu_dereference_protected(ioc->last_cic
    lockdep_is_held(&ioc->lock));
    if (last_cic == cic)
    rcu_assign_pointer(ioc->last_cic, NULL);

    > + spin_unlock_irqrestore(&ioc->lock, flags);
    >
    > if (cic->cfqq[BLK_RW_ASYNC]) {
    > cfq_exit_cfqq(cfqd, cic->cfqq[BLK_RW_ASYNC]);
    > @@ -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);

    And the above insertion can be replaced with:

    BUG_ON(rcu_access_pointer(ioc->last_cic) == cic);

    Use of rcu_access_pointer() is OK here because you are just testing
    the value of the RCU-protected pointer, not actually dereferencing it.
    Also, because you are just testing the value, you don't need to hold
    the update-side lock.

    >
    > 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);
    > }
    > @@ -3035,8 +3044,10 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
    > /*
    > * we maintain a last-hit cache, to avoid browsing over the tree
    > */
    > + spin_lock_irqsave(&ioc->lock, flags);
    > cic = rcu_dereference(ioc->last_cic);

    Is the above rcu_dereference() is the only reason that we are in this
    RCU read-side critical section? If so, you can drop the RCU read-side
    critical section and use rcu_dereference_protected(), as noted earlier.

    > if (cic && cic->key == cfqd) {
    > + spin_unlock_irqrestore(&ioc->lock, flags);
    > rcu_read_unlock();
    > return cic;
    > }
    > @@ -3052,12 +3063,12 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
    > continue;
    > }
    >
    > - spin_lock_irqsave(&ioc->lock, flags);
    > rcu_assign_pointer(ioc->last_cic, cic);
    > - spin_unlock_irqrestore(&ioc->lock, flags);
    > break;
    > } while (1);
    >
    > + spin_unlock_irqrestore(&ioc->lock, flags);
    > +
    > return cic;
    > }
    >
    > --
    > 1.7.5.2
    >
    >
    >


    \
     
     \ /
      Last update: 2011-06-05 19:01    [W:0.034 / U:0.340 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site