[patch] aio: fix buggy put_ioctx call in aio_complete

[patch] aio: fix buggy put_ioctx call in aio_complete

[Chen, Kenneth W]

Regarding to a bug report on:
http://marc.theaimsgroup.com/?l=linux-kernel&m=116599593200888&w=2

flush_workqueue() is not allowed to be called in the softirq context.
However, aio_complete() called from I/O interrupt can potentially call
put_ioctx with last ref count on ioctx and trigger a bug warning.  It
is simply incorrect to perform ioctx freeing from aio_complete.

This patch removes all duplicate ref counting for each kiocb as
reqs_active already used as a request ref count for each active ioctx.
This also ensures that buggy call to flush_workqueue() in softirq
context is eliminated. wait_for_all_aios currently will wait on last
active kiocb.  However, it is racy.  This patch also tighten it up
by utilizing rcu synchronization mechanism to ensure no further
reference to ioctx before put_ioctx function is run.


Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>


--- ./fs/aio.c.orig	2006-12-19 08:35:01.000000000 -0800
+++ ./fs/aio.c	2006-12-19 08:46:34.000000000 -0800
@@ -308,6 +308,7 @@ static void wait_for_all_aios(struct kio
 		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
 	}
 	__set_task_state(tsk, TASK_RUNNING);
+	synchronize_rcu();
 	remove_wait_queue(&ctx->wait, &wait);
 }
 
@@ -425,7 +426,6 @@ static struct kiocb fastcall *__aio_get_
 	ring = kmap_atomic(ctx->ring_info.ring_pages[0], KM_USER0);
 	if (ctx->reqs_active < aio_ring_avail(&ctx->ring_info, ring)) {
 		list_add(&req->ki_list, &ctx->active_reqs);
-		get_ioctx(ctx);
 		ctx->reqs_active++;
 		okay = 1;
 	}
@@ -538,8 +538,6 @@ int fastcall aio_put_req(struct kiocb *r
 	spin_lock_irq(&ctx->ctx_lock);
 	ret = __aio_put_req(ctx, req);
 	spin_unlock_irq(&ctx->ctx_lock);
-	if (ret)
-		put_ioctx(ctx);
 	return ret;
 }
 
@@ -795,8 +793,7 @@ static int __aio_run_iocbs(struct kioctx
 		 */
 		iocb->ki_users++;       /* grab extra reference */
 		aio_run_iocb(iocb);
-		if (__aio_put_req(ctx, iocb))  /* drop extra ref */
-			put_ioctx(ctx);
+		__aio_put_req(ctx, iocb);
  	}
 	if (!list_empty(&ctx->run_list))
 		return 1;
@@ -1012,6 +1009,7 @@ int fastcall aio_complete(struct kiocb *
 		iocb->ki_nbytes - iocb->ki_left, iocb->ki_nbytes);
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
+	rcu_read_lock();
 	ret = __aio_put_req(ctx, iocb);
 
 	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
@@ -1019,9 +1017,7 @@ put_rq:
 	if (waitqueue_active(&ctx->wait))
 		wake_up(&ctx->wait);
 
-	if (ret)
-		put_ioctx(ctx);
-
+	rcu_read_unlock();
 	return ret;
 }
 

Re: [patch] aio: fix buggy put_ioctx call in aio_complete

[Andrew Morton]

On Tue, 19 Dec 2006 13:49:18 -0800
"Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:

> Regarding to a bug report on:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116599593200888&w=2
> 
> flush_workqueue() is not allowed to be called in the softirq context.
> However, aio_complete() called from I/O interrupt can potentially call
> put_ioctx with last ref count on ioctx and trigger a bug warning.  It
> is simply incorrect to perform ioctx freeing from aio_complete.
> 
> This patch removes all duplicate ref counting for each kiocb as
> reqs_active already used as a request ref count for each active ioctx.
> This also ensures that buggy call to flush_workqueue() in softirq
> context is eliminated. wait_for_all_aios currently will wait on last
> active kiocb.  However, it is racy.  This patch also tighten it up
> by utilizing rcu synchronization mechanism to ensure no further
> reference to ioctx before put_ioctx function is run.
> 

