On Fri, 2005-03-25 at 06:59, Badari Pulavarty wrote:
> Andrew,
>
> When I debugged the problem, the issue seems to be only for the last
> block of the file. Filesize is not multiple of 4K blocks. (say 17K).
> So, on the disk we have a 4K block for the last block. The test is
> trying to read 20K. Since we have a block on the disk, get_block()
> won't complain and we do the IO. Once the IO is done, we can truncate
> the result to match the filesize.
>
> I tried fixing the problem by limiting the IO submits to the size of
> the file - which became really ugly (since I have to adjust the
> iovec[]).
>
> Daniel McNeil wanted to take a stab at it. Dan what happend to the fix ?
>
> Thanks,
> Badari
I updated the patch to add an i_size element to the dio structure and
sample i_size during i/o submission. When i/o completes the result can
be truncated to match the file size without using i_size_read(), thus
the aio result now matches the number of bytes read to the end of file.
Here's the patch. It applies to 2.6.11 and the latest bk.
Daniel
--- linux-2.6.11.orig/fs/direct-io.c 2005-04-01 15:33:11.000000000 -0800
+++ linux-2.6.11/fs/direct-io.c 2005-03-31 16:59:15.000000000 -0800
@@ -66,6 +66,7 @@ struct dio {
struct bio *bio; /* bio under assembly */
struct inode *inode;
int rw;
+ ssize_t i_size; /* i_size when submitted */
int lock_type; /* doesn't change */
unsigned blkbits; /* doesn't change */
unsigned blkfactor; /* When we're using an alignment which
@@ -230,17 +231,29 @@ static void finished_one_bio(struct dio
spin_lock_irqsave(&dio->bio_lock, flags);
if (dio->bio_count == 1) {
if (dio->is_async) {
+ ssize_t transferred;
+ loff_t offset;
+
/*
* Last reference to the dio is going away.
* Drop spinlock and complete the DIO.
*/
spin_unlock_irqrestore(&dio->bio_lock, flags);
- dio_complete(dio, dio->block_in_file << dio->blkbits,
- dio->result);
+
+ /* Check for short read case */
+ transferred = dio->result;
+ offset = dio->iocb->ki_pos;
+
+ if ((dio->rw == READ) &&
+ ((offset + transferred) > dio->i_size))
+ transferred = dio->i_size - offset;
+
+ dio_complete(dio, offset, transferred);
+
/* Complete AIO later if falling back to buffered i/o */
if (dio->result == dio->size ||
((dio->rw == READ) && dio->result)) {
- aio_complete(dio->iocb, dio->result, 0);
+ aio_complete(dio->iocb, transferred, 0);
kfree(dio);
return;
} else {
@@ -951,6 +964,7 @@ direct_io_worker(int rw, struct kiocb *i
dio->page_errors = 0;
dio->result = 0;
dio->iocb = iocb;
+ dio->i_size = i_size_read(inode);
/*
* BIO completion state.
Daniel McNeil <[email protected]> wrote:
>
> I updated the patch to add an i_size element to the dio structure and
> sample i_size during i/o submission. When i/o completes the result can
> be truncated to match the file size without using i_size_read(), thus
> the aio result now matches the number of bytes read to the end of file.
>
Can you provide the analysis of the bug, please?
>
> Daniel
>
> --- linux-2.6.11.orig/fs/direct-io.c 2005-04-01 15:33:11.000000000 -0800
> +++ linux-2.6.11/fs/direct-io.c 2005-03-31 16:59:15.000000000 -0800
> @@ -66,6 +66,7 @@ struct dio {
> struct bio *bio; /* bio under assembly */
> struct inode *inode;
> int rw;
> + ssize_t i_size; /* i_size when submitted */
I'll change this to loff_t, OK?
Andrew Morton <[email protected]> wrote:
>
> > --- linux-2.6.11.orig/fs/direct-io.c 2005-04-01 15:33:11.000000000 -0800
> > +++ linux-2.6.11/fs/direct-io.c 2005-03-31 16:59:15.000000000 -0800
> > @@ -66,6 +66,7 @@ struct dio {
> > struct bio *bio; /* bio under assembly */
> > struct inode *inode;
> > int rw;
> > + ssize_t i_size; /* i_size when submitted */
>
> I'll change this to loff_t, OK?
And I think local variable `transferred' can remain ssize_t. Agree?
On Fri, 2005-04-01 at 17:20, Andrew Morton wrote:
> Daniel McNeil <[email protected]> wrote:
> >
> > I updated the patch to add an i_size element to the dio structure and
> > sample i_size during i/o submission. When i/o completes the result can
> > be truncated to match the file size without using i_size_read(), thus
> > the aio result now matches the number of bytes read to the end of file.
> >
>
> Can you provide the analysis of the bug, please?
The direct i/o code is mapping the read request to the file system
block. If the file size was not on a block boundary, the result
would show the the read reading past EOF. This was only happening
for the AIO case. The non-AIO case truncates the result to match
file size (in direct_io_worker). This patch does the same thing
for the AIO case, it truncates the result to match the file size
if the read reads past EOF.
>
> >
> > Daniel
> >
> > --- linux-2.6.11.orig/fs/direct-io.c 2005-04-01 15:33:11.000000000 -0800
> > +++ linux-2.6.11/fs/direct-io.c 2005-03-31 16:59:15.000000000 -0800
> > @@ -66,6 +66,7 @@ struct dio {
> > struct bio *bio; /* bio under assembly */
> > struct inode *inode;
> > int rw;
> > + ssize_t i_size; /* i_size when submitted */
>
> I'll change this to loff_t, OK?
> -
Yes.
Daniel
On Fri, 2005-04-01 at 17:27, Andrew Morton wrote:
> Andrew Morton <[email protected]> wrote:
> >
> > > --- linux-2.6.11.orig/fs/direct-io.c 2005-04-01 15:33:11.000000000 -0800
> > > +++ linux-2.6.11/fs/direct-io.c 2005-03-31 16:59:15.000000000 -0800
> > > @@ -66,6 +66,7 @@ struct dio {
> > > struct bio *bio; /* bio under assembly */
> > > struct inode *inode;
> > > int rw;
> > > + ssize_t i_size; /* i_size when submitted */
> >
> > I'll change this to loff_t, OK?
>
> And I think local variable `transferred' can remain ssize_t. Agree?
Yes.
Daniel