2015-05-07 09:26:11

by Xiaoguang Wang

Subject: [PATCH] ext4: issue WARN_ON_ONCE() if page gets truncated in ext4_write_[da_]begin()

In ext4_write_[da_]begin(), when we get a page successfully by calling
grab_cache_page_write_begin(), then unless this page is truncated by
ext4_truncate_failed_write(), which is called when ext4_write_[da_]begin()
run into some errors, otherwise I think this page won't be truncated by
other kernel path because we're holding i_mutex.

In this patch, if page is truncated by ext4_truncate_failed_write(), then
we just put it and call grab_cache_page_write_begin() again to get a new
page, then this check 'if (page->mapping != mapping)' won't be necessary.
We can remove this check, but according to Jan Kara's suggestion, we issue
WARN_ON_ONCE() here if page is truncated, that means something is wrong.
(Thanks to Jan Kara for his help)

Signed-off-by: Xiaoguang Wang <[email protected]>
---
fs/ext4/inode.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4415cea..5961fdf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -984,7 +984,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
struct inode *inode = mapping->host;
int ret, needed_blocks;
handle_t *handle;
- int retries = 0;
+ int retries = 0, page_truncated = 0;
struct page *page;
pgoff_t index;
unsigned from, to;
@@ -1029,7 +1029,7 @@ retry_journal:
}

lock_page(page);
- if (page->mapping != mapping) {
+ if (WARN_ON_ONCE(page->mapping != mapping)) {
/* The page got truncated from under us */
unlock_page(page);
page_cache_release(page);
@@ -1074,6 +1074,7 @@ retry_journal:
ext4_journal_stop(handle);
if (pos + len > inode->i_size) {
ext4_truncate_failed_write(inode);
+ page_truncated = 1;
/*
* If truncate failed early the inode might
* still be on the orphan list; we need to
@@ -1085,8 +1086,15 @@ retry_journal:
}

if (ret == -ENOSPC &&
- ext4_should_retry_alloc(inode->i_sb, &retries))
- goto retry_journal;
+ ext4_should_retry_alloc(inode->i_sb, &retries)) {
+ if (page_truncated) {
+ page_cache_release(page);
+ page_truncated = 0;
+ goto retry_grab;
+ } else {
+ goto retry_journal;
+ }
+ }
page_cache_release(page);
return ret;
}
@@ -2609,7 +2617,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
struct page **pagep, void **fsdata)
{
- int ret, retries = 0;
+ int ret, retries = 0, page_truncated = 0;
struct page *page;
pgoff_t index;
struct inode *inode = mapping->host;
@@ -2663,7 +2671,7 @@ retry_journal:
}

lock_page(page);
- if (page->mapping != mapping) {
+ if (WARN_ON_ONCE(page->mapping != mapping)) {
/* The page got truncated from under us */
unlock_page(page);
page_cache_release(page);
@@ -2687,12 +2695,21 @@ retry_journal:
* outside i_size. Trim these off again. Don't need
* i_size_read because we hold i_mutex.
*/
- if (pos + len > inode->i_size)
+ if (pos + len > inode->i_size) {
ext4_truncate_failed_write(inode);
+ page_truncated = 1;
+ }

if (ret == -ENOSPC &&
- ext4_should_retry_alloc(inode->i_sb, &retries))
- goto retry_journal;
+ ext4_should_retry_alloc(inode->i_sb, &retries)) {
+ if (page_truncated) {
+ page_cache_release(page);
+ page_truncated = 0;
+ goto retry_grab;
+ } else {
+ goto retry_journal;
+ }
+ }

page_cache_release(page);
return ret;
--
1.8.3.1

2015-05-17 17:51:20

by Theodore Ts'o

Subject: Re: [PATCH] ext4: issue WARN_ON_ONCE() if page gets truncated in ext4_write_[da_]begin()

On Thu, May 07, 2015 at 05:17:23PM +0800, Xiaoguang Wang wrote:
> In ext4_write_[da_]begin(), when we get a page successfully by calling
> grab_cache_page_write_begin(), then unless this page is truncated by
> ext4_truncate_failed_write(), which is called when ext4_write_[da_]begin()
> run into some errors, otherwise I think this page won't be truncated by
> other kernel path because we're holding i_mutex.
>
> In this patch, if page is truncated by ext4_truncate_failed_write(), then
> we just put it and call grab_cache_page_write_begin() again to get a new
> page, then this check 'if (page->mapping != mapping)' won't be necessary.
> We can remove this check, but according to Jan Kara's suggestion, we issue
> WARN_ON_ONCE() here if page is truncated, that means something is wrong.
> (Thanks to Jan Kara for his help)

It would be helpful if the commit description explained why this
change is a benefit. There's a very detailed explanation about why
it's safe, but why do we need to release the page only to try again to
grab it by jumping to retry_grab instead of retry_journal? What
problem are you trying to solve?

Thanks, regards,

- Ted