hrm, maybe.  Does this count as "abuse of the RCU interfaces".  Or "reuse"?

> 
> 
> --- ./fs/aio.c.orig	2006-12-19 08:35:01.000000000 -0800
> +++ ./fs/aio.c	2006-12-19 08:46:34.000000000 -0800
> @@ -308,6 +308,7 @@ static void wait_for_all_aios(struct kio
>  		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
>  	}
>  	__set_task_state(tsk, TASK_RUNNING);
> +	synchronize_rcu();
>  	remove_wait_queue(&ctx->wait, &wait);
>  }

argh.  Pity the poor fresh-faced wannabe kernel developer who stumbles
across a stray synchronize_rcu() in the AIO code and wonders what the hell
that's doing there.

Please, this needs good commenting.   More than zero, anyway.

> @@ -425,7 +426,6 @@ static struct kiocb fastcall *__aio_get_
>  	ring = kmap_atomic(ctx->ring_info.ring_pages[0], KM_USER0);
>  	if (ctx->reqs_active < aio_ring_avail(&ctx->ring_info, ring)) {
>  		list_add(&req->ki_list, &ctx->active_reqs);
> -		get_ioctx(ctx);
>  		ctx->reqs_active++;
>  		okay = 1;
>  	}
> @@ -538,8 +538,6 @@ int fastcall aio_put_req(struct kiocb *r
>  	spin_lock_irq(&ctx->ctx_lock);
>  	ret = __aio_put_req(ctx, req);
>  	spin_unlock_irq(&ctx->ctx_lock);
> -	if (ret)
> -		put_ioctx(ctx);
>  	return ret;
>  }
>  
> @@ -795,8 +793,7 @@ static int __aio_run_iocbs(struct kioctx
>  		 */
>  		iocb->ki_users++;       /* grab extra reference */
>  		aio_run_iocb(iocb);
> -		if (__aio_put_req(ctx, iocb))  /* drop extra ref */
> -			put_ioctx(ctx);
> +		__aio_put_req(ctx, iocb);
>   	}
>  	if (!list_empty(&ctx->run_list))
>  		return 1;
> @@ -1012,6 +1009,7 @@ int fastcall aio_complete(struct kiocb *
>  		iocb->ki_nbytes - iocb->ki_left, iocb->ki_nbytes);
>  put_rq:
>  	/* everything turned out well, dispose of the aiocb. */
> +	rcu_read_lock();
>  	ret = __aio_put_req(ctx, iocb);
>  
>  	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
> @@ -1019,9 +1017,7 @@ put_rq:
>  	if (waitqueue_active(&ctx->wait))
>  		wake_up(&ctx->wait);
>  
> -	if (ret)
> -		put_ioctx(ctx);
> -
> +	rcu_read_unlock();
>  	return ret;
>  }
>  

RE: [patch] aio: fix buggy put_ioctx call in aio_complete

[Chen, Kenneth W]

Andrew Morton wrote on Wednesday, December 20, 2006 8:06 PM
> On Tue, 19 Dec 2006 13:49:18 -0800
> "Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:
> > Regarding to a bug report on:
> > http://marc.theaimsgroup.com/?l=linux-kernel&m=116599593200888&w=2
> > 
> > flush_workqueue() is not allowed to be called in the softirq context.
> > However, aio_complete() called from I/O interrupt can potentially call
> > put_ioctx with last ref count on ioctx and trigger a bug warning.  It
> > is simply incorrect to perform ioctx freeing from aio_complete.
> > 
> > This patch removes all duplicate ref counting for each kiocb as
> > reqs_active already used as a request ref count for each active ioctx.
> > This also ensures that buggy call to flush_workqueue() in softirq
> > context is eliminated. wait_for_all_aios currently will wait on last
> > active kiocb.  However, it is racy.  This patch also tighten it up
> > by utilizing rcu synchronization mechanism to ensure no further
> > reference to ioctx before put_ioctx function is run.
> 
> hrm, maybe.  Does this count as "abuse of the RCU interfaces".  Or "reuse"?

