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 from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
