[PATCH] Direct IO async short read bug followup

[PATCH] Direct IO async short read bug followup

[Daniel McNeil]

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.

Re: [PATCH] Direct IO async short read bug followup

[Andrew Morton]

Daniel McNeil <daniel@osdl.org> 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?

Re: [PATCH] Direct IO async short read bug followup

[Andrew Morton]

Andrew Morton <akpm@osdl.org> 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?

Re: [PATCH] Direct IO async short read bug followup

[Daniel McNeil]

On Fri, 2005-04-01 at 17:27, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> 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

Re: [PATCH] Direct IO async short read bug followup

[Daniel McNeil]

On Fri, 2005-04-01 at 17:20, Andrew Morton wrote:
> Daniel McNeil <daniel@osdl.org> 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