Yeah, it's abuse.

Problem is in wait_for_all_aios(), it is checking wait status without
properly holding an ioctx lock. Perhaps, this patch is walking on thin
ice.  It abuses rcu over a buggy code. OTOH, I really don't want to hold
ctx_lock over the entire wakeup call at the end of aio_complete:

        if (waitqueue_active(&ctx->wait))
                wake_up(&ctx->wait);

I'm worried about longer lock hold time in aio_complete and potentially 
increase lock contention for concurrent I/O completion.  A quick look
at lockmeter data I had on a 4 socket system (with dual core + HT), it
already showing signs of substantial lock contention in aio_complete.
I'm afraid putting the above call inside ioctx_lock will make things
worse.

And synchronize_rcu fits the bill perfectly: aio_complete sets wakeup
status, drop ioctx_lock, do the wakeup call all protected inside rcu
lock.  Then wait_for_all_aios will just wait for all that sequence to
complete before it proceed with __put_ioctx().  All nice and easy.

Re: [patch] aio: fix buggy put_ioctx call in aio_complete

[Andrew Morton]

On Wed, 20 Dec 2006 23:58:43 -0800
"Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:

> Andrew Morton wrote on Wednesday, December 20, 2006 8:06 PM
> > On Tue, 19 Dec 2006 13:49:18 -0800
> > "Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:
> > > Regarding to a bug report on:
> > > http://marc.theaimsgroup.com/?l=linux-kernel&m=116599593200888&w=2
> > > 
> > > flush_workqueue() is not allowed to be called in the softirq context.
> > > However, aio_complete() called from I/O interrupt can potentially call
> > > put_ioctx with last ref count on ioctx and trigger a bug warning.  It
> > > is simply incorrect to perform ioctx freeing from aio_complete.
> > > 
> > > This patch removes all duplicate ref counting for each kiocb as
> > > reqs_active already used as a request ref count for each active ioctx.
> > > This also ensures that buggy call to flush_workqueue() in softirq
> > > context is eliminated. wait_for_all_aios currently will wait on last
> > > active kiocb.  However, it is racy.  This patch also tighten it up
> > > by utilizing rcu synchronization mechanism to ensure no further
> > > reference to ioctx before put_ioctx function is run.
> > 
> > hrm, maybe.  Does this count as "abuse of the RCU interfaces".  Or "reuse"?
> 
> Yeah, it's abuse.

Alas, your above description doesn't really tell us what the bug is, so I'm
at a bit of a loss here.

<finds http://marc.theaimsgroup.com/?l=linux-aio&m=116616463009218&w=2>

So that's a refcounting bug.  But it's really a locking bug, because
refcounting needs locking too.

> Problem is in wait_for_all_aios(), it is checking wait status without
> properly holding an ioctx lock. Perhaps, this patch is walking on thin
> ice.  It abuses rcu over a buggy code. OTOH, I really don't want to hold
> ctx_lock over the entire wakeup call at the end of aio_complete:
> 
>         if (waitqueue_active(&ctx->wait))
>                 wake_up(&ctx->wait);
> 
> I'm worried about longer lock hold time in aio_complete and potentially 
> increase lock contention for concurrent I/O completion.

There is a potential problem where we deliver a storm of wakeups at the
waitqueue, and until the waked-up process actually ges going and removes
itself from the waitqueue, those wake_up()s do lots of work waking up an
already-runnable task.

If we were using DEFINE_WAIT/prepare_to_wait/finish_wait in there then the
*first* wake_up() would do some work, but all the following ones are
practically free.

So if you're going to get in there and run the numbers on this, try both
approaches.

>  A quick look
> at lockmeter data I had on a 4 socket system (with dual core + HT), it
> already showing signs of substantial lock contention in aio_complete.
> I'm afraid putting the above call inside ioctx_lock will make things
> worse.

It beats oopsing.

