[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <49B960DE.6000406@cosmosbay.com>
Date: Thu, 12 Mar 2009 20:22:06 +0100
From: Eric Dumazet <dada1@...mosbay.com>
To: Andrew Morton <akpm@...ux-foundation.org>
CC: Jeff Moyer <jmoyer@...hat.com>, Avi Kivity <avi@...hat.com>,
linux-aio <linux-aio@...ck.org>, zach.brown@...cle.com,
bcrl@...ck.org, linux-kernel@...r.kernel.org,
Davide Libenzi <davidel@...ilserver.org>,
Christoph Lameter <cl@...ux-foundation.org>
Subject: Re: [PATCH] fs: fput() can be called from interrupt context
Andrew Morton a écrit :
> On Thu, 12 Mar 2009 07:10:38 +0100 Eric Dumazet <dada1@...mosbay.com> wrote:
>
>>> Did you reproduce the bug, and confirm that the patch fixes it?
>> take Davide program : http://www.xmailserver.org/eventfd-aio-test.c
>>
>> and add at line 318 :
>> close(afd);
>>
>> It should produce the kernel bug...
>
> "should"?
Sorry I had to run this morning, so I was a litle bit lazy...
Maybe some kind of O_DIRECT / NFS / blockdev setup or something calling
aio_complete() from interrupt context ? I am not familiar of this part,
but considering spin_lock_irq()/spin_unlock_irq() calls in fs/aio.c,
there must be a reason for this ?
>
>>> Are there simpler ways of fixing it? Maybe sneak a call to
>>> wait_for_all_aios() into the right place? I doubt if it's performance
>>> critical, as nobody seems to have ever hit the bug.
>> Take the time to check how fs/aio.c handle the fput(req->ki_filp) case
>> (or read my 2nd patch, it should spot the thing)
>
> Well yes, a kludge like that seems a bit safer.
>
> It's somewhat encouraging that we're apparently already doing fput()
> from within keventd (although how frequently?). There might be
> problems with file locking, security code, etc from doing fput() from
> an unexpected thread. And then there are all the usual weird problem
> with using the keventd queues which take a long time to get discovered.
>
Hum... Do you mean "if (in_interrupt())" is not the right test to
perform ?
>
>> If you want to add another kludge to properly fput(req->ki_eventfd),
>> be my guest :-(
>>
>>> Bear in mind that if the bug _is_ real then it's now out there, and
>>> we would like a fix which is usable by 2.6.<two-years-worth>.
>
> The patches are large and scary and it would be a real problem to merge
> them into 2.6.29 at this stage, let alone 2.6.25, etc.
>
> Especially as the code which you sent out appears to be untested:
>
I actually tested it and got a working kernel before sending patch,
please trust me...
Oh yes, my dev machine always called the slow path then, this is
why I didnt noticed.
Thank you for spotting this inverted test.
>> void fput(struct file *file)
>> {
>> - if (atomic_long_dec_and_test(&file->f_count))
>> - __fput(file);
>> + if (atomic_long_dec_and_test(&file->f_count)) {
>> + if (unlikely(!in_interrupt()))
>
> ^
>
>> + fd_defer_queue(NULL, file);
>> + else
>> + __fput(file);
>> + }
>> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists