Re: [PATCH] fix invalidate_inode_pages2_range not to clear ret

[Zach Brown <zach.brown@oracle.com>]

Hisashi Hifumi wrote:
> Hi.
> 
> DIO invalidates page cache through invalidate_inode_pages2_range().
> invalidate_inode_pages2_range() sets ret=-EIO when
> invalidate_complete_page2()
> fails, but this ret is cleared if do_launder_page() succeed on a page of
> next index.
> In this case, dio is carried out even if invalidate_complete_page2()
> fails on some pages.
> This can cause inconsistency between memory and blocks on HDD because
> the page
> cache still exists.
> 
> Following patch fixes this issue.

I like the idea of fixing this in a separate patch, yes, thanks!  But...

> 
> Thanks.
> 
> Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>
> 
> diff -Nrup linux-2.6.24-rc5.org/mm/truncate.c
> linux-2.6.24-rc5/mm/truncate.c
> --- linux-2.6.24-rc5.org/mm/truncate.c    2007-12-12 16:32:45.000000000
> +0900
> +++ linux-2.6.24-rc5/mm/truncate.c    2007-12-13 11:45:29.000000000 +0900
> @@ -392,6 +392,7 @@ int invalidate_inode_pages2_range(struct
>      pgoff_t next;
>      int i;
>      int ret = 0;
> +    int ret2 = 0;
>      int did_range_unmap = 0;
>      int wrapped = 0;
> 
> @@ -441,13 +442,13 @@ int invalidate_inode_pages2_range(struct
>              BUG_ON(page_mapped(page));
>              ret = do_launder_page(mapping, page);
>              if (ret == 0 && !invalidate_complete_page2(mapping, page))
> -                ret = -EIO;
> +                ret2 = -EIO;
>              unlock_page(page);
>          }
>          pagevec_release(&pvec);
>          cond_resched();
>      }
> -    return ret;
> +    return !ret ? ret2 : ret;
>  }
>  EXPORT_SYMBOL_GPL(invalidate_inode_pages2_range);

... this doesn't work. Notice that it only propagates the -EIO into ret2? It can lose errors from do_launder_page() itself because they aren't stored into ret2, which is what's returned if the last do_launder_page() succeeds.

This isn't I meant when I mentioned that 'ret2' pattern. The idea is to store the errors from inner calls into some private var and then only promote those errors as the function's return code (ret) if there was an error and ret wasn't already set.  We do this in a few places in dio, you'll notice.  This patch gets the meaning of 'ret' and 'ret2' opposite from those uses, which is kind of confusing.

So, uh, how about the following. Totally untested, but compiled.

-------

invalidate_inode_pages2_range(): consistently return first error

Hisashi Hifumi noticed that we were losing errors in
invalidate_inode_pages2_range().  Later do_launder_page() calls could overwrite
errors generated by earlier calls.  Fix this by storing do_launder_page in a
temporary variable which is only promoted to the function's return code if it
hadn't already generated an error.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
Cc: Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp> 

diff --git a/mm/truncate.c b/mm/truncate.c
index cadc156..24578b3 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -392,6 +392,7 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 	pgoff_t next;
 	int i;
 	int ret = 0;
+	int rc;
 	int did_range_unmap = 0;
 	int wrapped = 0;
 
@@ -439,9 +440,11 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 				}
 			}
 			BUG_ON(page_mapped(page));
-			ret = do_launder_page(mapping, page);
-			if (ret == 0 && !invalidate_complete_page2(mapping, page))
-				ret = -EIO;
+			rc = do_launder_page(mapping, page);
+			if (rc == 0 && !invalidate_complete_page2(mapping, page))
+				rc = -EIO;
+			if (rc && ret == 0)
+				ret = rc;
 			unlock_page(page);
 		}
 		pagevec_release(&pvec);

>  
> 
> -
> 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