> And synchronize_rcu fits the bill perfectly: aio_complete sets wakeup
> status, drop ioctx_lock, do the wakeup call all protected inside rcu
> lock.  Then wait_for_all_aios will just wait for all that sequence to
> complete before it proceed with __put_ioctx().  All nice and easy.

Possibly it would be less abusive to use preempt_disable()/enable (with
suitable comments) and synchronize_kernel().  To at least remove the rcu
signals from in there.

RE: [patch] aio: fix buggy put_ioctx call in aio_complete

[Chen, Kenneth W]

Andrew Morton wrote on Thursday, December 21, 2006 12:18 AM
> Alas, your above description doesn't really tell us what the bug is, so I'm
> at a bit of a loss here.
> 
> <finds http://marc.theaimsgroup.com/?l=linux-aio&m=116616463009218&w=2>
> 
> So that's a refcounting bug.  But it's really a locking bug, because
> refcounting needs locking too.

I should've quoted the original bug report (kicking myself for those
fat fingers!): http://marc.theaimsgroup.com/?l=linux-kernel&m=116599593200888&w=2

The bug manifested from an expectation that __put_ioctx can be called
in the softirq context, but it really shouldn't. Normally, aio_complete
will not decrement last ref count on ioctx, but under stressed system,
it might.


> > Problem is in wait_for_all_aios(), it is checking wait status without
> > properly holding an ioctx lock. Perhaps, this patch is walking on thin
> > ice.  It abuses rcu over a buggy code. OTOH, I really don't want to hold
> > ctx_lock over the entire wakeup call at the end of aio_complete:
> > 
> >         if (waitqueue_active(&ctx->wait))
> >                 wake_up(&ctx->wait);
> > 
> > I'm worried about longer lock hold time in aio_complete and potentially 
> > increase lock contention for concurrent I/O completion.
> 
> There is a potential problem where we deliver a storm of wakeups at the
> waitqueue, and until the waked-up process actually ges going and removes
> itself from the waitqueue, those wake_up()s do lots of work waking up an
> already-runnable task.
> 
> If we were using DEFINE_WAIT/prepare_to_wait/finish_wait in there then the
> *first* wake_up() would do some work, but all the following ones are
> practically free.
> 
> So if you're going to get in there and run the numbers on this, try both
> approaches.

Yes, I agree and I would like to bring your patch on "DEFINE_WAIT..." back
too.  I will run that experiment.


> > A quick look
> > at lockmeter data I had on a 4 socket system (with dual core + HT), it
> > already showing signs of substantial lock contention in aio_complete.
> > I'm afraid putting the above call inside ioctx_lock will make things
> > worse.
> 
> It beats oopsing.

Yeah, correctness absolutely over rule optimization.  I just hope I can
find a way to satisfy both.


> > And synchronize_rcu fits the bill perfectly: aio_complete sets wakeup
> > status, drop ioctx_lock, do the wakeup call all protected inside rcu
> > lock.  Then wait_for_all_aios will just wait for all that sequence to
> > complete before it proceed with __put_ioctx().  All nice and easy.
> 
> Possibly it would be less abusive to use preempt_disable()/enable (with
> suitable comments) and synchronize_kernel().  To at least remove the rcu
> signals from in there.

I think I'm going to abandon this whole synchronize thing and going to
put the wake up call inside ioctx_lock spin lock along with the other
patch you mentioned above in the waiter path.  On top of that, I have
another patch attempts to perform wake-up only when the waiter can truly
proceed in aio_read_evt so dribbling I/O completion doesn't inefficiently
waking up waiter too frequently and only to have waiter put back to sleep
again. I will dig that up and experiment.

Re: [patch] aio: fix buggy put_ioctx call in aio_complete

[jmoyer]

==> Regarding RE: [patch] aio: fix buggy put_ioctx call in aio_complete; "Chen, Kenneth W" <kenneth.w.chen@intel.com> adds:

