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

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

[Paul Bolle]

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

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

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

[Paul E. McKenney]

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

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

[Jens Axboe]

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

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

[Paul Bolle]

On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> On 2011-06-05 18:26, Paul Bolle wrote:
> > @@ -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.

Just to show that I'm RCU-challenged, is that because:
1) my use of locking on ioc->lock defends for a race that is not
actually possible; or
2) the worst thing that could happen is that some new and correct value
of ioc->last_cic will be replaced with NULL, which is simply not a big
deal?

> But this hunk will clash with the
> merged part anyway.

Looking at Paul's feedback I do have a feeling that in your commit
(9b50902db5eb8a220160fb89e95aa11967998d12, "cfq-iosched: fix locking
around ioc->ioc_data assignment") the line:
	if (rcu_dereference(ioc->ioc_data) == cic) {

could actually be replaced with:
	if (rcu_access_pointer(ioc->ioc_data) == cic) {

Is that correct?

> [...]
> See Pauls comment on this part.

You seem to be offline right now. When you're back online, could you
please say whether or not you accept the two renaming patches that you
have rejected a few days ago (and for which I gave some follow up
arguments)? After that I'll send an update to this patch (and its commit
message) to reflect Paul's review and your review.


Paul Bolle

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

[Paul E. McKenney]

On Wed, Jun 08, 2011 at 08:18:44PM +0200, Paul Bolle wrote:
> On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> > On 2011-06-05 18:26, Paul Bolle wrote:
> > > @@ -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.
> 
> Just to show that I'm RCU-challenged, is that because:
> 1) my use of locking on ioc->lock defends for a race that is not
> actually possible; or
> 2) the worst thing that could happen is that some new and correct value
> of ioc->last_cic will be replaced with NULL, which is simply not a big
> deal?

My likely incorrect guess is that acquiring the lock excludes any
updates, so that there is no point in the RCU read-side critical
section.  But I don't claim to understand this code.

> > But this hunk will clash with the
> > merged part anyway.
> 
> Looking at Paul's feedback I do have a feeling that in your commit
> (9b50902db5eb8a220160fb89e95aa11967998d12, "cfq-iosched: fix locking
> around ioc->ioc_data assignment") the line:
> 	if (rcu_dereference(ioc->ioc_data) == cic) {
> 
> could actually be replaced with:
> 	if (rcu_access_pointer(ioc->ioc_data) == cic) {
> 
> Is that correct?

If you are not actually dereferencing the pointer, then yes, you
can use rcu_access_pointer() instead of rcu_dereference().  In
this case, the pointer is being compared against, not dereferenced,
so rcu_access_pointer() should do it.

							Thanx, Paul

> > [...]
> > See Pauls comment on this part.
> 
> You seem to be offline right now. When you're back online, could you
> please say whether or not you accept the two renaming patches that you
> have rejected a few days ago (and for which I gave some follow up
> arguments)? After that I'll send an update to this patch (and its commit
> message) to reflect Paul's review and your review.
> 
> 
> Paul Bolle
> 

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

[Vivek Goyal]

On Wed, Jun 08, 2011 at 08:18:44PM +0200, Paul Bolle wrote:
> On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> > On 2011-06-05 18:26, Paul Bolle wrote:
> > > @@ -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.
> 
> Just to show that I'm RCU-challenged, is that because:
> 1) my use of locking on ioc->lock defends for a race that is not
> actually possible; or
> 2) the worst thing that could happen is that some new and correct value
> of ioc->last_cic will be replaced with NULL, which is simply not a big
> deal?

I don't understand this point. All ioc->ioc_data updates are under
ioc->lock except the one __cfq_exit_single_io_context() and that's what
jens patch fixed. So clearly there was atleast one race where we were
doing a value update without taking appropriate lock. 

Why do you think that some new and correct value will be replaced
by NULL?

Thanks
Vivek

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

[Paul Bolle]

On Wed, 2011-06-08 at 14:42 -0400, Vivek Goyal wrote:
> On Wed, Jun 08, 2011 at 08:18:44PM +0200, Paul Bolle wrote:
> > On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> > > On 2011-06-05 18:26, Paul Bolle wrote:
> > > > @@ -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.
> > 
> > Just to show that I'm RCU-challenged, is that because:
> > 1) my use of locking on ioc->lock defends for a race that is not
> > actually possible; or
> > 2) the worst thing that could happen is that some new and correct value
> > of ioc->last_cic will be replaced with NULL, which is simply not a big
> > deal?
> 
> I don't understand this point.

