RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
> kjournald submited buffers for IO and waiting for them to finish.
It is means that the patch incorrectly moves internal kernel
synchronization
problem into user space as EIO instead of fixing a root cause or perform
iterative
synchronization.
After patching users will be surprised a lot of EIO
(remember 47% EIO in aio-stress runs after patching) will buy new
disk...
This patch is harmful.
Leonid
>-Original Message-
>From: Zach Brown [mailto:[EMAIL PROTECTED]
>Sent: Wednesday, February 21, 2007 9:25 PM
>To: Ken Chen
>Cc: Ananiev, Leonid I; Chris Mason; linux-aio; Linux Kernel Mailing
List; Benjamin LaHaise; Suparna
>bhattacharya; Andrew Morton; Badari Pulavarty
>Subject: Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to
completion event
>
>
>On Feb 21, 2007, at 12:35 AM, Ken Chen wrote:
>
>> On 2/20/07, Ananiev, Leonid <[EMAIL PROTECTED]> wrote:
>>> 1) mem=1G in kernel boot param if you have more
>>> 2) unmount; mk2fs; mount
>>> 3) dd if=/dev/zero of= bs=1M count=1200
>>> 4) aiostress -s 1200m -O -o 2 -i 1 -r 16k
>>> 5) if i++<50 goto 2).
>>
>> Would you please instrument the call chain of
>> invalidate_complete_page2() and tell us exactly where it returns zero
>> value in your failure case?
>>
>> invalidate_complete_page2
>> try_to_release_page
>> ext3_releasepage
>>journal_try_to_free_buffers
>> ???
>
>For what it's worth, Badari has explained this race in the past in a
>credible way. I'll take the liberty of pasting a mail from him:
>
>"
>kjournald submited buffers for IO and waiting for them to finish.
>Note that it has a ref. against the buffer.
>
>journal_commit_transaction()
> ...
> submited buffers for IO
> /* Waiting for IO to complete */
> while (commit_transaction->t_locked_list) {
> ...
> get_bh(bh);
> if (buffer_locked(bh)) {
> spin_unlock(&journal->j_list_lock);
> wait_on_buffer(bh); <<<<<<
> spin_lock(&journal->j_list_lock);
> }
>
> ..
> put_bh(bh);
> }
>
>Now, DIO process comes to frees the jh through
>journal_try_to_free_buffers()
>but fails to drop_buffers() since kjournald() has a reference against
>it.
>invalidate_inode_pages2_range()
> ..
> ext3_releasepage()
> journal_try_to_free_buffers()
> journal_put_journal_head()
> __journal_try_to_free_buffer()
> <--- freed jh
>
> try_to_free_buffers()
> drop_buffers()
> if (buffer_busy(bh))
> goto failed;
> <<--- returns EIO due to
>b_count
>
>"
>
>I don't mean to say that we shouldn't get traces to confirm the
>theory, just sharing. And now we can point to this in the archives
>next time :).
>
>- z
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
This is an interesting trick, but I'd like to consider hard whether
the added
complexity is worth it. Could you list the various other cases you
have in mind
which would want to use it ?
I'm happy to report that the sync case and the
invalidate_inode_pages2_range() case are the only two which try to
rewrite 'ret'. I audited all the call paths between aio_{read,write}
and -EIOCBQUUEUED.
So, sure, maybe we can shuffle the house of cards a little in the
direction away from having a fs/aio.c helper for the situation by
simply removing the few current instances of the problem. It won't
scale as people add problems without knowing about the rule to not
overwrite -EIOCBQUEUED, but hopefully that won't be a rule for much
longer :) :).
For the O_SYNC case we could add another magical test to the is_async
assignment which won't perform AIO on O_SYNC descriptors.
For the invalidate_inode_pages2_range() case, I wonder if the right
place to issue this is after the direct IO write has completed and
before
aio_complete() is issued (somewhat like the way we do
bio_check_pages_dirty
for DIO reads), rather than after submission when the IO may still
not have
hit the disk. This would also make the behaviour uniform for
synchronous and
async cases.
Hmm, I think I like that. It solves the problem for the current sole
user of -EIOCBQUEUED without too much disruption.
BTW, am I right in interpreting that with your change aio_complete
() may
trigger an io_getevents() wakeup, before the corresponding event is
placed
on the ring buffer ?
Hmm, yeah, it looks like I goofed that.
I'll roll a patch which does the invalidation down in fs/direct-io.c.
- z
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On Feb 21, 2007, at 12:35 AM, Ken Chen wrote:
On 2/20/07, Ananiev, Leonid <[EMAIL PROTECTED]> wrote:
1) mem=1G in kernel boot param if you have more
2) unmount; mk2fs; mount
3) dd if=/dev/zero of= bs=1M count=1200
4) aiostress -s 1200m -O -o 2 -i 1 -r 16k
5) if i++<50 goto 2).
Would you please instrument the call chain of
invalidate_complete_page2() and tell us exactly where it returns zero
value in your failure case?
invalidate_complete_page2
try_to_release_page
ext3_releasepage
journal_try_to_free_buffers
???
For what it's worth, Badari has explained this race in the past in a
credible way. I'll take the liberty of pasting a mail from him:
"
kjournald submited buffers for IO and waiting for them to finish.
Note that it has a ref. against the buffer.
journal_commit_transaction()
...
submited buffers for IO
/* Waiting for IO to complete */
while (commit_transaction->t_locked_list) {
...
get_bh(bh);
if (buffer_locked(bh)) {
spin_unlock(&journal->j_list_lock);
wait_on_buffer(bh); <<
spin_lock(&journal->j_list_lock);
}
..
put_bh(bh);
}
Now, DIO process comes to frees the jh through
journal_try_to_free_buffers()
but fails to drop_buffers() since kjournald() has a reference against
it.
invalidate_inode_pages2_range()
..
ext3_releasepage()
journal_try_to_free_buffers()
journal_put_journal_head()
__journal_try_to_free_buffer()
<--- freed jh
try_to_free_buffers()
drop_buffers()
if (buffer_busy(bh))
goto failed;
<<--- returns EIO due to
b_count
"
I don't mean to say that we shouldn't get traces to confirm the
theory, just sharing. And now we can point to this in the archives
next time :).
- z
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On Mon, Feb 19, 2007 at 01:38:35PM -0800, Zach Brown wrote:
> aio: propogate post-EIOCBQUEUED errors to completion event
>
> This addresses an oops reported by Leonid Ananiev <[EMAIL PROTECTED]>
> as archived at http://lkml.org/lkml/2007/2/8/337.
>
> O_DIRECT kicks off bios and returns -EIOCBQUEUED to indicate its intention to
> call aio_complete() once the bios complete. As we return from submission we
> must preserve the -EIOCBQUEUED return code so that fs/aio.c knows to let the
> bio completion call aio_complete(). This stops us from returning errors after
> O_DIRECT submission.
>
> But we have a few places that are very interested in generating errors after
> bio submission.
>
> The most critical of these is invalidating the page cache after a write. This
> avoids exposing stale data to buffered operations that are performed after the
> O_DIRECT write succeeds. We must do this after submission because the user
> buffer might have been an mmap()ed buffer of the region being written to. The
> get_user_pages() in the O_DIRECT completion path could have faulted in stale
> data.
>
> So this patch introduces a helper, aio_propogate_error(), which queues
> post-submission errors in the iocb so that they are given to the user
> completion event when aio_complete() is finally called.
>
> To get this working we change the aio_complete() path so that the ring
> insertion is performed as the final iocb reference is dropped. This gives the
> submission path time to queue its pending error before it drops its reference.
> This increases the space in the iocb as it has to record the two result codes
> from aio_complete() and the pending error from the submission path.
This is an interesting trick, but I'd like to consider hard whether the added
complexity is worth it. Could you list the various other cases you have in mind
which would want to use it ?
For the invalidate_inode_pages2_range() case, I wonder if the right
place to issue this is after the direct IO write has completed and before
aio_complete() is issued (somewhat like the way we do bio_check_pages_dirty
for DIO reads), rather than after submission when the IO may still not have
hit the disk. This would also make the behaviour uniform for synchronous and
async cases.
BTW, am I right in interpreting that with your change aio_complete() may
trigger an io_getevents() wakeup, before the corresponding event is placed
on the ring buffer ?
Regards
Suparna
>
> This was tested by running O_DIRECT aio-stress concurrently with buffered
> reads
> while triggering EIO in invalidate_inode_pages2_range() with the help of a
> debugfs bool hack. Previously the kernel would oops as fs/aio.c and bio
> completion both called aio_complete(). With this patch aio-stress sees -EIO.
>
> Signed-off-by: Zach Brown <[EMAIL PROTECTED]>
> ---
>
> fs/aio.c| 49 +-
> include/linux/aio.h | 30 +
> mm/filemap.c|4 +--
> 3 files changed, 57 insertions(+), 26 deletions(-)
>
> --- a/fs/aio.cMon Feb 19 13:12:20 2007 -0800
> +++ b/fs/aio.cMon Feb 19 13:16:00 2007 -0800
> @@ -193,8 +193,7 @@ static int aio_setup_ring(struct kioctx
> kunmap_atomic((void *)((unsigned long)__event & PAGE_MASK), km); \
> } while(0)
>
> -static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb *iocb,
> - long res, long res2)
> +static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb *iocb)
> {
> struct aio_ring_info*info;
> struct aio_ring *ring;
> @@ -213,12 +212,12 @@ static void aio_ring_insert_entry(struct
>
> event->obj = (u64)(unsigned long)iocb->ki_obj.user;
> event->data = iocb->ki_user_data;
> - event->res = res;
> - event->res2 = res2;
> -
> - dprintk("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
> + event->res = iocb->ki_pending_err ? iocb->ki_pending_err : iocb->ki_res;
> + event->res2 = iocb->ki_res2;
> +
> + dprintk("aio_complete: %p[%lu]: %p: %p %Lx %d %lx %lx\n",
> ctx, tail, iocb, iocb->ki_obj.user, iocb->ki_user_data,
> - res, res2);
> + iocb->ki_pending_err, iocb->ki_res, iocb->ki_res2);
>
> /* after flagging the request as done, we
>* must never even look at it again
> @@ -459,6 +458,7 @@ static struct kiocb fastcall *__aio_get_
> req->ki_cancel = NULL;
> req->ki_retry = NULL;
> req->ki_dtor = NULL;
> + req->ki_pending_err = 0;
> req->private = NULL;
> req->ki_iovec = NULL;
> INIT_LIST_HEAD(&req->ki_run_list);
> @@ -548,10 +548,14 @@ static int __aio_put_req(struct kioctx *
>
> assert_spin_locked(&ctx->ctx_lock);
>
> - req->ki_users --;
> + req->ki_users--;
> BUG_ON(req->ki_users < 0);
> if (likely(req->ki_users))
> return 0;
> +
> + if (kiocbIsInserted(req))
> + aio_ring_insert_entry(ctx, req);
>
RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
> where it returns zero I've wrote in the mail http://lkml.org/lkml/2007/2/8/337 invalidate_inode_pages2_range() reports BUG: warning at mm/truncate.c:398 occurs becouse of invalidate_complete_page2()returns 0; it returns 0 because of try_to_release_page() returns 0; it returns 0 because of ext3_releasepage() returns 0; it returns 0 because of journal_try_to_free_buffers() returns 0; it returns 0 because of try_to_free_buffers() returns 0; it returns 0 because of drop_buffers() returns 0; it returns 0 because of buffer_busy(bh)returns 1; it returns 0 because of buffer_head count is 1 (bh->b_count==1) as additional printk reports. Leonid -Original Message- From: Ken Chen [mailto:[EMAIL PROTECTED] Sent: Wednesday, February 21, 2007 11:36 AM To: Ananiev, Leonid I Cc: Chris Mason; Zach Brown; [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; Benjamin LaHaise; Suparna bhattacharya; Andrew Morton Subject: Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event On 2/20/07, Ananiev, Leonid <[EMAIL PROTECTED]> wrote: > 1) mem=1G in kernel boot param if you have more > 2) unmount; mk2fs; mount > 3) dd if=/dev/zero of= bs=1M count=1200 > 4) aiostress -s 1200m -O -o 2 -i 1 -r 16k > 5) if i++<50 goto 2). Would you please instrument the call chain of invalidate_complete_page2() and tell us exactly where it returns zero value in your failure case? invalidate_complete_page2 try_to_release_page ext3_releasepage journal_try_to_free_buffers ??? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On 2/20/07, Ananiev, Leonid <[EMAIL PROTECTED]> wrote: 1) mem=1G in kernel boot param if you have more 2) unmount; mk2fs; mount 3) dd if=/dev/zero of= bs=1M count=1200 4) aiostress -s 1200m -O -o 2 -i 1 -r 16k 5) if i++<50 goto 2). Would you please instrument the call chain of invalidate_complete_page2() and tell us exactly where it returns zero value in your failure case? invalidate_complete_page2 try_to_release_page ext3_releasepage journal_try_to_free_buffers ??? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
-O I've missed:
aiostress -s 1200m -O -o 2 -i 1 -r 16k
You are right I've used harness scripts.
i=0; while ((i++<50)); do ~/bm/bin/runs I2 -; done &
It runs bmrun harness script which is long for different hardware
configurations and test options.
The lines in aio-stress wrapper which could be useful for you are:
gcc aiostress.c -Wall -o aiostress -lpthread -laio
2>>${file_log}
...
sudo umount -f ${pdisk} ${ppath} > /dev/null 2>&1
sudo /sbin/mke2fs -j ${pdisk} > /dev/null 2>>${file_log}
sudo mount ${pdisk} ${ppath} > /dev/null 2>>${file_log}...
export LD_LIBRARY_PATH=/lib:/usr/lib:$LD_LIBRARY_PATH
all_files_size=1200 # in megabytes
...
else
num_disks=1
((file_size = all_files_size / num_disks))
testfiles=${ppath}/test/testfile
dd if=/dev/zero of=$testfiles bs=1M count=${file_size} >
/dev/null
fi
aiostress ${*} -v -o 2 -s ${file_size}m -i 1 -r 16k $testfiles >
${file_res} 2>&1
if [ "$1" == "-S" ] ; then
info="O_SYNC"
elif [ "$1" == "-O" ] ; then
info="O_DIRECT"
fi
awk ${file_res}
Which does
1) mem=1G in kernel boot param if you have more
2) unmount; mk2fs; mount
3) dd if=/dev/zero of= bs=1M count=1200
4) aiostress -s 1200m -O -o 2 -i 1 -r 16k
5) if i++<50 goto 2).
Leonid
-Original Message-
From: Chris Mason [mailto:[EMAIL PROTECTED]
Sent: Tuesday, February 20, 2007 8:55 PM
To: Ananiev, Leonid I
Cc: Zach Brown; [EMAIL PROTECTED]; linux-kernel@vger.kernel.org;
Benjamin LaHaise; Suparna bhattacharya; Andrew Morton
Subject: Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to
completion event
On Tue, Feb 20, 2007 at 08:17:51PM +0300, Ananiev, Leonid I wrote:
> aio-stress command lines used for test
> 1) mem=1G in kernel boot param if you have more
> 2) mk2fs for test_file
> 3) dd if=/dev/zero of= bs=1M count=1200
> 4) aiostress -s 1200m -o 2 -i 1 -r 16k
>
Sorry, this aio-stress command line doesn't do O_DIRECT. Do you have
this bundled into a test script that I can try?
-chris
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On Feb 20, 2007, at 9:33 AM, Ananiev, Leonid I wrote: There is change in the patch which is uncommented in preface. Now aio_ring_insert_entry() is not called if req->ki_users>=1. Before was called. Could you comment it? It is described in the patch description, though perhaps not explicitly enough: "To get this working we change the aio_complete() path so that the ring insertion is performed as the final iocb reference is dropped." This stops an aio_complete() from generating a completion event before the submission path has had a chance to generate an error. This could happen if O_DIRECT bios completed very quickly before the submission path had a chance to return up and call ivalidate_inode_pages2_range(). - z - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On Tue, Feb 20, 2007 at 08:17:51PM +0300, Ananiev, Leonid I wrote: > aio-stress command lines used for test > 1) mem=1G in kernel boot param if you have more > 2) mk2fs for test_file > 3) dd if=/dev/zero of= bs=1M count=1200 > 4) aiostress -s 1200m -o 2 -i 1 -r 16k > Sorry, this aio-stress command line doesn't do O_DIRECT. Do you have this bundled into a test script that I can try? -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
There is change in the patch which is uncommented in preface.
Now aio_ring_insert_entry() is not called if req->ki_users>=1.
Before was called.
Could you comment it?
Leonid
-Original Message-
From: Zach Brown [mailto:[EMAIL PROTECTED]
Sent: Tuesday, February 20, 2007 12:39 AM
To: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Cc: Benjamin LaHaise; Suparna bhattacharya; Andrew Morton; Ananiev,
Leonid I
Subject: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to
completion event
aio: propogate post-EIOCBQUEUED errors to completion event
This addresses an oops reported by Leonid Ananiev
<[EMAIL PROTECTED]>
as archived at http://lkml.org/lkml/2007/2/8/337.
O_DIRECT kicks off bios and returns -EIOCBQUEUED to indicate its
intention to
call aio_complete() once the bios complete. As we return from
submission we
must preserve the -EIOCBQUEUED return code so that fs/aio.c knows to let
the
bio completion call aio_complete(). This stops us from returning errors
after
O_DIRECT submission.
But we have a few places that are very interested in generating errors
after
bio submission.
The most critical of these is invalidating the page cache after a write.
This
avoids exposing stale data to buffered operations that are performed
after the
O_DIRECT write succeeds. We must do this after submission because the
user
buffer might have been an mmap()ed buffer of the region being written
to. The
get_user_pages() in the O_DIRECT completion path could have faulted in
stale
data.
So this patch introduces a helper, aio_propogate_error(), which queues
post-submission errors in the iocb so that they are given to the user
completion event when aio_complete() is finally called.
To get this working we change the aio_complete() path so that the ring
insertion is performed as the final iocb reference is dropped. This
gives the
submission path time to queue its pending error before it drops its
reference.
This increases the space in the iocb as it has to record the two result
codes
from aio_complete() and the pending error from the submission path.
This was tested by running O_DIRECT aio-stress concurrently with
buffered reads
while triggering EIO in invalidate_inode_pages2_range() with the help of
a
debugfs bool hack. Previously the kernel would oops as fs/aio.c and bio
completion both called aio_complete(). With this patch aio-stress sees
-EIO.
Signed-off-by: Zach Brown <[EMAIL PROTECTED]>
---
fs/aio.c| 49 +-
include/linux/aio.h | 30 +
mm/filemap.c|4 +--
3 files changed, 57 insertions(+), 26 deletions(-)
--- a/fs/aio.c Mon Feb 19 13:12:20 2007 -0800
+++ b/fs/aio.c Mon Feb 19 13:16:00 2007 -0800
@@ -193,8 +193,7 @@ static int aio_setup_ring(struct kioctx
kunmap_atomic((void *)((unsigned long)__event & PAGE_MASK), km);
\
} while(0)
-static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb
*iocb,
- long res, long res2)
+static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb
*iocb)
{
struct aio_ring_info*info;
struct aio_ring *ring;
@@ -213,12 +212,12 @@ static void aio_ring_insert_entry(struct
event->obj = (u64)(unsigned long)iocb->ki_obj.user;
event->data = iocb->ki_user_data;
- event->res = res;
- event->res2 = res2;
-
- dprintk("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
+ event->res = iocb->ki_pending_err ? iocb->ki_pending_err :
iocb->ki_res;
+ event->res2 = iocb->ki_res2;
+
+ dprintk("aio_complete: %p[%lu]: %p: %p %Lx %d %lx %lx\n",
ctx, tail, iocb, iocb->ki_obj.user, iocb->ki_user_data,
- res, res2);
+ iocb->ki_pending_err, iocb->ki_res, iocb->ki_res2);
/* after flagging the request as done, we
* must never even look at it again
@@ -459,6 +458,7 @@ static struct kiocb fastcall *__aio_get_
req->ki_cancel = NULL;
req->ki_retry = NULL;
req->ki_dtor = NULL;
+ req->ki_pending_err = 0;
req->private = NULL;
req->ki_iovec = NULL;
INIT_LIST_HEAD(&req->ki_run_list);
@@ -548,10 +548,14 @@ static int __aio_put_req(struct kioctx *
assert_spin_locked(&ctx->ctx_lock);
- req->ki_users --;
+ req->ki_users--;
BUG_ON(req->ki_users < 0);
if (likely(req->ki_users))
return 0;
+
+ if (kiocbIsInserted(req))
+ aio_ring_insert_entry(ctx, req);
+
list_del(&req->ki_list);/* remove from
active_reqs */
req->ki_cancel = NULL;
req->ki_retry = NULL;
@@ -983,27 +987,24 @@ int fastcall aio_complete(struct kiocb *
return 1;
}
- /* add a completion event to the ring buffer.
-
RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
aio-stress command lines used for test 1) mem=1G in kernel boot param if you have more 2) mk2fs for test_file 3) dd if=/dev/zero of= bs=1M count=1200 4) aiostress -s 1200m -o 2 -i 1 -r 16k Leonid -Original Message- From: Chris Mason [mailto:[EMAIL PROTECTED] Sent: Tuesday, February 20, 2007 8:04 PM To: Ananiev, Leonid I Cc: Zach Brown; [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; Benjamin LaHaise; Suparna bhattacharya; Andrew Morton Subject: Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event On Tue, Feb 20, 2007 at 07:57:49PM +0300, Ananiev, Leonid I wrote: > Zach> This addresses an oops reported by Leonid Ananiev > <[EMAIL PROTECTED]> > Zach> as archived at http://lkml.org/lkml/2007/2/8/337. > > Zach> This was tested by running O_DIRECT aio-stress concurrently with > buffered reads > > The oops was with aio-stress only running in the loop > WITHOUT buffered or mmaped IO which are patched and discussed now. > Actually 47% aio is finished with EIO after patch. I looked through the thread and couldn't find the aio-stress command line used for the whole test. Could you please post it here? If aio+dio is being used to extend the file, you'll get pages in the page cache, which is hopefully why you're able to trigger the EIOs. -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
On Tue, Feb 20, 2007 at 07:57:49PM +0300, Ananiev, Leonid I wrote: > Zach> This addresses an oops reported by Leonid Ananiev > <[EMAIL PROTECTED]> > Zach> as archived at http://lkml.org/lkml/2007/2/8/337. > > Zach> This was tested by running O_DIRECT aio-stress concurrently with > buffered reads > > The oops was with aio-stress only running in the loop > WITHOUT buffered or mmaped IO which are patched and discussed now. > Actually 47% aio is finished with EIO after patch. I looked through the thread and couldn't find the aio-stress command line used for the whole test. Could you please post it here? If aio+dio is being used to extend the file, you'll get pages in the page cache, which is hopefully why you're able to trigger the EIOs. -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
Zach> This addresses an oops reported by Leonid Ananiev
<[EMAIL PROTECTED]>
Zach> as archived at http://lkml.org/lkml/2007/2/8/337.
Zach> This was tested by running O_DIRECT aio-stress concurrently with
buffered reads
The oops was with aio-stress only running in the loop
WITHOUT buffered or mmaped IO which are patched and discussed now.
Actually 47% aio is finished with EIO after patch.
Leonnid
-Original Message-
From: Zach Brown [mailto:[EMAIL PROTECTED]
Sent: Tuesday, February 20, 2007 12:39 AM
To: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Cc: Benjamin LaHaise; Suparna bhattacharya; Andrew Morton; Ananiev,
Leonid I
Subject: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to
completion event
aio: propogate post-EIOCBQUEUED errors to completion event
This addresses an oops reported by Leonid Ananiev
<[EMAIL PROTECTED]>
as archived at http://lkml.org/lkml/2007/2/8/337.
O_DIRECT kicks off bios and returns -EIOCBQUEUED to indicate its
intention to
call aio_complete() once the bios complete. As we return from
submission we
must preserve the -EIOCBQUEUED return code so that fs/aio.c knows to let
the
bio completion call aio_complete(). This stops us from returning errors
after
O_DIRECT submission.
But we have a few places that are very interested in generating errors
after
bio submission.
The most critical of these is invalidating the page cache after a write.
This
avoids exposing stale data to buffered operations that are performed
after the
O_DIRECT write succeeds. We must do this after submission because the
user
buffer might have been an mmap()ed buffer of the region being written
to. The
get_user_pages() in the O_DIRECT completion path could have faulted in
stale
data.
So this patch introduces a helper, aio_propogate_error(), which queues
post-submission errors in the iocb so that they are given to the user
completion event when aio_complete() is finally called.
To get this working we change the aio_complete() path so that the ring
insertion is performed as the final iocb reference is dropped. This
gives the
submission path time to queue its pending error before it drops its
reference.
This increases the space in the iocb as it has to record the two result
codes
from aio_complete() and the pending error from the submission path.
This was tested by running O_DIRECT aio-stress concurrently with
buffered reads
while triggering EIO in invalidate_inode_pages2_range() with the help of
a
debugfs bool hack. Previously the kernel would oops as fs/aio.c and bio
completion both called aio_complete(). With this patch aio-stress sees
-EIO.
Signed-off-by: Zach Brown <[EMAIL PROTECTED]>
---
fs/aio.c| 49 +-
include/linux/aio.h | 30 +
mm/filemap.c|4 +--
3 files changed, 57 insertions(+), 26 deletions(-)
--- a/fs/aio.c Mon Feb 19 13:12:20 2007 -0800
+++ b/fs/aio.c Mon Feb 19 13:16:00 2007 -0800
@@ -193,8 +193,7 @@ static int aio_setup_ring(struct kioctx
kunmap_atomic((void *)((unsigned long)__event & PAGE_MASK), km);
\
} while(0)
-static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb
*iocb,
- long res, long res2)
+static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb
*iocb)
{
struct aio_ring_info*info;
struct aio_ring *ring;
@@ -213,12 +212,12 @@ static void aio_ring_insert_entry(struct
event->obj = (u64)(unsigned long)iocb->ki_obj.user;
event->data = iocb->ki_user_data;
- event->res = res;
- event->res2 = res2;
-
- dprintk("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
+ event->res = iocb->ki_pending_err ? iocb->ki_pending_err :
iocb->ki_res;
+ event->res2 = iocb->ki_res2;
+
+ dprintk("aio_complete: %p[%lu]: %p: %p %Lx %d %lx %lx\n",
ctx, tail, iocb, iocb->ki_obj.user, iocb->ki_user_data,
- res, res2);
+ iocb->ki_pending_err, iocb->ki_res, iocb->ki_res2);
/* after flagging the request as done, we
* must never even look at it again
@@ -459,6 +458,7 @@ static struct kiocb fastcall *__aio_get_
req->ki_cancel = NULL;
req->ki_retry = NULL;
req->ki_dtor = NULL;
+ req->ki_pending_err = 0;
req->private = NULL;
req->ki_iovec = NULL;
INIT_LIST_HEAD(&req->ki_run_list);
@@ -548,10 +548,14 @@ static int __aio_put_req(struct kioctx *
assert_spin_locked(&ctx->ctx_lock);
- req->ki_users --;
+ req->ki_users--;
BUG_ON(req->ki_users < 0);
if (likely(req->ki_users))
return 0;
+
+ if (kiocbIsInserted(req))
+ aio_ring_insert_entry(ctx, req);
+
list_del(&req->ki_list);/* remove from
activ
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
Zach> So this patch introduces a helper, aio_propogate_error(),
...which is spelled incorrectly: aio_propagate_error.
Man, I am batting 1000! Randy also made fun of my 'intead'.
Zach> which queues post-submission errors in the iocb so that they are
Zach> given to the user completion event when aio_complete() is
Zach> finally called.
Ugly, but I can't think of a better way to do it, either.
Yeah, this seemed to be the lesser of the available evils. We (Chris
and I, while in california) considered introducing a primitive to
have the submission path wait for aio_complete() to be called so that
it could just return the error. We also thought about turning EIOCB
{RETRY,QUEUED} into bits on the iocb instead of return codes that we
have to lovingly pass back up to fs/aio.c.
This seemed to be the least intrusive :/.
- z
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
==> On Mon, 19 Feb 2007 13:38:35 -0800 (PST), Zach Brown <[EMAIL PROTECTED]>
said:
Zach> This addresses an oops reported by Leonid Ananiev <[EMAIL PROTECTED]>
Zach> as archived at http://lkml.org/lkml/2007/2/8/337.
[snip]
Zach> So this patch introduces a helper, aio_propogate_error(),
...which is spelled incorrectly: aio_propagate_error.
Zach> which queues post-submission errors in the iocb so that they are
Zach> given to the user completion event when aio_complete() is
Zach> finally called.
Ugly, but I can't think of a better way to do it, either.
> +
> + if (kiocbIsInserted(req))
> + aio_ring_insert_entry(ctx, req);
> +
This confused me at first reading. If it's inserted, then insert it.
Perhaps KIF_COMPLETED would be a better name?
> +/*
> + * This function is used to make sure that an error is communicated to
> + * userspace on iocb completion without stopping -EIOCBQUEUED from bubbling
> up
> + * to fs/aio.c from the place where it originated.
> + *
> + * If we have an existing -EIOCBQUEUED it must be returned all the way to
> + * fs/aio.c so that it doesn't double-complete the iocb along with whoever
> + * returned -EIOCBQUEUED.. In that case we put the new error in the iocb.
> It
> + * will be returned to userspace *intead of* the first result code given to
> + * aio_complete(). Use this only for errors which must overwrite whatever
> the
> + * return code might have been. The first non-zero new_err given to this
> + * function for a given iocb will be returned to userspace.
> + */
> +static inline int aio_propogate_error(struct kiocb *iocb, int existing_err,
> + int new_err)
> +{
> + if (existing_err != -EIOCBQUEUED)
> + return new_err;
> + if (!iocb->ki_pending_err)
> + iocb->ki_pending_err = new_err;
> + return -EIOCBQUEUED;
> +}
> +
I think that we would ideally cancel the I/Os in flight since we are
going to throw away the results. However, we don't exactly support
cancellation today. So, I have no objection to this less optimal
solution.
-Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] aio: propogate post-EIOCBQUEUED errors to completion event
aio: propogate post-EIOCBQUEUED errors to completion event
This addresses an oops reported by Leonid Ananiev <[EMAIL PROTECTED]>
as archived at http://lkml.org/lkml/2007/2/8/337.
O_DIRECT kicks off bios and returns -EIOCBQUEUED to indicate its intention to
call aio_complete() once the bios complete. As we return from submission we
must preserve the -EIOCBQUEUED return code so that fs/aio.c knows to let the
bio completion call aio_complete(). This stops us from returning errors after
O_DIRECT submission.
But we have a few places that are very interested in generating errors after
bio submission.
The most critical of these is invalidating the page cache after a write. This
avoids exposing stale data to buffered operations that are performed after the
O_DIRECT write succeeds. We must do this after submission because the user
buffer might have been an mmap()ed buffer of the region being written to. The
get_user_pages() in the O_DIRECT completion path could have faulted in stale
data.
So this patch introduces a helper, aio_propogate_error(), which queues
post-submission errors in the iocb so that they are given to the user
completion event when aio_complete() is finally called.
To get this working we change the aio_complete() path so that the ring
insertion is performed as the final iocb reference is dropped. This gives the
submission path time to queue its pending error before it drops its reference.
This increases the space in the iocb as it has to record the two result codes
from aio_complete() and the pending error from the submission path.
This was tested by running O_DIRECT aio-stress concurrently with buffered reads
while triggering EIO in invalidate_inode_pages2_range() with the help of a
debugfs bool hack. Previously the kernel would oops as fs/aio.c and bio
completion both called aio_complete(). With this patch aio-stress sees -EIO.
Signed-off-by: Zach Brown <[EMAIL PROTECTED]>
---
fs/aio.c| 49 +-
include/linux/aio.h | 30 +
mm/filemap.c|4 +--
3 files changed, 57 insertions(+), 26 deletions(-)
--- a/fs/aio.c Mon Feb 19 13:12:20 2007 -0800
+++ b/fs/aio.c Mon Feb 19 13:16:00 2007 -0800
@@ -193,8 +193,7 @@ static int aio_setup_ring(struct kioctx
kunmap_atomic((void *)((unsigned long)__event & PAGE_MASK), km); \
} while(0)
-static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb *iocb,
- long res, long res2)
+static void aio_ring_insert_entry(struct kioctx *ctx, struct kiocb *iocb)
{
struct aio_ring_info*info;
struct aio_ring *ring;
@@ -213,12 +212,12 @@ static void aio_ring_insert_entry(struct
event->obj = (u64)(unsigned long)iocb->ki_obj.user;
event->data = iocb->ki_user_data;
- event->res = res;
- event->res2 = res2;
-
- dprintk("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
+ event->res = iocb->ki_pending_err ? iocb->ki_pending_err : iocb->ki_res;
+ event->res2 = iocb->ki_res2;
+
+ dprintk("aio_complete: %p[%lu]: %p: %p %Lx %d %lx %lx\n",
ctx, tail, iocb, iocb->ki_obj.user, iocb->ki_user_data,
- res, res2);
+ iocb->ki_pending_err, iocb->ki_res, iocb->ki_res2);
/* after flagging the request as done, we
* must never even look at it again
@@ -459,6 +458,7 @@ static struct kiocb fastcall *__aio_get_
req->ki_cancel = NULL;
req->ki_retry = NULL;
req->ki_dtor = NULL;
+ req->ki_pending_err = 0;
req->private = NULL;
req->ki_iovec = NULL;
INIT_LIST_HEAD(&req->ki_run_list);
@@ -548,10 +548,14 @@ static int __aio_put_req(struct kioctx *
assert_spin_locked(&ctx->ctx_lock);
- req->ki_users --;
+ req->ki_users--;
BUG_ON(req->ki_users < 0);
if (likely(req->ki_users))
return 0;
+
+ if (kiocbIsInserted(req))
+ aio_ring_insert_entry(ctx, req);
+
list_del(&req->ki_list);/* remove from active_reqs */
req->ki_cancel = NULL;
req->ki_retry = NULL;
@@ -983,27 +987,24 @@ int fastcall aio_complete(struct kiocb *
return 1;
}
- /* add a completion event to the ring buffer.
-* must be done holding ctx->ctx_lock to prevent
-* other code from messing with the tail
-* pointer since we might be called from irq
-* context.
-*/
+ /*
+* We queue up the completion codes into the iocb. They are combined
+* with a potential error from the submission path and inserted into
+* the ring once the last reference to the iocb is dropped. Cancelled
+* iocbs don't insert events on completion because userland was given
+* an event directly as part of the cancelation interface.
+*/
spin_lock_irqsave(&ctx->ctx_lock, flags);
if (ioc