kenneth.w.chen> I think I'm going to abandon this whole synchronize thing
kenneth.w.chen> and going to put the wake up call inside ioctx_lock spin
kenneth.w.chen> lock along with the other patch you mentioned above in the
kenneth.w.chen> waiter path.  On top of that, I have another patch attempts
kenneth.w.chen> to perform wake-up only when the waiter can truly proceed
kenneth.w.chen> in aio_read_evt so dribbling I/O completion doesn't
kenneth.w.chen> inefficiently waking up waiter too frequently and only to
kenneth.w.chen> have waiter put back to sleep again. I will dig that up and
kenneth.w.chen> experiment.

In the mean time, can't we simply take the context lock in
wait_for_all_aios?  Unless I missed something, I think that will address
the reference count problem.

Thanks,

Jeff

RE: [patch] aio: fix buggy put_ioctx call in aio_complete

[Chen, Kenneth W]

jmoyer@redhat.com wrote on Thursday, December 21, 2006 8:56 AM
> kenneth.w.chen> I think I'm going to abandon this whole synchronize thing
> kenneth.w.chen> and going to put the wake up call inside ioctx_lock spin
> kenneth.w.chen> lock along with the other patch you mentioned above in the
> kenneth.w.chen> waiter path.  On top of that, I have another patch attempts
> kenneth.w.chen> to perform wake-up only when the waiter can truly proceed
> kenneth.w.chen> in aio_read_evt so dribbling I/O completion doesn't
> kenneth.w.chen> inefficiently waking up waiter too frequently and only to
> kenneth.w.chen> have waiter put back to sleep again. I will dig that up and
> kenneth.w.chen> experiment.
> 
> In the mean time, can't we simply take the context lock in
> wait_for_all_aios?  Unless I missed something, I think that will address
> the reference count problem.

Take ioctx_lock is one part, the other part is to move

	spin_unlock_irqrestore(&ctx->ctx_lock, flags);

in aio_complete all the way down to the end of the function, after wakeup
and put_ioctx.  But then the ref counting on ioctx in aio_complete path is
Meaningless, which is the thing I'm trying to remove.

Re: [patch] aio: fix buggy put_ioctx call in aio_complete

[jmoyer]

==> Regarding RE: [patch] aio: fix buggy put_ioctx call in aio_complete; "Chen, Kenneth W" <kenneth.w.chen@intel.com> adds:

kenneth.w.chen> jmoyer@redhat.com wrote on Thursday, December 21, 2006 8:56
kenneth.w.chen> AM I think I'm going to abandon this whole synchronize
kenneth.w.chen> thing and going to put the wake up call inside ioctx_lock
kenneth.w.chen> spin lock along with the other patch you mentioned above in
kenneth.w.chen> the waiter path.  On top of that, I have another patch
kenneth.w.chen> attempts to perform wake-up only when the waiter can truly
kenneth.w.chen> proceed in aio_read_evt so dribbling I/O completion doesn't
kenneth.w.chen> inefficiently waking up waiter too frequently and only to
kenneth.w.chen> have waiter put back to sleep again. I will dig that up and
kenneth.w.chen> experiment.
>> In the mean time, can't we simply take the context lock in
>> wait_for_all_aios?  Unless I missed something, I think that will address
>> the reference count problem.

kenneth.w.chen> Take ioctx_lock is one part, the other part is to move

kenneth.w.chen> 	spin_unlock_irqrestore(&ctx->ctx_lock, flags);

kenneth.w.chen> in aio_complete all the way down to the end of the
kenneth.w.chen> function, after wakeup and put_ioctx.  But then the ref
kenneth.w.chen> counting on ioctx in aio_complete path is Meaningless,
kenneth.w.chen> which is the thing I'm trying to remove.

OK, right.  But are we simply papering over the real problem?  Earlier in
this thread, you stated:

> flush_workqueue() is not allowed to be called in the softirq context.
> However, aio_complete() called from I/O interrupt can potentially call
> put_ioctx with last ref count on ioctx and trigger a bug warning.  It
> is simply incorrect to perform ioctx freeing from aio_complete.

But how do we end up with the last reference to the ioctx in the aio
completion path?  That's a question I haven't seen answered.