That could be because I don't really get all the RCU voodoo, nor how
this all interacts with the io_contect->lock here, so I probably asked
an impossible question.

> All ioc->ioc_data updates are under
> ioc->lock except the one __cfq_exit_single_io_context() and that's what
> jens patch fixed. So clearly there was atleast one race where we were
> doing a value update without taking appropriate lock. 
> 
> Why do you think that some new and correct value will be replaced
> by NULL?

Jens updated the code to
	if (rcu_dereference(ioc->ioc_data) == cic) {
		spin_lock(&ioc->lock);
		rcu_assign_pointer(ioc->ioc_data, NULL);
		spin_unlock(&ioc->lock);
	}

I basically suggested (except for an apparently useless
rcu_read_lock() / rcu_read_unlock() pair and using spin_lock_irqsave()
etc.):
	spin_lock(&ioc->lock);
	if (rcu_dereference(ioc->ioc_data) == cic)
		rcu_assign_pointer(ioc->ioc_data, NULL);
	spin_unlock(&ioc->lock);

Ie, I thought that reads of and updates to ioc->ioc_data should be done
with ioc->lock held. My reasoning was that in Jens code ioc->ioc_data
might already be updated (by another thread, or whatever) and thus not
be equal to cic by the time it's updated to NULL. See, in my
understanding ioc->ioc_data could be equal to cic when it's
rcu_dereference()'d, but unequal to cic by the time it's
rcu_assign_pointer()'d to NULL.


Paul Bolle

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

[Vivek Goyal]

On Wed, Jun 08, 2011 at 09:32:52PM +0200, Paul Bolle wrote:
> On Wed, 2011-06-08 at 14:42 -0400, Vivek Goyal wrote:
> > On Wed, Jun 08, 2011 at 08:18:44PM +0200, Paul Bolle wrote:
> > > On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> > > > On 2011-06-05 18:26, Paul Bolle wrote:
> > > > > @@ -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.
> > > 
> > > Just to show that I'm RCU-challenged, is that because:
> > > 1) my use of locking on ioc->lock defends for a race that is not
> > > actually possible; or
> > > 2) the worst thing that could happen is that some new and correct value
> > > of ioc->last_cic will be replaced with NULL, which is simply not a big
> > > deal?
> > 
> > I don't understand this point.
> 
> That could be because I don't really get all the RCU voodoo, nor how
> this all interacts with the io_contect->lock here, so I probably asked
> an impossible question.
> 
> > All ioc->ioc_data updates are under
> > ioc->lock except the one __cfq_exit_single_io_context() and that's what
> > jens patch fixed. So clearly there was atleast one race where we were
> > doing a value update without taking appropriate lock. 
> > 
> > Why do you think that some new and correct value will be replaced
> > by NULL?
> 
> Jens updated the code to
> 	if (rcu_dereference(ioc->ioc_data) == cic) {
> 		spin_lock(&ioc->lock);
> 		rcu_assign_pointer(ioc->ioc_data, NULL);
> 		spin_unlock(&ioc->lock);
> 	}
> 
> I basically suggested (except for an apparently useless
> rcu_read_lock() / rcu_read_unlock() pair and using spin_lock_irqsave()
> etc.):
> 	spin_lock(&ioc->lock);
> 	if (rcu_dereference(ioc->ioc_data) == cic)
> 		rcu_assign_pointer(ioc->ioc_data, NULL);
> 	spin_unlock(&ioc->lock);
> 
> Ie, I thought that reads of and updates to ioc->ioc_data should be done
> with ioc->lock held. My reasoning was that in Jens code ioc->ioc_data
> might already be updated (by another thread, or whatever) and thus not
> be equal to cic by the time it's updated to NULL. See, in my
> understanding ioc->ioc_data could be equal to cic when it's
> rcu_dereference()'d, but unequal to cic by the time it's
> rcu_assign_pointer()'d to NULL.

Ok, I see it now. Yep theoritically this race is present. Jens must have
checked the value without taking lock so that if we cic is not the cached
one, we don't take ioc lock.

I would not worry too much about it as the side effect of this race is
only trashing the cache which will soon be filled again upon next
request.

Thanks
Vivek