Re: [PATCH 5/5] CFQ: use proper locking for cache of last hit cic

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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