On Fri, Aug 22, 2014 at 12:26:30PM -0400, Benjamin LaHaise wrote:
> On Fri, Aug 22, 2014 at 07:15:02PM +0300, Dan Aloni wrote:
> > Sorry, I was waiting for a new patch from your direction, I should
> > have replied earlier. What bothered me about the patch you sent is that
> > completed_events is added as a new field but nothing assigns to it, so I
> > wonder how it can be effective.
>
> Ah, that was missing a hunk then. Try this version instead.
Ben, seems that the test program needs some twidling to make the bug
appear still by setting MAX_IOS to 256 (and it still passes on a
kernel with the original patch reverted). Under this condition the
ring buffer size remains 128 (here, 32*4 CPUs), and it is overrun on
the second attempt.
$ ./aio_bug
Submitting: 256
Submitted: 126
Submitting: 130
Submitted too much, that's okay
Completed: 126
Submitting: 130
Submitted: 130
<stuck>
I think I have found two problems with your patch: first, the
completed_events field is never decremented so it goes up until 2^32
wraparound. So I tested with 'ctx->completed_events -= completed;'
there (along with some prints), but testing revealed that this didn't
solve the problem, so secondly, I also fixed the 'avail = ' line. The
case where the 'head > tail' case didn't look correct to me.
So the good news is that it works now with fix below and MAX_IOS=256
and even with MAX_IOS=512. You can git-amend this it to your patch I
guess.
Signed-off: Dan Aloni <dan@xxxxxxxxxxxx>
diff --git a/fs/aio.c b/fs/aio.c
index 6982357d9372..eafc96c60a8c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -893,12 +893,20 @@ static void refill_reqs_available(struct kioctx *ctx)
tail = ACCESS_ONCE(ring->tail);
kunmap_atomic(ring);
- avail = (head <= tail ? tail : ctx->nr_events) - head;
+ if (head <= tail)
+ avail = tail - head;
+ else
+ avail = ctx->nr_events - (head - tail);
+
completed = ctx->completed_events;
+ pr_debug("%u %u h%u t%u\n", avail, completed, head, tail);
if (avail < completed)
completed -= avail;
else
completed = 0;
+ pr_debug("completed %u\n", completed);
+
+ ctx->completed_events -= completed;
put_reqs_available(ctx, completed);
}
BTW, I am not an expert on this code so I am still not sure that
'ctx->completed_events' wouldn't get wrong if for instance - one SMP
core does userspace event reaping and another calls io_submit(). Do
you think it would work alright in that case?
--
Dan Aloni
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html