Re: [PATCH 1/2] fs: Break generic_file_buffered_read up into multiple functions
On 10/17/20 2:10 PM, Kent Overstreet wrote: > This is prep work for changing generic_file_buffered_read() to use > find_get_pages_contig() to batch up all the pagecache lookups. > > This patch should be functionally identical to the existing code and > changes as little as of the flow control as possible. More refactoring > could be done, this patch is intended to be relatively minimal. This is a sorely needed cleanup. Reviewed-by: Jens Axboe -- Jens Axboe
[PATCH 1/2] fs: Break generic_file_buffered_read up into multiple functions
This is prep work for changing generic_file_buffered_read() to use
find_get_pages_contig() to batch up all the pagecache lookups.
This patch should be functionally identical to the existing code and
changes as little as of the flow control as possible. More refactoring
could be done, this patch is intended to be relatively minimal.
Signed-off-by: Kent Overstreet
Cc: Matthew Wilcox (Oracle)
Cc: Jens Axboe
---
mm/filemap.c | 473 ---
1 file changed, 261 insertions(+), 212 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 99c49eeae7..482fd75d66 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2138,6 +2138,234 @@ static void shrink_readahead_size_eio(struct
file_ra_state *ra)
ra->ra_pages /= 4;
}
+static int lock_page_for_iocb(struct kiocb *iocb, struct page *page)
+{
+ if (iocb->ki_flags & IOCB_WAITQ)
+ return lock_page_async(page, iocb->ki_waitq);
+ else if (iocb->ki_flags & IOCB_NOWAIT)
+ return trylock_page(page) ? 0 : -EAGAIN;
+ else
+ return lock_page_killable(page);
+}
+
+static int generic_file_buffered_read_page_ok(struct kiocb *iocb,
+ struct iov_iter *iter,
+ struct page *page)
+{
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
+ struct inode *inode = mapping->host;
+ struct file_ra_state *ra = >ki_filp->f_ra;
+ unsigned int offset = iocb->ki_pos & ~PAGE_MASK;
+ unsigned int bytes, copied;
+ loff_t isize, end_offset;
+
+ BUG_ON(iocb->ki_pos >> PAGE_SHIFT != page->index);
+
+ /*
+* i_size must be checked after we know the page is Uptodate.
+*
+* Checking i_size after the check allows us to calculate
+* the correct value for "bytes", which means the zero-filled
+* part of the page is not copied back to userspace (unless
+* another truncate extends the file - this is desired though).
+*/
+
+ isize = i_size_read(inode);
+ if (unlikely(iocb->ki_pos >= isize))
+ return 1;
+
+ end_offset = min_t(loff_t, isize, iocb->ki_pos + iter->count);
+
+ bytes = min_t(loff_t, end_offset - iocb->ki_pos, PAGE_SIZE - offset);
+
+ /* If users can be writing to this page using arbitrary
+* virtual addresses, take care about potential aliasing
+* before reading the page on the kernel side.
+*/
+ if (mapping_writably_mapped(mapping))
+ flush_dcache_page(page);
+
+ /*
+* Ok, we have the page, and it's up-to-date, so
+* now we can copy it to user space...
+*/
+
+ copied = copy_page_to_iter(page, offset, bytes, iter);
+
+ iocb->ki_pos += copied;
+
+ /*
+* When a sequential read accesses a page several times,
+* only mark it as accessed the first time.
+*/
+ if (iocb->ki_pos >> PAGE_SHIFT != ra->prev_pos >> PAGE_SHIFT)
+ mark_page_accessed(page);
+
+ ra->prev_pos = iocb->ki_pos;
+
+ if (copied < bytes)
+ return -EFAULT;
+
+ return !iov_iter_count(iter) || iocb->ki_pos == isize;
+}
+
+static struct page *
+generic_file_buffered_read_readpage(struct kiocb *iocb,
+ struct file *filp,
+ struct address_space *mapping,
+ struct page *page)
+{
+ struct file_ra_state *ra = >f_ra;
+ int error;
+
+ if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT)) {
+ unlock_page(page);
+ put_page(page);
+ return ERR_PTR(-EAGAIN);
+ }
+
+ /*
+* A previous I/O error may have been due to temporary
+* failures, eg. multipath errors.
+* PG_error will be set again if readpage fails.
+*/
+ ClearPageError(page);
+ /* Start the actual read. The read will unlock the page. */
+ error = mapping->a_ops->readpage(filp, page);
+
+ if (unlikely(error)) {
+ put_page(page);
+ return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
+ }
+
+ if (!PageUptodate(page)) {
+ error = lock_page_for_iocb(iocb, page);
+ if (unlikely(error)) {
+ put_page(page);
+ return ERR_PTR(error);
+ }
+ if (!PageUptodate(page)) {
+ if (page->mapping == NULL) {
+ /*
+* invalidate_mapping_pages got it
+*/
+ unlock_page(page);
+ put_page(page);
+ return NULL;
+ }
+ unlock_page(page);
+ shrink_readahead_size_eio(ra);
+ put_page(page);
+
[PATCH 1/2] fs: Break generic_file_buffered_read up into multiple functions
This is prep work for changing generic_file_buffered_read() to use
find_get_pages_contig() to batch up all the pagecache lookups.
This patch should be functionally identical to the existing code and
changes as little as of the flow control as possible. More refactoring
could be done, this patch is intended to be relatively minimal.
Signed-off-by: Kent Overstreet
---
mm/filemap.c | 418 ---
1 file changed, 228 insertions(+), 190 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index e67fa8ab48..206d51a1c9 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2051,6 +2051,210 @@ static void shrink_readahead_size_eio(struct
file_ra_state *ra)
ra->ra_pages /= 4;
}
+static int generic_file_buffered_read_page_ok(struct kiocb *iocb,
+ struct iov_iter *iter,
+ struct page *page)
+{
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
+ struct inode *inode = mapping->host;
+ struct file_ra_state *ra = >ki_filp->f_ra;
+ unsigned offset = iocb->ki_pos & ~PAGE_MASK;
+ unsigned bytes, copied;
+ loff_t isize, end_offset;
+
+ BUG_ON(iocb->ki_pos >> PAGE_SHIFT != page->index);
+
+ /*
+* i_size must be checked after we know the page is Uptodate.
+*
+* Checking i_size after the check allows us to calculate
+* the correct value for "bytes", which means the zero-filled
+* part of the page is not copied back to userspace (unless
+* another truncate extends the file - this is desired though).
+*/
+
+ isize = i_size_read(inode);
+ if (unlikely(iocb->ki_pos >= isize))
+ return 1;
+
+ end_offset = min_t(loff_t, isize, iocb->ki_pos + iter->count);
+
+ bytes = min_t(loff_t, end_offset - iocb->ki_pos, PAGE_SIZE - offset);
+
+ /* If users can be writing to this page using arbitrary
+* virtual addresses, take care about potential aliasing
+* before reading the page on the kernel side.
+*/
+ if (mapping_writably_mapped(mapping))
+ flush_dcache_page(page);
+
+ /*
+* Ok, we have the page, and it's up-to-date, so
+* now we can copy it to user space...
+*/
+
+ copied = copy_page_to_iter(page, offset, bytes, iter);
+
+ iocb->ki_pos += copied;
+
+ /*
+* When a sequential read accesses a page several times,
+* only mark it as accessed the first time.
+*/
+ if (iocb->ki_pos >> PAGE_SHIFT != ra->prev_pos >> PAGE_SHIFT)
+ mark_page_accessed(page);
+
+ ra->prev_pos = iocb->ki_pos;
+
+ if (copied < bytes)
+ return -EFAULT;
+
+ return !iov_iter_count(iter) || iocb->ki_pos == isize;
+}
+
+static struct page *
+generic_file_buffered_read_readpage(struct file *filp,
+ struct address_space *mapping,
+ struct page *page)
+{
+ struct file_ra_state *ra = >f_ra;
+ int error;
+
+ /*
+* A previous I/O error may have been due to temporary
+* failures, eg. multipath errors.
+* PG_error will be set again if readpage fails.
+*/
+ ClearPageError(page);
+ /* Start the actual read. The read will unlock the page. */
+ error = mapping->a_ops->readpage(filp, page);
+
+ if (unlikely(error)) {
+ put_page(page);
+ return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
+ }
+
+ if (!PageUptodate(page)) {
+ error = lock_page_killable(page);
+ if (unlikely(error)) {
+ put_page(page);
+ return ERR_PTR(error);
+ }
+ if (!PageUptodate(page)) {
+ if (page->mapping == NULL) {
+ /*
+* invalidate_mapping_pages got it
+*/
+ unlock_page(page);
+ put_page(page);
+ return NULL;
+ }
+ unlock_page(page);
+ shrink_readahead_size_eio(ra);
+ put_page(page);
+ return ERR_PTR(-EIO);
+ }
+ unlock_page(page);
+ }
+
+ return page;
+}
+
+static struct page *
+generic_file_buffered_read_pagenotuptodate(struct file *filp,
+ struct iov_iter *iter,
+ struct page *page,
+ loff_t pos, loff_t count)
+{
+ struct address_space *mapping = filp->f_mapping;
+ struct inode *inode = mapping->host;
+ int error;
+
+ /*
+* See comment in do_read_cache_page on why
+* wait_on_page_locked is used to
[PATCH 1/2] fs: Break generic_file_buffered_read up into multiple functions
This is prep work for changing generic_file_buffered_read() to use
find_get_pages_contig() to batch up all the pagecache lookups.
This patch should be functionally identical to the existing code and
changes as little as of the flow control as possible. More refactoring
could be done, this patch is intended to be relatively minimal.
Signed-off-by: Kent Overstreet
---
mm/filemap.c | 418 ---
1 file changed, 228 insertions(+), 190 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index c7723d..308bdd466f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2110,6 +2110,210 @@ static void shrink_readahead_size_eio(struct file *filp,
ra->ra_pages /= 4;
}
+static int generic_file_buffered_read_page_ok(struct kiocb *iocb,
+ struct iov_iter *iter,
+ struct page *page)
+{
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
+ struct inode *inode = mapping->host;
+ struct file_ra_state *ra = >ki_filp->f_ra;
+ unsigned offset = iocb->ki_pos & ~PAGE_MASK;
+ unsigned bytes, copied;
+ loff_t isize, end_offset;
+
+ BUG_ON(iocb->ki_pos >> PAGE_SHIFT != page->index);
+
+ /*
+* i_size must be checked after we know the page is Uptodate.
+*
+* Checking i_size after the check allows us to calculate
+* the correct value for "bytes", which means the zero-filled
+* part of the page is not copied back to userspace (unless
+* another truncate extends the file - this is desired though).
+*/
+
+ isize = i_size_read(inode);
+ if (unlikely(iocb->ki_pos >= isize))
+ return 1;
+
+ end_offset = min_t(loff_t, isize, iocb->ki_pos + iter->count);
+
+ bytes = min_t(loff_t, end_offset - iocb->ki_pos, PAGE_SIZE - offset);
+
+ /* If users can be writing to this page using arbitrary
+* virtual addresses, take care about potential aliasing
+* before reading the page on the kernel side.
+*/
+ if (mapping_writably_mapped(mapping))
+ flush_dcache_page(page);
+
+ /*
+* Ok, we have the page, and it's up-to-date, so
+* now we can copy it to user space...
+*/
+
+ copied = copy_page_to_iter(page, offset, bytes, iter);
+
+ iocb->ki_pos += copied;
+
+ /*
+* When a sequential read accesses a page several times,
+* only mark it as accessed the first time.
+*/
+ if (iocb->ki_pos >> PAGE_SHIFT != ra->prev_pos >> PAGE_SHIFT)
+ mark_page_accessed(page);
+
+ ra->prev_pos = iocb->ki_pos;
+
+ if (copied < bytes)
+ return -EFAULT;
+
+ return !iov_iter_count(iter) || iocb->ki_pos == isize;
+}
+
+static struct page *
+generic_file_buffered_read_readpage(struct file *filp,
+ struct address_space *mapping,
+ struct page *page)
+{
+ struct file_ra_state *ra = >f_ra;
+ int error;
+
+ /*
+* A previous I/O error may have been due to temporary
+* failures, eg. multipath errors.
+* PG_error will be set again if readpage fails.
+*/
+ ClearPageError(page);
+ /* Start the actual read. The read will unlock the page. */
+ error = mapping->a_ops->readpage(filp, page);
+
+ if (unlikely(error)) {
+ put_page(page);
+ return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
+ }
+
+ if (!PageUptodate(page)) {
+ error = lock_page_killable(page);
+ if (unlikely(error)) {
+ put_page(page);
+ return ERR_PTR(error);
+ }
+ if (!PageUptodate(page)) {
+ if (page->mapping == NULL) {
+ /*
+* invalidate_mapping_pages got it
+*/
+ unlock_page(page);
+ put_page(page);
+ return NULL;
+ }
+ unlock_page(page);
+ shrink_readahead_size_eio(filp, ra);
+ put_page(page);
+ return ERR_PTR(-EIO);
+ }
+ unlock_page(page);
+ }
+
+ return page;
+}
+
+static struct page *
+generic_file_buffered_read_pagenotuptodate(struct file *filp,
+ struct iov_iter *iter,
+ struct page *page,
+ loff_t pos, loff_t count)
+{
+ struct address_space *mapping = filp->f_mapping;
+ struct inode *inode = mapping->host;
+ int error;
+
+ /*
+* See comment in do_read_cache_page on why
+* wait_on_page_locked is used to
[PATCH 1/2] fs: Break generic_file_buffered_read up into multiple functions
This is prep work for changing generic_file_buffered_read() to use
find_get_pages_contig() to batch up all the pagecache lookups.
This patch should be functionally identical to the existing code and
changes as little as of the flow control as possible. More refactoring
could be done, this patch is intended to be relatively minimal.
Signed-off-by: Kent Overstreet
---
mm/filemap.c | 418 ---
1 file changed, 228 insertions(+), 190 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index c7723d..308bdd466f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2110,6 +2110,210 @@ static void shrink_readahead_size_eio(struct file *filp,
ra->ra_pages /= 4;
}
+static int generic_file_buffered_read_page_ok(struct kiocb *iocb,
+ struct iov_iter *iter,
+ struct page *page)
+{
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
+ struct inode *inode = mapping->host;
+ struct file_ra_state *ra = >ki_filp->f_ra;
+ unsigned offset = iocb->ki_pos & ~PAGE_MASK;
+ unsigned bytes, copied;
+ loff_t isize, end_offset;
+
+ BUG_ON(iocb->ki_pos >> PAGE_SHIFT != page->index);
+
+ /*
+* i_size must be checked after we know the page is Uptodate.
+*
+* Checking i_size after the check allows us to calculate
+* the correct value for "bytes", which means the zero-filled
+* part of the page is not copied back to userspace (unless
+* another truncate extends the file - this is desired though).
+*/
+
+ isize = i_size_read(inode);
+ if (unlikely(iocb->ki_pos >= isize))
+ return 1;
+
+ end_offset = min_t(loff_t, isize, iocb->ki_pos + iter->count);
+
+ bytes = min_t(loff_t, end_offset - iocb->ki_pos, PAGE_SIZE - offset);
+
+ /* If users can be writing to this page using arbitrary
+* virtual addresses, take care about potential aliasing
+* before reading the page on the kernel side.
+*/
+ if (mapping_writably_mapped(mapping))
+ flush_dcache_page(page);
+
+ /*
+* Ok, we have the page, and it's up-to-date, so
+* now we can copy it to user space...
+*/
+
+ copied = copy_page_to_iter(page, offset, bytes, iter);
+
+ iocb->ki_pos += copied;
+
+ /*
+* When a sequential read accesses a page several times,
+* only mark it as accessed the first time.
+*/
+ if (iocb->ki_pos >> PAGE_SHIFT != ra->prev_pos >> PAGE_SHIFT)
+ mark_page_accessed(page);
+
+ ra->prev_pos = iocb->ki_pos;
+
+ if (copied < bytes)
+ return -EFAULT;
+
+ return !iov_iter_count(iter) || iocb->ki_pos == isize;
+}
+
+static struct page *
+generic_file_buffered_read_readpage(struct file *filp,
+ struct address_space *mapping,
+ struct page *page)
+{
+ struct file_ra_state *ra = >f_ra;
+ int error;
+
+ /*
+* A previous I/O error may have been due to temporary
+* failures, eg. multipath errors.
+* PG_error will be set again if readpage fails.
+*/
+ ClearPageError(page);
+ /* Start the actual read. The read will unlock the page. */
+ error = mapping->a_ops->readpage(filp, page);
+
+ if (unlikely(error)) {
+ put_page(page);
+ return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
+ }
+
+ if (!PageUptodate(page)) {
+ error = lock_page_killable(page);
+ if (unlikely(error)) {
+ put_page(page);
+ return ERR_PTR(error);
+ }
+ if (!PageUptodate(page)) {
+ if (page->mapping == NULL) {
+ /*
+* invalidate_mapping_pages got it
+*/
+ unlock_page(page);
+ put_page(page);
+ return NULL;
+ }
+ unlock_page(page);
+ shrink_readahead_size_eio(filp, ra);
+ put_page(page);
+ return ERR_PTR(-EIO);
+ }
+ unlock_page(page);
+ }
+
+ return page;
+}
+
+static struct page *
+generic_file_buffered_read_pagenotuptodate(struct file *filp,
+ struct iov_iter *iter,
+ struct page *page,
+ loff_t pos, loff_t count)
+{
+ struct address_space *mapping = filp->f_mapping;
+ struct inode *inode = mapping->host;
+ int error;
+
+ /*
+* See comment in do_read_cache_page on why
+* wait_on_page_locked is used to
