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-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
