On Mon, Mar 07, 2005 at 10:39:17PM -0800, Andrew Morton wrote:
> 6
> Lines: 76
>
> Sébastien Dugué <[EMAIL PROTECTED]> wrote:
> >
> > When reading a file in async mode (using kernel AIO), and the file
> > size is lower than the requested size (short read), the direct IO
> > layer reports an incorrect number of bytes read (transferred).
> >
> > That case is handled for the sync path in 'direct_io_worker'
> > (fs/direct-io.c) where a check is made against the file size.
> >
> > This patch does the same thing for the async path.
>
> It looks sane to me. It needs a couple of fixes, below. One of them is
> horrid and isn't really a fix at all, but it improves things.
>
> Would Suparna and Badari have time to check the logic of these two patches
> please?
>
Bugs in this area seem never-ending don't they - plug one, open up
another - hard to be confident/verify :( - someday we'll have to
rewrite a part of this code.
Hmm, shouldn't dio->result ideally have been adjusted to be within
i_size at the time of io submission, so we don't have to deal with
this during completion ? We are creating bios with the right size
after all.
We have this:
if (!buffer_mapped(map_bh)) {
....
if (dio->block_in_file >=
i_size_read(dio->inode)>>blkbits) {
/* We hit eof */
page_cache_release(page);
goto out;
}
and
dio->result += iov[seg].iov_len -
((dio->final_block_in_request - dio->block_in_file) <<
blkbits);
can you spot what is going wrong here that we have to try and
workaround this later ?
Regards
Suparna
>
>
>
> - i_size is 64 bit, ssize_t is 32-bit
>
> - whitespace tweaks.
>
> - i_size_read() in interrupt context is a no-no.
>
> Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
> ---
>
> 25-akpm/fs/direct-io.c | 14 +++++++++++---
> 1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff -puN fs/direct-io.c~direct-io-async-short-read-fix-fix fs/direct-io.c
> --- 25/fs/direct-io.c~direct-io-async-short-read-fix-fix 2005-03-07
> 22:28:52.000000000 -0800
> +++ 25-akpm/fs/direct-io.c 2005-03-07 22:37:18.000000000 -0800
> @@ -231,7 +231,7 @@ static void finished_one_bio(struct dio
> if (dio->bio_count == 1) {
> if (dio->is_async) {
> ssize_t transferred;
> - ssize_t i_size;
> + loff_t i_size;
> loff_t offset;
>
> /*
> @@ -241,11 +241,19 @@ static void finished_one_bio(struct dio
> spin_unlock_irqrestore(&dio->bio_lock, flags);
>
> /* Check for short read case */
> +
> + /*
> + * We should use i_size_read() here. But we're called
> + * in interrupt context. If this CPU happened to be
> + * in the middle of i_size_write() when the interrupt
> + * occurred, i_size_read() would lock up.
> + * So we just risk getting a wrong result instead :(
> + */
> + i_size = dio->inode->i_size;
> transferred = dio->result;
> - i_size = i_size_read (dio->inode);
> offset = dio->iocb->ki_pos;
>
> - if ((dio->rw == READ) && ((offset + transferred) >
> i_size))
> + if ((dio->rw == READ) && (offset+transferred > i_size))
> transferred = i_size - offset;
>
> dio_complete(dio, offset, transferred);
> _
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to [EMAIL PROTECTED] For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"[EMAIL PROTECTED]">[EMAIL PROTECTED]
--
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/
