[RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[Zheng Liu]

From: Zheng Liu <wenqing.lz@taobao.com>

Currently when we issue a dio read at a given offset that is beyond the
end of file we will fallback to buffered read.  Then we check i_size in
buffered read path after we know the page is updated.  But it could
return some zero-filled pages to the userspace when we do some append
dio writes.  We could use the following code snippet to reproduce this
problem in a ext2/3/4 filesystem.

code snippet:
  #define _GNU_SOURCE

  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <memory.h>

  #include <unistd.h>
  #include <fcntl.h>
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <errno.h>

  #include <pthread.h>

  #define BUF_ALIGN	1024

  struct writer_data {
  	int fd;
  	size_t blksize;
  	char *buf;
  };

  static void *writer(void *arg)
  {
  	struct writer_data *data = (struct writer_data *)arg;
  	int ret;

  	ret = write(data->fd, data->buf, data->blksize);
  	if (ret < 0)
  		fprintf(stderr, "write file failed: %s\n", strerror(errno));

  	return NULL;
  }

  int main(int argc, char *argv[])
  {
  	pthread_t tid;
  	struct writer_data wdata;
  	size_t max_blocks = 10 * 1024;
  	size_t blksize = 1 * 1024 * 1024;
  	char *rbuf, *wbuf;
  	int readfd, writefd;
  	int i, j;

  	if (argc < 2) {
  		fprintf(stderr, "usage: %s [filename]\n", argv[0]);
  		exit(1);
  	}

  	writefd = open(argv[1], O_CREAT|O_DIRECT|O_WRONLY|O_APPEND|O_TRUNC, S_IRWXU);
  	if (writefd < 0) {
  		fprintf(stderr, "failed to open wfile: %s\n", strerror(errno));
  		exit(1);
  	}
  	readfd = open(argv[1], O_DIRECT|O_RDONLY, S_IRWXU);
  	if (readfd < 0) {
  		fprintf(stderr, "failed to open rfile: %s\n", strerror(errno));
  		exit(1);
  	}

  	if (posix_memalign((void **)&wbuf, BUF_ALIGN, blksize)) {
  		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
  		exit(1);
  	}

  	if (posix_memalign((void **)&rbuf, 4096, blksize)) {
  		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
  		exit(1);
  	}

  	memset(wbuf, 'a', blksize);

  	wdata.fd = writefd;
  	wdata.blksize = blksize;
  	wdata.buf = wbuf;

  	for (i = 0; i < max_blocks; i++) {
  		void *retval;
  		int ret;

  		ret = pthread_create(&tid, NULL, writer, &wdata);
  		if (ret) {
  			fprintf(stderr, "create thread failed: %s\n", strerror(errno));
  			exit(1);
  		}

  		memset(rbuf, 'b', blksize);
  		do {
  			ret = pread(readfd, rbuf, blksize, i * blksize);
  		} while (ret <= 0);

  		if (ret < 0) {
  			fprintf(stderr, "read file failed: %s\n", strerror(errno));
  			exit(1);
  		}

  		if (pthread_join(tid, &retval)) {
  			fprintf(stderr, "pthread join failed: %s\n", strerror(errno));
  			exit(1);
  		}

  		if (ret >= 0) {
  			for (j = 0; j < ret; j++) {
  				if (rbuf[j] != 'a') {
  					fprintf(stderr, "encounter an error: offset %ld\n",
  						i);
  					goto err;
  				}
  			}
  		}
  	}

  err:
  	free(wbuf);
  	free(rbuf);

  	return 0;
  }

build & run:
  $ gcc code.c -o code -lpthread # build
  $ ./code ${filename} # run

As we expected, we should read nothing or data with 'a'.  But now we
read data with '0'.  I take a closer look at the code and it seems that
there is a bug in vfs.  Let me describe my found here.

  reader					writer
                                                generic_file_aio_write()
                                                ->__generic_file_aio_write()
                                                  ->generic_file_direct_write()
  generic_file_aio_read()
  ->do_generic_file_read()
    [fallback to buffered read]

    ->find_get_page()
    ->page_cache_sync_readahead()
    ->find_get_page()
    [in find_page label, we couldn't find a
     page before and after calling
     page_cache_sync_readahead().  So go to
     no_cached_page label]

    ->page_cache_alloc_cold()
    ->add_to_page_cache_lru()
    [in no_cached_page label, we alloc a page
     and goto readpage label.]

    ->aops->readpage()
    [in readpage label, readpage() callback
     is called and mpage_readpage() return a
     zero-filled page (e.g. ext3/4), and go
     to page_ok label]

                                                  ->a_ops->direct_IO()
                                                  ->i_size_write()
                                                  [we enlarge the i_size]

    Here we check i_size
    [in page_ok label, we check i_size but
     it has been enlarged.  Thus, we pass
     the check and return a zero-filled page]

This commit let dio read return directly if the current offset of the
dio read is beyond the end of file in order to avoid this problem.

Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
 mm/filemap.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 1e6aec4..4179249 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1452,6 +1452,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
 				file_accessed(filp);
 				goto out;
 			}
