Re: [FSAIO][PATCH 7/8] Filesystem AIO read
On Thu, Dec 28, 2006 at 08:48:30PM +0530, Suparna Bhattacharya wrote: > Yes, we can do that -- how about aio_restarted() as an alternate name ? Sounds fine to me. > > Pluse possible naming updates discussed in the last mail. Also do we > > really need to pass current->io_wait here? Isn't the waitqueue in > > the kiocb always guaranteed to be the same? Now that all pagecache > > We don't have have the kiocb available to this routine. Using current->io_wait > avoids the need to pass the iocb down to deeper levels just for the sync vs > async checks, also allowing such routines to be shared by other code which > does not use iocbs (e.g. generic_file_sendfile->do_generic_file_read > ->do_generic_mapping_read) without having to set up dummy iocbs. We really want to switch senfile to kiocbs btw, - for one thing to allow an aio_sendfile implementation and second to make it more common to all the other I/O path code so we can avoid special cases in the fs code So I'm not convinced by that argument. But again we don't need to put the io_wait removal into your patchkit. I'll try to hack on it once I'll get a little spare time. - 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: [FSAIO][PATCH 7/8] Filesystem AIO read
On Thu, 28 Dec 2006 17:22:07 +0100 (MET) Jan Engelhardt wrote:
>
> On Dec 28 2006 11:57, Christoph Hellwig wrote:
> >
> >> +
> >> + if ((error = __lock_page(page, current->io_wait))) {
> >> + goto readpage_error;
> >> + }
> >
> >This should be
> >
> > error = __lock_page(page, current->io_wait);
> > if (error)
> > goto readpage_error;
>
> That's effectively the same. Essentially a style thing, and I see if((err =
> xyz)) not being uncommon in the kernel tree.
The combined if/assignment has been known to contain coding errors
that are legal C, just not what was intended.
Since the latter replacement shouldn't cause any code efficiency
problems and it's more readable, it's becoming preferred.
---
~Randy
-
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: [FSAIO][PATCH 7/8] Filesystem AIO read
On Dec 28 2006 11:57, Christoph Hellwig wrote:
>
>> +
>> +if ((error = __lock_page(page, current->io_wait))) {
>> +goto readpage_error;
>> +}
>
>This should be
>
> error = __lock_page(page, current->io_wait);
> if (error)
> goto readpage_error;
That's effectively the same. Essentially a style thing, and I see if((err =
xyz)) not being uncommon in the kernel tree.
-`J'
--
-
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: [FSAIO][PATCH 7/8] Filesystem AIO read
On Thu, Dec 28, 2006 at 11:57:47AM +, Christoph Hellwig wrote:
> > + if (in_aio()) {
> > + /* Avoid repeat readahead */
> > + if (kiocbTryRestart(io_wait_to_kiocb(current->io_wait)))
> > + next_index = last_index;
> > + }
>
> Every place we use kiocbTryRestart in this and the next patch it's in
> this from, so we should add a little helper for it:
>
> int aio_try_restart(void)
> {
> struct wait_queue_head_t *wq = current->io_wait;
>
> if (!is_sync_wait(wq) && kiocbTryRestart(io_wait_to_kiocb(wq)))
> return 1;
> return 0;
> }
Yes, we can do that -- how about aio_restarted() as an alternate name ?
>
> with a big kerneldoc comment explaining this idiom (and possible a better
> name for the function ;-))
>
> > +
> > + if ((error = __lock_page(page, current->io_wait))) {
> > + goto readpage_error;
> > + }
>
> This should be
>
> error = __lock_page(page, current->io_wait);
> if (error)
> goto readpage_error;
>
> Pluse possible naming updates discussed in the last mail. Also do we
> really need to pass current->io_wait here? Isn't the waitqueue in
> the kiocb always guaranteed to be the same? Now that all pagecache
We don't have have the kiocb available to this routine. Using current->io_wait
avoids the need to pass the iocb down to deeper levels just for the sync vs
async checks, also allowing such routines to be shared by other code which
does not use iocbs (e.g. generic_file_sendfile->do_generic_file_read
->do_generic_mapping_read) without having to set up dummy iocbs.
Does that clarify ? We could abstract this away within a lock page wrapper,
but I don't know if that makes a difference.
> I/O goes through the ->aio_read/->aio_write routines I'd prefer to
> get rid of the task_struct field cludges and pass all this around in
> the kiocb.
Regards
Suparna
--
Suparna Bhattacharya ([EMAIL PROTECTED])
Linux Technology Center
IBM Software Lab, India
-
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: [FSAIO][PATCH 7/8] Filesystem AIO read
> Pluse possible naming updates discussed in the last mail. Also do we > really need to pass current->io_wait here? Isn't the waitqueue in > the kiocb always guaranteed to be the same? Now that all pagecache > I/O goes through the ->aio_read/->aio_write routines I'd prefer to > get rid of the task_struct field cludges and pass all this around in > the kiocb. Btw, in current mainline task_struct.iowait is not used at all! The patch below would remove it vs mainline, although I don't think it should go in as-is as it would create quite a bit of messup for your patchset. - 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: [FSAIO][PATCH 7/8] Filesystem AIO read
> + if (in_aio()) {
> + /* Avoid repeat readahead */
> + if (kiocbTryRestart(io_wait_to_kiocb(current->io_wait)))
> + next_index = last_index;
> + }
Every place we use kiocbTryRestart in this and the next patch it's in
this from, so we should add a little helper for it:
int aio_try_restart(void)
{
struct wait_queue_head_t *wq = current->io_wait;
if (!is_sync_wait(wq) && kiocbTryRestart(io_wait_to_kiocb(wq)))
return 1;
return 0;
}
with a big kerneldoc comment explaining this idiom (and possible a better
name for the function ;-))
> +
> + if ((error = __lock_page(page, current->io_wait))) {
> + goto readpage_error;
> + }
This should be
error = __lock_page(page, current->io_wait);
if (error)
goto readpage_error;
Pluse possible naming updates discussed in the last mail. Also do we
really need to pass current->io_wait here? Isn't the waitqueue in
the kiocb always guaranteed to be the same? Now that all pagecache
I/O goes through the ->aio_read/->aio_write routines I'd prefer to
get rid of the task_struct field cludges and pass all this around in
the kiocb.
-
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/
[FSAIO][PATCH 7/8] Filesystem AIO read
Converts the wait for page to become uptodate (lock page)
after readahead/readpage (in do_generic_mapping_read) to a retry
exit, to make buffered filesystem AIO reads actually synchronous.
The patch avoids exclusive wakeups with AIO, a problem originally
spotted by Chris Mason, though the reasoning for why it is an
issue is now much clearer (see explanation in the comment below
in aio.c), and the solution is perhaps slightly simpler.
Signed-off-by: Suparna Bhattacharya <[EMAIL PROTECTED]>
Acked-by: Ingo Molnar <[EMAIL PROTECTED]>
---
linux-2.6.20-rc1-root/fs/aio.c| 11 ++-
linux-2.6.20-rc1-root/include/linux/aio.h |5 +
linux-2.6.20-rc1-root/mm/filemap.c| 19 ---
3 files changed, 31 insertions(+), 4 deletions(-)
diff -puN fs/aio.c~aio-fs-read fs/aio.c
--- linux-2.6.20-rc1/fs/aio.c~aio-fs-read 2006-12-21 08:46:13.0
+0530
+++ linux-2.6.20-rc1-root/fs/aio.c 2006-12-28 09:26:30.0 +0530
@@ -1529,7 +1529,16 @@ static int aio_wake_function(wait_queue_
list_del_init(&wait->task_list);
kick_iocb(iocb);
- return 1;
+ /*
+* Avoid exclusive wakeups with retries since an exclusive wakeup
+* may involve implicit expectations of waking up the next waiter
+* and there is no guarantee that the retry will take a path that
+* would do so. For example if a page has become up-to-date, then
+* a retried read may end up straightaway performing a copyout
+* and not go through a lock_page - unlock_page that would have
+* passed the baton to the next waiter.
+*/
+ return 0;
}
int fastcall io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
diff -puN mm/filemap.c~aio-fs-read mm/filemap.c
--- linux-2.6.20-rc1/mm/filemap.c~aio-fs-read 2006-12-21 08:46:13.0
+0530
+++ linux-2.6.20-rc1-root/mm/filemap.c 2006-12-28 09:31:48.0 +0530
@@ -909,6 +909,11 @@ void do_generic_mapping_read(struct addr
if (!isize)
goto out;
+ if (in_aio()) {
+ /* Avoid repeat readahead */
+ if (kiocbTryRestart(io_wait_to_kiocb(current->io_wait)))
+ next_index = last_index;
+ }
end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
for (;;) {
struct page *page;
@@ -978,7 +983,10 @@ page_ok:
page_not_up_to_date:
/* Get exclusive access to the page ... */
- lock_page(page);
+
+ if ((error = __lock_page(page, current->io_wait))) {
+ goto readpage_error;
+ }
/* Did it get truncated before we got the lock? */
if (!page->mapping) {
@@ -1006,7 +1014,8 @@ readpage:
}
if (!PageUptodate(page)) {
- lock_page(page);
+ if ((error = __lock_page(page, current->io_wait)))
+ goto readpage_error;
if (!PageUptodate(page)) {
if (page->mapping == NULL) {
/*
@@ -1052,7 +1061,11 @@ readpage:
goto page_ok;
readpage_error:
- /* UHHUH! A synchronous read error occurred. Report it */
+ /* We don't have uptodate data in the page yet */
+ /* Could be due to an error or because we need to
+* retry when we get an async i/o notification.
+* Report the reason.
+*/
desc->error = error;
page_cache_release(page);
goto out;
diff -puN include/linux/aio.h~aio-fs-read include/linux/aio.h
--- linux-2.6.20-rc1/include/linux/aio.h~aio-fs-read2006-12-21
08:46:13.0 +0530
+++ linux-2.6.20-rc1-root/include/linux/aio.h 2006-12-28 09:26:27.0
+0530
@@ -34,21 +34,26 @@ struct kioctx;
/* #define KIF_LOCKED 0 */
#define KIF_KICKED 1
#define KIF_CANCELLED 2
+#define KIF_RESTARTED 3
#define kiocbTryLock(iocb) test_and_set_bit(KIF_LOCKED, &(iocb)->ki_flags)
#define kiocbTryKick(iocb) test_and_set_bit(KIF_KICKED, &(iocb)->ki_flags)
+#define kiocbTryRestart(iocb) test_and_set_bit(KIF_RESTARTED,
&(iocb)->ki_flags)
#define kiocbSetLocked(iocb) set_bit(KIF_LOCKED, &(iocb)->ki_flags)
#define kiocbSetKicked(iocb) set_bit(KIF_KICKED, &(iocb)->ki_flags)
#define kiocbSetCancelled(iocb)set_bit(KIF_CANCELLED,
&(iocb)->ki_flags)
+#define kiocbSetRestarted(iocb)set_bit(KIF_RESTARTED,
&(iocb)->ki_flags)
#define kiocbClearLocked(iocb) clear_bit(KIF_LOCKED, &(iocb)->ki_flags)
#define kiocbClearKicked(iocb) clear_bit(KIF_KICKED, &(iocb)->ki_flags)
#define kiocbClearCancelled(iocb) clear_bit(KIF_CANCELLED,
&(iocb)->ki_flags)
+#define kiocbClearRestarted(iocb) clear_bit(KIF_RESTARTED,
&(iocb)->ki_flags)
#define kiocb