-Jeff

RE: [patch] aio: fix buggy put_ioctx call in aio_complete

[Chen, Kenneth W]

jmoyer@redhat.com wrote on Thursday, December 21, 2006 9:35 AM
> kenneth.w.chen> Take ioctx_lock is one part, the other part is to move
> kenneth.w.chen> 	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
> kenneth.w.chen> in aio_complete all the way down to the end of the
> kenneth.w.chen> function, after wakeup and put_ioctx.  But then the ref
> kenneth.w.chen> counting on ioctx in aio_complete path is Meaningless,
> kenneth.w.chen> which is the thing I'm trying to remove.
> 
> OK, right.  But are we simply papering over the real problem?  Earlier in
> this thread, you stated:
> 
> > flush_workqueue() is not allowed to be called in the softirq context.
> > However, aio_complete() called from I/O interrupt can potentially call
> > put_ioctx with last ref count on ioctx and trigger a bug warning.  It
> > is simply incorrect to perform ioctx freeing from aio_complete.
> 
> But how do we end up with the last reference to the ioctx in the aio
> completion path?  That's a question I haven't seen answered.


It is explained in this posting, A race between io_destroy and aio_complete:
http://groups.google.com/group/linux.kernel/msg/d2ba7d73aca1dd0c?&hl=en

Trond spotted a bug in that posting.  The correct way of locking is to
move the spin_unlock_irqrestore in aio_complete all the way down instead
of calling aio_put_req at the end.  Like this:


--- ./fs/aio.c.orig	2006-12-21 08:08:14.000000000 -0800
+++ ./fs/aio.c	2006-12-21 08:14:27.000000000 -0800
@@ -298,17 +298,23 @@ static void wait_for_all_aios(struct kio
 	struct task_struct *tsk = current;
 	DECLARE_WAITQUEUE(wait, tsk);
 
+	spin_lock_irq(&ctx->ctx_lock);
 	if (!ctx->reqs_active)
-		return;
+		goto out;
 
 	add_wait_queue(&ctx->wait, &wait);
 	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
 	while (ctx->reqs_active) {
+		spin_unlock_irq(&ctx->ctx_lock);
 		schedule();
 		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		spin_lock_irq(&ctx->ctx_lock);
 	}
 	__set_task_state(tsk, TASK_RUNNING);
 	remove_wait_queue(&ctx->wait, &wait);
+
+out:
+	spin_unlock_irq(&ctx->ctx_lock);
 }
 
 /* wait_on_sync_kiocb:
@@ -424,7 +430,6 @@ static struct kiocb fastcall *__aio_get_
 	ring = kmap_atomic(ctx->ring_info.ring_pages[0], KM_USER0);
 	if (ctx->reqs_active < aio_ring_avail(&ctx->ring_info, ring)) {
 		list_add(&req->ki_list, &ctx->active_reqs);
-		get_ioctx(ctx);
 		ctx->reqs_active++;
 		okay = 1;
 	}
@@ -536,8 +541,6 @@ int fastcall aio_put_req(struct kiocb *r
 	spin_lock_irq(&ctx->ctx_lock);
 	ret = __aio_put_req(ctx, req);
 	spin_unlock_irq(&ctx->ctx_lock);
-	if (ret)
-		put_ioctx(ctx);
 	return ret;
 }
 
@@ -782,8 +785,7 @@ static int __aio_run_iocbs(struct kioctx
 		 */
 		iocb->ki_users++;       /* grab extra reference */
 		aio_run_iocb(iocb);
-		if (__aio_put_req(ctx, iocb))  /* drop extra ref */
-			put_ioctx(ctx);
+		__aio_put_req(ctx, iocb);
  	}
 	if (!list_empty(&ctx->run_list))
 		return 1;
@@ -998,14 +1000,10 @@ put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);
 
-	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
-
 	if (waitqueue_active(&ctx->wait))
 		wake_up(&ctx->wait);
 
-	if (ret)
-		put_ioctx(ctx);
-
+	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
 	return ret;
 }