+		} else {
+			goto out;
 		}
 	}
 
-- 
1.7.9.7

Re: [RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[Jan Kara]

On Thu 21-11-13 16:34:14, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> Currently when we issue a dio read at a given offset that is beyond the
> end of file we will fallback to buffered read.  Then we check i_size in
> buffered read path after we know the page is updated.  But it could
> return some zero-filled pages to the userspace when we do some append
> dio writes.  We could use the following code snippet to reproduce this
> problem in a ext2/3/4 filesystem.
> 
> code snippet:
>   #define _GNU_SOURCE
> 
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <memory.h>
> 
>   #include <unistd.h>
>   #include <fcntl.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <errno.h>
> 
>   #include <pthread.h>
> 
>   #define BUF_ALIGN	1024
> 
>   struct writer_data {
>   	int fd;
>   	size_t blksize;
>   	char *buf;
>   };
> 
>   static void *writer(void *arg)
>   {
>   	struct writer_data *data = (struct writer_data *)arg;
>   	int ret;
> 
>   	ret = write(data->fd, data->buf, data->blksize);
>   	if (ret < 0)
>   		fprintf(stderr, "write file failed: %s\n", strerror(errno));
> 
>   	return NULL;
>   }
> 
>   int main(int argc, char *argv[])
>   {
>   	pthread_t tid;
>   	struct writer_data wdata;
>   	size_t max_blocks = 10 * 1024;
>   	size_t blksize = 1 * 1024 * 1024;
>   	char *rbuf, *wbuf;
>   	int readfd, writefd;
>   	int i, j;
> 
>   	if (argc < 2) {
>   		fprintf(stderr, "usage: %s [filename]\n", argv[0]);
>   		exit(1);
>   	}
> 
>   	writefd = open(argv[1], O_CREAT|O_DIRECT|O_WRONLY|O_APPEND|O_TRUNC, S_IRWXU);
>   	if (writefd < 0) {
>   		fprintf(stderr, "failed to open wfile: %s\n", strerror(errno));
>   		exit(1);
>   	}
>   	readfd = open(argv[1], O_DIRECT|O_RDONLY, S_IRWXU);
>   	if (readfd < 0) {
>   		fprintf(stderr, "failed to open rfile: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	if (posix_memalign((void **)&wbuf, BUF_ALIGN, blksize)) {
>   		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	if (posix_memalign((void **)&rbuf, 4096, blksize)) {
>   		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	memset(wbuf, 'a', blksize);
> 
>   	wdata.fd = writefd;
>   	wdata.blksize = blksize;
>   	wdata.buf = wbuf;
> 
>   	for (i = 0; i < max_blocks; i++) {
>   		void *retval;
>   		int ret;
> 
>   		ret = pthread_create(&tid, NULL, writer, &wdata);
>   		if (ret) {
>   			fprintf(stderr, "create thread failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		memset(rbuf, 'b', blksize);
>   		do {
>   			ret = pread(readfd, rbuf, blksize, i * blksize);
>   		} while (ret <= 0);
> 
>   		if (ret < 0) {
>   			fprintf(stderr, "read file failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		if (pthread_join(tid, &retval)) {
>   			fprintf(stderr, "pthread join failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		if (ret >= 0) {
>   			for (j = 0; j < ret; j++) {
>   				if (rbuf[j] != 'a') {
>   					fprintf(stderr, "encounter an error: offset %ld\n",
>   						i);
>   					goto err;
>   				}
>   			}
>   		}
>   	}
> 
>   err:
>   	free(wbuf);
>   	free(rbuf);
> 
>   	return 0;
>   }
> 
> build & run:
>   $ gcc code.c -o code -lpthread # build
>   $ ./code ${filename} # run
> 
> As we expected, we should read nothing or data with 'a'.  But now we
> read data with '0'.  I take a closer look at the code and it seems that
> there is a bug in vfs.  Let me describe my found here.
> 
>   reader					writer
>                                                 generic_file_aio_write()
>                                                 ->__generic_file_aio_write()
>                                                   ->generic_file_direct_write()
>   generic_file_aio_read()
>   ->do_generic_file_read()
>     [fallback to buffered read]
> 
>     ->find_get_page()
>     ->page_cache_sync_readahead()
>     ->find_get_page()
>     [in find_page label, we couldn't find a
>      page before and after calling
>      page_cache_sync_readahead().  So go to
>      no_cached_page label]
> 
>     ->page_cache_alloc_cold()
>     ->add_to_page_cache_lru()
>     [in no_cached_page label, we alloc a page
>      and goto readpage label.]
> 
>     ->aops->readpage()
>     [in readpage label, readpage() callback
>      is called and mpage_readpage() return a
>      zero-filled page (e.g. ext3/4), and go
>      to page_ok label]
> 
>                                                   ->a_ops->direct_IO()
>                                                   ->i_size_write()
>                                                   [we enlarge the i_size]
> 
>     Here we check i_size
>     [in page_ok label, we check i_size but
>      it has been enlarged.  Thus, we pass
>      the check and return a zero-filled page]
> 
> This commit let dio read return directly if the current offset of the
> dio read is beyond the end of file in order to avoid this problem.
  This would be OK as well but it doesn't solve the case when the reader
uses a normal buffered read, does it? So I think trimming read count as I
described as a reply to your previous patch might be still better.

								Honza

> 
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> ---
>  mm/filemap.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1e6aec4..4179249 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1452,6 +1452,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>  				file_accessed(filp);
>  				goto out;
>  			}
> +		} else {
> +			goto out;
>  		}
>  	}
>  
> -- 
> 1.7.9.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[Zheng Liu]

Hi Jan,

On Thu, Nov 28, 2013 at 12:03:59AM +0100, Jan Kara wrote:
[...]
> > This commit let dio read return directly if the current offset of the
> > dio read is beyond the end of file in order to avoid this problem.
>   This would be OK as well but it doesn't solve the case when the reader
> uses a normal buffered read, does it?

Yup, it doesn't fix that case with buffered read because as Dave's
comment we shouldn't need to ensure that.  So in this commit I just try
to fix the dio case.

> So I think trimming read count as I
> described as a reply to your previous patch might be still better.

I have seen your reply.  Thanks for your suggestion, and I will give it
a try.

                                                - Zheng

> 
> 								Honza
> 
> > 
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > ---
> >  mm/filemap.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 1e6aec4..4179249 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1452,6 +1452,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> >  				file_accessed(filp);
> >  				goto out;
> >  			}
> > +		} else {
> > +			goto out;
> >  		}
> >  	}
> >  
> > -- 
> > 1.7.9.7
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR

Re: [RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[Andrew Morton]

(cc linux-mm)

It would be good if we could get some more eyes onto this please.

On Thu, 21 Nov 2013 16:34:14 +0800 Zheng Liu <gnehzuil.liu@gmail.com> wrote:

> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> Currently when we issue a dio read at a given offset that is beyond the
> end of file we will fallback to buffered read.  Then we check i_size in
> buffered read path after we know the page is updated.  But it could
> return some zero-filled pages to the userspace when we do some append
> dio writes.  We could use the following code snippet to reproduce this
> problem in a ext2/3/4 filesystem.
> 
> code snippet:
>   #define _GNU_SOURCE
> 
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <memory.h>
> 
>   #include <unistd.h>
>   #include <fcntl.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <errno.h>
> 
>   #include <pthread.h>
> 
>   #define BUF_ALIGN	1024
> 
>   struct writer_data {
>   	int fd;
>   	size_t blksize;
>   	char *buf;
>   };
> 
>   static void *writer(void *arg)
>   {
>   	struct writer_data *data = (struct writer_data *)arg;
>   	int ret;
> 
>   	ret = write(data->fd, data->buf, data->blksize);
>   	if (ret < 0)
>   		fprintf(stderr, "write file failed: %s\n", strerror(errno));
> 
>   	return NULL;
>   }
> 
>   int main(int argc, char *argv[])
>   {
>   	pthread_t tid;
>   	struct writer_data wdata;
>   	size_t max_blocks = 10 * 1024;
>   	size_t blksize = 1 * 1024 * 1024;
>   	char *rbuf, *wbuf;
>   	int readfd, writefd;
>   	int i, j;
> 
>   	if (argc < 2) {
>   		fprintf(stderr, "usage: %s [filename]\n", argv[0]);
>   		exit(1);
>   	}
> 
>   	writefd = open(argv[1], O_CREAT|O_DIRECT|O_WRONLY|O_APPEND|O_TRUNC, S_IRWXU);
>   	if (writefd < 0) {
>   		fprintf(stderr, "failed to open wfile: %s\n", strerror(errno));
>   		exit(1);
>   	}
>   	readfd = open(argv[1], O_DIRECT|O_RDONLY, S_IRWXU);
>   	if (readfd < 0) {
>   		fprintf(stderr, "failed to open rfile: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	if (posix_memalign((void **)&wbuf, BUF_ALIGN, blksize)) {
>   		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	if (posix_memalign((void **)&rbuf, 4096, blksize)) {
>   		fprintf(stderr, "failed to alloc memory: %s\n", strerror(errno));
>   		exit(1);
>   	}
> 
>   	memset(wbuf, 'a', blksize);
> 
>   	wdata.fd = writefd;
>   	wdata.blksize = blksize;
>   	wdata.buf = wbuf;
> 
>   	for (i = 0; i < max_blocks; i++) {
>   		void *retval;
>   		int ret;
> 
>   		ret = pthread_create(&tid, NULL, writer, &wdata);
>   		if (ret) {
>   			fprintf(stderr, "create thread failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		memset(rbuf, 'b', blksize);
>   		do {
>   			ret = pread(readfd, rbuf, blksize, i * blksize);
>   		} while (ret <= 0);
> 
>   		if (ret < 0) {
>   			fprintf(stderr, "read file failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		if (pthread_join(tid, &retval)) {
>   			fprintf(stderr, "pthread join failed: %s\n", strerror(errno));
>   			exit(1);
>   		}
> 
>   		if (ret >= 0) {
>   			for (j = 0; j < ret; j++) {
>   				if (rbuf[j] != 'a') {
>   					fprintf(stderr, "encounter an error: offset %ld\n",
>   						i);
>   					goto err;
>   				}
>   			}
>   		}
>   	}
> 
>   err:
>   	free(wbuf);
>   	free(rbuf);
> 
>   	return 0;
>   }
> 
> build & run:
>   $ gcc code.c -o code -lpthread # build
>   $ ./code ${filename} # run
> 
> As we expected, we should read nothing or data with 'a'.  But now we
> read data with '0'.  I take a closer look at the code and it seems that
> there is a bug in vfs.  Let me describe my found here.
> 
>   reader					writer
>                                                 generic_file_aio_write()
>                                                 ->__generic_file_aio_write()
>                                                   ->generic_file_direct_write()
>   generic_file_aio_read()
>   ->do_generic_file_read()
>     [fallback to buffered read]
> 
>     ->find_get_page()
>     ->page_cache_sync_readahead()
>     ->find_get_page()
>     [in find_page label, we couldn't find a
>      page before and after calling
>      page_cache_sync_readahead().  So go to
>      no_cached_page label]

It's odd that do_generic_file_read() is permitting a "read" outside
i_size.  Perhaps we should be checking for this in the `no_cached_page'
block.

>     ->page_cache_alloc_cold()
>     ->add_to_page_cache_lru()
>     [in no_cached_page label, we alloc a page
>      and goto readpage label.]
> 
>     ->aops->readpage()
>     [in readpage label, readpage() callback
>      is called and mpage_readpage() return a
>      zero-filled page (e.g. ext3/4), and go
>      to page_ok label]
> 
>                                                   ->a_ops->direct_IO()
>                                                   ->i_size_write()
>                                                   [we enlarge the i_size]
> 
>     Here we check i_size
>     [in page_ok label, we check i_size but
>      it has been enlarged.  Thus, we pass
>      the check and return a zero-filled page]

OK, so it's a race.

> This commit let dio read return directly if the current offset of the
> dio read is beyond the end of file in order to avoid this problem.
> 
> ...
>
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1452,6 +1452,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
>  				file_accessed(filp);
>  				goto out;
>  			}
> +		} else {
> +			goto out;
>  		}
>  	}

OK, so we don't fall back to buffered reading at all if we're outside
i_size.

I'm not sure this 100% fixes the problem.  In generic_file_aio_read():

: 		if (pos < size) {

write() extends i_size now.

: 			retval = filemap_write_and_wait_range(mapping, pos,
: 					pos + iov_length(iov, nr_segs) - 1);
: 			if (!retval) {
: 				retval = mapping->a_ops->direct_IO(READ, iocb,
: 							iov, pos, nr_segs);
: 			}
: 			if (retval > 0) {
: 				*ppos = pos + retval;
: 				count -= retval;
: 			}
: 
: 			/*
: 			 * Btrfs can have a short DIO read if we encounter
: 			 * compressed extents, so if there was an error, or if
: 			 * we've already read everything we wanted to, or if
: 			 * there was a short read because we hit EOF, go ahead
: 			 * and return.  Otherwise fallthrough to buffered io for
: 			 * the rest of the read.
: 			 */
: 			if (retval < 0 || !count || *ppos >= size) {
: 				file_accessed(filp);
: 				goto out;
: 			}

we can still fall through to buffered read.

: 		} else {
: 			goto out;
: 		}
: 	}


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC][PATCH] vfs: don't fallback to buffered read if the offset of dio read is beyond eof

[Zheng Liu]

Hi Andrew,

Thanks for reviewing this patch.  This patch has been updated.  The
following links are the v2 [1] and v3 [2].  In v2 I use a new approach
to fix this bug, and in v3 only change is to add a 'Reviewed-by' tag.

1. http://permalink.gmane.org/gmane.linux.file-systems/80327
2. http://www.spinics.net/lists/linux-fsdevel/msg70899.html

On Sun, Dec 15, 2013 at 11:21:32PM -0800, Andrew Morton wrote:
[...]
> > As we expected, we should read nothing or data with 'a'.  But now we
> > read data with '0'.  I take a closer look at the code and it seems that
> > there is a bug in vfs.  Let me describe my found here.
> > 
> >   reader					writer
> >                                                 generic_file_aio_write()
> >                                                 ->__generic_file_aio_write()
> >                                                   ->generic_file_direct_write()
> >   generic_file_aio_read()
> >   ->do_generic_file_read()
> >     [fallback to buffered read]
> > 
> >     ->find_get_page()
> >     ->page_cache_sync_readahead()
> >     ->find_get_page()
> >     [in find_page label, we couldn't find a
> >      page before and after calling
> >      page_cache_sync_readahead().  So go to
> >      no_cached_page label]
> 
> It's odd that do_generic_file_read() is permitting a "read" outside
> i_size.  Perhaps we should be checking for this in the `no_cached_page'
> block.

In v2 I check i_size at the beginning of do_generic_file_read() to avoid
permitting a read outside i_size.

> 
> >     ->page_cache_alloc_cold()
> >     ->add_to_page_cache_lru()
> >     [in no_cached_page label, we alloc a page
> >      and goto readpage label.]
> > 
> >     ->aops->readpage()
> >     [in readpage label, readpage() callback
> >      is called and mpage_readpage() return a
> >      zero-filled page (e.g. ext3/4), and go
> >      to page_ok label]
> > 
> >                                                   ->a_ops->direct_IO()
> >                                                   ->i_size_write()
> >                                                   [we enlarge the i_size]
> > 
> >     Here we check i_size
> >     [in page_ok label, we check i_size but
> >      it has been enlarged.  Thus, we pass
> >      the check and return a zero-filled page]
> 
> OK, so it's a race.
> 
> > This commit let dio read return directly if the current offset of the
> > dio read is beyond the end of file in order to avoid this problem.
> > 
> > ...
> >
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1452,6 +1452,8 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
> >  				file_accessed(filp);
> >  				goto out;
> >  			}
> > +		} else {
> > +			goto out;
> >  		}
> >  	}
> 
> OK, so we don't fall back to buffered reading at all if we're outside
> i_size.
> 
> I'm not sure this 100% fixes the problem.  In generic_file_aio_read():
> 
> : 		if (pos < size) {
> 
> write() extends i_size now.

Uhh, I don't think so.  If write() extends i_size here, we will read
something after calling ->direct_IO().  So '*ppos' should be equal to
'size (old i_size)', and we will goto 'out' label.

> 
> : 			retval = filemap_write_and_wait_range(mapping, pos,
> : 					pos + iov_length(iov, nr_segs) - 1);
> : 			if (!retval) {
> : 				retval = mapping->a_ops->direct_IO(READ, iocb,
> : 							iov, pos, nr_segs);
> : 			}
> : 			if (retval > 0) {
> : 				*ppos = pos + retval;
> : 				count -= retval;
> : 			}
> : 
> : 			/*
> : 			 * Btrfs can have a short DIO read if we encounter
> : 			 * compressed extents, so if there was an error, or if
> : 			 * we've already read everything we wanted to, or if
> : 			 * there was a short read because we hit EOF, go ahead
> : 			 * and return.  Otherwise fallthrough to buffered io for
> : 			 * the rest of the read.
> : 			 */
> : 			if (retval < 0 || !count || *ppos >= size) {

We will goto 'out' label here.

Thanks,
                                                - Zheng

> : 				file_accessed(filp);
> : 				goto out;
> : 			}
> 
> we can still fall through to buffered read.
> 
> : 		} else {
> : 			goto out;
> : 		}
> : 	}
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>