On Tue, 2007-07-03 at 15:28 -0700, Zach Brown wrote:
> Linus, Andrew, please apply the bug fix patch at the end of this reply
> for .22.
>
> > >>One of our perf. team ran into this while doing some runs.
> > >>I didn't see anything obvious - it looks like we converted
> > >>async IO to synchronous one. I didn't spend much time digging
> > >>around.
>
> OK, I think this BUG_ON() is just broken. I wasn't able to find any
> obvious bugs from reading the code which would cause the BUG_ON() to
> fire. If it's reproducible I'd love to hear what the recipe is.
>
> I did notice that this BUG_ON() is evaluating dio after having dropped
> it's ref :/. So it's not completely absurd to fear that it's a race
> with the dio's memory being reused, but that'd be a pretty tight race.
>
> Let's remove this stupid BUG_ON and see if that test box still has
> trouble. It might just hit the valid BUG_ON a few lines down, but this
> unsafe BUG_ON needs to go.
I went through the code multiple times, I can't find how we can trigger
the BUG_ON(). But unfortunately, our perf. team is able reproduce the
problem. Debug indicated that, the ret2 == 1 :(
Not sure how that can happen. Ideas ?
Thanks,
Badari
>
> -------
>
> dio: remove bogus refcounting BUG_ON
>
> Badari Pulavarty reported a case of this BUG_ON is triggering during
> testing. It's completely bogus and should be removed.
>
> It's trying to notice if we left references to the dio hanging around in
> the sync case. They should have been dropped as IO completed while this
> path was in dio_await_completion(). This condition will also be
> checked, via some twisty logic, by the BUG_ON(ret != -EIOCBQUEUED) a few
> lines lower. So to start this BUG_ON() is redundant.
>
> More fatally, it's dereferencing dio-> after having dropped its
> reference. It's only safe to dereference the dio after releasing the
> lock if the final reference was just dropped. Another CPU might free
> the dio in bio completion and reuse the memory after this path drops the
> dio lock but before the BUG_ON() is evaluated.
>
> This patch passed aio+dio regression unit tests and aio-stress on ext3.
>
> Signed-off-by: Zach Brown <zach.brown@xxxxxxxxxx>
> Cc: Badari Pulavarty <pbadari@xxxxxxxxxx>
>
> diff -r 509ce354ae1b fs/direct-io.c
> --- a/fs/direct-io.c Sun Jul 01 22:00:49 2007 +0000
> +++ b/fs/direct-io.c Tue Jul 03 14:56:41 2007 -0700
> @@ -1106,7 +1106,7 @@ direct_io_worker(int rw, struct kiocb *i
> spin_lock_irqsave(&dio->bio_lock, flags);
> ret2 = --dio->refcount;
> spin_unlock_irqrestore(&dio->bio_lock, flags);
> - BUG_ON(!dio->is_async && ret2 != 0);
> +
> if (ret2 == 0) {
> ret = dio_complete(dio, offset, ret);
> kfree(dio);
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
