fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE

RFC PATCH Superseded Abhi Das 2 participants 2 messages Dec 15, 2015 VFS

Share & tools

Abhi Das

author

2015-12-15 01:34 UTC #0 lore raw

RFC PATCH 1/1 fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE

During testing, I discovered that __generic_file_splice_read() returns 0 (EOF) when aops->readpage fails with AOP_TRUNCATED_PAGE on the first page of a single/multi-page splice read operation. This EOF return code causes the userspace test to (correctly) report a zero-length read error when it was expecting otherwise.

The current strategy of returning a partial non-zero read when ->readpage returns AOP_TRUNCATED_PAGE works only when the failed page is not the first of the lot being processed.

This patch attempts to retry lookup and call ->readpage again on pages that had previously failed with AOP_TRUNCATED_PAGE. With this patch, my tests pass and I haven't noticed any unwanted side effects.

Signed-off-by Abhi Das <[email protected]>

---
 fs/splice.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

fs/splice.c +7 -5

index 801c21c..c424d44 100644
@@ -387,6 +387,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 	spd.nr_pages = 0;
 	for (page_nr = 0; page_nr < nr_pages; page_nr++) {
 		unsigned int this_len;
+		int retries = 0;
 
 		if (!len)
 			break;

@@ -415,6 +416,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			 */
 			if (!page->mapping) {
 				unlock_page(page);
+retry_lookup:
 				page = find_or_create_page(mapping, index,
 						mapping_gfp_mask(mapping));

@@ -439,13 +441,13 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			error = mapping->a_ops->readpage(in, page);
 			if (unlikely(error)) {
 				/*
-				 * We really should re-lookup the page here,
-				 * but it complicates things a lot. Instead
-				 * lets just do what we already stored, and
-				 * we'll get it the next time we are called.
+				 * Re-lookup the page
 				 */
-				if (error == AOP_TRUNCATED_PAGE)
+				if (error == AOP_TRUNCATED_PAGE) {
+					if (retries++ < 3)
+						goto retry_lookup;
 					error = 0;
+				}
 
 				break;
 			}

Signature · 1 line

2.4.3

Bob Peterson

2015-12-15 13:45 UTC #1 lore raw

Hi Abhi,

Comment below.

----- Original Message -----

Quoted 45 lines

During testing, I discovered that __generic_file_splice_read() returns 0 (EOF) when aops->readpage fails with AOP_TRUNCATED_PAGE on the first page of a single/multi-page splice read operation. This EOF return code causes the userspace test to (correctly) report a zero-length read error when it was expecting otherwise.
The current strategy of returning a partial non-zero read when ->readpage returns AOP_TRUNCATED_PAGE works only when the failed page is not the first of the lot being processed.
This patch attempts to retry lookup and call ->readpage again on pages that had previously failed with AOP_TRUNCATED_PAGE. With this patch, my tests pass and I haven't noticed any unwanted side effects.
Signed-off-by Abhi Das <[email protected]>
---
 fs/splice.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
fs/splice.c +2 -1
index 801c21c..c424d44 100644
@@ -387,6 +387,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 	spd.nr_pages = 0;
 	for (page_nr = 0; page_nr < nr_pages; page_nr++) {
 		unsigned int this_len;
+		int retries = 0;
 
 		if (!len)
 			break;
@@ -415,6 +416,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			 */
 			if (!page->mapping) {
 				unlock_page(page);
+retry_lookup:
 				page = find_or_create_page(mapping, index,
 						mapping_gfp_mask(mapping));
 
@@ -439,13 +441,13 @@ __generic_file_splice_read(struct file *in, loff_t
 *ppos,
 			error = mapping->a_ops->readpage(in, page);
 			if (unlikely(error)) {
 				/*
-				 * We really should re-lookup the page here,

fs/splice.c __generic_file_splice_read(struct file *in, loff_t · line 450

Quoted 10 lines

fs/splice.c +4 -4

-				 * but it complicates things a lot. Instead
-				 * lets just do what we already stored, and
-				 * we'll get it the next time we are called.
+				 * Re-lookup the page
 				 */
-				if (error == AOP_TRUNCATED_PAGE)
+				if (error == AOP_TRUNCATED_PAGE) {
+					if (retries++ < 3)
+						goto retry_lookup;
 					error = 0;

I have no problem with doing the retry. However, I think we should probably set error = 0 before we decide whether to retry here. Otherwise, if error is AOP_TRUNCATED_PAGE, it might jump to retry_lookup, but then discover that the page is PageUptodate(), goto fill_it with error still set to AOP_TRUNCATED_PAGE, then end up returning that to the caller.

Regards,

Bob Peterson
Red Hat File Systems