| Message ID | 1234898659-4686-1-git-send-email-dmonakhov@openvz.org |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 17, 2009 at 10:24:19PM +0300, Dmitri Monakhov wrote: > Seems there is no strict rule to for this case. In fact everybody > just ignored this variable after write_begin() has failed. > This was true until ext4_defrag_partial(). So let's follows simple > rule similar to block_write_begin(). I've checked the latest ext4_defrag_partial, and it doesn't reference the page variable if write_begin() failed.... - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Theodore Tso <tytso@mit.edu> writes: > On Tue, Feb 17, 2009 at 10:24:19PM +0300, Dmitri Monakhov wrote: >> Seems there is no strict rule to for this case. In fact everybody >> just ignored this variable after write_begin() has failed. >> This was true until ext4_defrag_partial(). So let's follows simple >> rule similar to block_write_begin(). > > I've checked the latest ext4_defrag_partial, and it doesn't reference > the page variable if write_begin() failed.... > > - Ted Latest posted defrag version 1.0 from 30 Jan, contains following code + up_write(&EXT4_I(org_inode)->i_data_sem); + ret = a_ops->write_begin(o_filp, mapping, offs, data_len, w_flags, + &page, &fsdata); + down_write(&EXT4_I(org_inode)->i_data_sem); + + if (unlikely(ret < 0)) + goto out; ..... +out: + if (unlikely(page)) { + if (PageLocked(page)) + unlock_page(page); + page_cache_release(page); + } In fact the patch i'm propose is mostly a cleanup, rather then serious fix for real issue. IMHO it is not bad idea to fold it in to some real patch. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c index 5fa453b..7aa60db 100644 --- a/fs/ext3/inode.c +++ b/fs/ext3/inode.c @@ -1170,6 +1170,7 @@ retry: if (IS_ERR(handle)) { unlock_page(page); page_cache_release(page); + *pagep = NULL; ret = PTR_ERR(handle); goto out; } @@ -1187,6 +1188,7 @@ write_begin_failed: ext3_journal_stop(handle); unlock_page(page); page_cache_release(page); + *pagep = NULL; /* * block_write_begin may have instantiated a few blocks * outside i_size. Trim these off again. Don't need diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 658c4a7..0581c48 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1388,6 +1388,7 @@ retry: unlock_page(page); ext4_journal_stop(handle); page_cache_release(page); + *pagep = NULL; /* * block_write_begin may have instantiated a few blocks * outside i_size. Trim these off again. Don't need
Seems there is no strict rule to for this case. In fact everybody just ignored this variable after write_begin() has failed. This was true until ext4_defrag_partial(). So let's follows simple rule similar to block_write_begin(). Signed-off-by: Dmitri Monakhov <dmonakhov@openvz.org> --- fs/ext3/inode.c | 2 ++ fs/ext4/inode.c | 1 + 2 files changed, 3 insertions(+), 0 deletions(-)