Check return value of ext3_journal_get_write_access() and
ext3_journal_dirty_metadata().
Signed-off-by: Namhyung Kim <[email protected]>
---
fs/ext3/namei.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 672cea1..c8415e7 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -2371,7 +2371,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
goto end_rename;
} else {
BUFFER_TRACE(new_bh, "get write access");
- ext3_journal_get_write_access(handle, new_bh);
+ retval = ext3_journal_get_write_access(handle, new_bh);
+ if (retval)
+ goto journal_error;
new_de->inode = cpu_to_le32(old_inode->i_ino);
if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
EXT3_FEATURE_INCOMPAT_FILETYPE))
@@ -2380,7 +2382,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
new_dir->i_ctime = new_dir->i_mtime = CURRENT_TIME_SEC;
ext3_mark_inode_dirty(handle, new_dir);
BUFFER_TRACE(new_bh, "call ext3_journal_dirty_metadata");
- ext3_journal_dirty_metadata(handle, new_bh);
+ retval = ext3_journal_dirty_metadata(handle, new_bh);
+ if (retval)
+ goto journal_error;
brelse(new_bh);
new_bh = NULL;
}
@@ -2429,10 +2433,17 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
ext3_update_dx_flag(old_dir);
if (dir_bh) {
BUFFER_TRACE(dir_bh, "get_write_access");
- ext3_journal_get_write_access(handle, dir_bh);
+ retval = ext3_journal_get_write_access(handle, dir_bh);
+ if (retval)
+ goto journal_error;
PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
- ext3_journal_dirty_metadata(handle, dir_bh);
+ retval = ext3_journal_dirty_metadata(handle, dir_bh);
+ if (retval) {
+journal_error:
+ ext3_std_error(new_dir->i_sb, retval);
+ goto end_rename;
+ }
drop_nlink(old_dir);
if (new_inode) {
drop_nlink(new_inode);
--
1.7.0.4
On Tue, Nov 23, 2010 at 6:30 AM, Namhyung Kim <[email protected]> wrote:
> Check return value of ext3_journal_get_write_access() and
> ext3_journal_dirty_metadata().
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> ?fs/ext3/namei.c | ? 19 +++++++++++++++----
> ?1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 672cea1..c8415e7 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -2371,7 +2371,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> ? ? ? ? ? ? ? ? ? ? ? ?goto end_rename;
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?BUFFER_TRACE(new_bh, "get write access");
> - ? ? ? ? ? ? ? ext3_journal_get_write_access(handle, new_bh);
> + ? ? ? ? ? ? ? retval = ext3_journal_get_write_access(handle, new_bh);
> + ? ? ? ? ? ? ? if (retval)
> + ? ? ? ? ? ? ? ? ? ? ? goto journal_error;
> ? ? ? ? ? ? ? ?new_de->inode = cpu_to_le32(old_inode->i_ino);
> ? ? ? ? ? ? ? ?if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?EXT3_FEATURE_INCOMPAT_FILETYPE))
> @@ -2380,7 +2382,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> ? ? ? ? ? ? ? ?new_dir->i_ctime = new_dir->i_mtime = CURRENT_TIME_SEC;
> ? ? ? ? ? ? ? ?ext3_mark_inode_dirty(handle, new_dir);
> ? ? ? ? ? ? ? ?BUFFER_TRACE(new_bh, "call ext3_journal_dirty_metadata");
> - ? ? ? ? ? ? ? ext3_journal_dirty_metadata(handle, new_bh);
> + ? ? ? ? ? ? ? retval = ext3_journal_dirty_metadata(handle, new_bh);
> + ? ? ? ? ? ? ? if (retval)
> + ? ? ? ? ? ? ? ? ? ? ? goto journal_error;
> ? ? ? ? ? ? ? ?brelse(new_bh);
> ? ? ? ? ? ? ? ?new_bh = NULL;
> ? ? ? ?}
> @@ -2429,10 +2433,17 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> ? ? ? ?ext3_update_dx_flag(old_dir);
> ? ? ? ?if (dir_bh) {
> ? ? ? ? ? ? ? ?BUFFER_TRACE(dir_bh, "get_write_access");
> - ? ? ? ? ? ? ? ext3_journal_get_write_access(handle, dir_bh);
> + ? ? ? ? ? ? ? retval = ext3_journal_get_write_access(handle, dir_bh);
> + ? ? ? ? ? ? ? if (retval)
> + ? ? ? ? ? ? ? ? ? ? ? goto journal_error;
> ? ? ? ? ? ? ? ?PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
> ? ? ? ? ? ? ? ?BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
> - ? ? ? ? ? ? ? ext3_journal_dirty_metadata(handle, dir_bh);
> + ? ? ? ? ? ? ? retval = ext3_journal_dirty_metadata(handle, dir_bh);
> + ? ? ? ? ? ? ? if (retval) {
> +journal_error:
> + ? ? ? ? ? ? ? ? ? ? ? ext3_std_error(new_dir->i_sb, retval);
> + ? ? ? ? ? ? ? ? ? ? ? goto end_rename;
> + ? ? ? ? ? ? ? }
Hi Kim,
A better way to handle this situation is to get_write_access much sooner,
when things can still be rolled back properly.
Please find below a patch I am using in next3 to fix some un-handled
journal errors
and see if you can/want to use any of it as reference for your patches.
Amir.
Some places in Ext3 original code don't check for return value of
JBD functions. Check for snapshot/journal errors in those places.
Signed-off-by: Amir Goldstein <[email protected]>
--------------------------------------------------------------------------------
diff -Nuarp a/fs/ext3/balloc.c b/fs/ext3/balloc.c
--- a/fs/ext3/balloc.c 2010-11-23 09:38:13.395922811 +0200
+++ b/fs/ext3/balloc.c 2010-11-23 09:38:13.185923236 +0200
@@ -674,7 +674,9 @@ do_more:
/* We dirtied the bitmap block */
BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
- err = ext3_journal_dirty_metadata(handle, bitmap_bh);
+ ret = ext3_journal_dirty_metadata(handle, bitmap_bh);
+ if (!err)
+ err = ret;
/* And the group descriptor block */
BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
diff -Nuarp a/fs/ext3/inode.c b/fs/ext3/inode.c
--- a/fs/ext3/inode.c 2010-11-23 09:38:13.405923214 +0200
+++ b/fs/ext3/inode.c 2010-11-23 09:38:13.195922747 +0200
@@ -2362,6 +2362,9 @@ static void ext3_clear_blocks(handle_t
if (bh) {
BUFFER_TRACE(bh, "retaking write access");
ext3_journal_get_write_access_inode(handle, inode, bh);
+ /* we may have lost write_access on bh */
+ if (is_handle_aborted(handle))
+ return;
}
}
@@ -2444,6 +2447,9 @@ static void ext3_free_data(handle_t *ha
ext3_clear_blocks(handle, inode, this_bh,
block_to_free,
count, block_to_free_p, p);
+ /* we may have lost write_access on this_bh */
+ if (is_handle_aborted(handle))
+ return;
block_to_free = nr;
block_to_free_p = p;
count = 1;
@@ -2454,6 +2460,9 @@ static void ext3_free_data(handle_t *ha
if (count > 0)
ext3_clear_blocks(handle, inode, this_bh, block_to_free,
count, block_to_free_p, p);
+ /* we may have lost write_access on this_bh */
+ if (is_handle_aborted(handle))
+ return;
if (this_bh) {
BUFFER_TRACE(this_bh, "call ext3_journal_dirty_metadata");
diff -Nuarp a/fs/ext3/namei.c b/fs/ext3/namei.c
--- a/fs/ext3/namei.c 2010-11-23 09:38:13.415922664 +0200
+++ b/fs/ext3/namei.c 2010-11-23 09:38:13.205923227 +0200
@@ -1649,8 +1649,12 @@ static int ext3_delete_entry (handle_t
if (!ext3_check_dir_entry("ext3_delete_entry", dir, de, bh, i))
return -EIO;
if (de == de_del) {
+ int err;
+
BUFFER_TRACE(bh, "get_write_access");
- ext3_journal_get_write_access(handle, bh);
+ err = ext3_journal_get_write_access(handle, bh);
+ if (err)
+ return err;
if (pde)
pde->rec_len = ext3_rec_len_to_disk(
ext3_rec_len_from_disk(pde->rec_len) +
@@ -1797,7 +1801,16 @@ retry:
goto out_stop;
}
BUFFER_TRACE(dir_block, "get_write_access");
- ext3_journal_get_write_access(handle, dir_block);
+ err = ext3_journal_get_write_access(handle, dir_block);
+ if (err) {
+ drop_nlink(inode); /* is this nlink == 0? */
+ unlock_new_inode(inode);
+ /* no need to check for errors - we failed anyway */
+ (void) ext3_mark_inode_dirty(handle, inode);
+ iput(inode);
+ brelse(dir_block);
+ goto out_stop;
+ }
de = (struct ext3_dir_entry_2 *) dir_block->b_data;
de->inode = cpu_to_le32(inode->i_ino);
de->name_len = 1;
@@ -2337,6 +2350,10 @@ static int ext3_rename (struct inode *
if (!new_inode && new_dir!=old_dir &&
new_dir->i_nlink >= EXT3_LINK_MAX)
goto end_rename;
+ BUFFER_TRACE(dir_bh, "get_write_access");
+ retval = ext3_journal_get_write_access(handle, dir_bh);
+ if (retval)
+ goto end_rename;
}
if (!new_bh) {
retval = ext3_add_entry (handle, new_dentry, old_inode);
@@ -2344,7 +2361,9 @@ static int ext3_rename (struct inode *
goto end_rename;
} else {
BUFFER_TRACE(new_bh, "get write access");
- ext3_journal_get_write_access(handle, new_bh);
+ retval = ext3_journal_get_write_access(handle, new_bh);
+ if (retval)
+ goto end_rename;
new_de->inode = cpu_to_le32(old_inode->i_ino);
if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
EXT3_FEATURE_INCOMPAT_FILETYPE))
@@ -2401,8 +2420,6 @@ static int ext3_rename (struct inode *
old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
ext3_update_dx_flag(old_dir);
if (dir_bh) {
- BUFFER_TRACE(dir_bh, "get_write_access");
- ext3_journal_get_write_access(handle, dir_bh);
PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
ext3_journal_dirty_metadata(handle, dir_bh);
Hello Amir,
On Tue 23-11-10 09:58:58, Amir Goldstein wrote:
> A better way to handle this situation is to get_write_access much sooner,
> when things can still be rolled back properly.
> Please find below a patch I am using in next3 to fix some un-handled
> journal errors
> and see if you can/want to use any of it as reference for your patches.
While generally it is certainly desirable to detect errors early and bail
out without doing harm, in this particular case I don't see a big point
in complicating the code since the only realistic failure case is that the
journal is aborted, we have to run fsck anyway, and we are not causing any
unfixable damage... So I'll take the patch as is.
Honza
> Some places in Ext3 original code don't check for return value of
> JBD functions. Check for snapshot/journal errors in those places.
>
> Signed-off-by: Amir Goldstein <[email protected]>
>
> --------------------------------------------------------------------------------
> diff -Nuarp a/fs/ext3/balloc.c b/fs/ext3/balloc.c
> --- a/fs/ext3/balloc.c 2010-11-23 09:38:13.395922811 +0200
> +++ b/fs/ext3/balloc.c 2010-11-23 09:38:13.185923236 +0200
> @@ -674,7 +674,9 @@ do_more:
>
> /* We dirtied the bitmap block */
> BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
> - err = ext3_journal_dirty_metadata(handle, bitmap_bh);
> + ret = ext3_journal_dirty_metadata(handle, bitmap_bh);
> + if (!err)
> + err = ret;
>
> /* And the group descriptor block */
> BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
>
> diff -Nuarp a/fs/ext3/inode.c b/fs/ext3/inode.c
> --- a/fs/ext3/inode.c 2010-11-23 09:38:13.405923214 +0200
> +++ b/fs/ext3/inode.c 2010-11-23 09:38:13.195922747 +0200
> @@ -2362,6 +2362,9 @@ static void ext3_clear_blocks(handle_t
> if (bh) {
> BUFFER_TRACE(bh, "retaking write access");
> ext3_journal_get_write_access_inode(handle, inode, bh);
> + /* we may have lost write_access on bh */
> + if (is_handle_aborted(handle))
> + return;
> }
> }
>
> @@ -2444,6 +2447,9 @@ static void ext3_free_data(handle_t *ha
> ext3_clear_blocks(handle, inode, this_bh,
> block_to_free,
> count, block_to_free_p, p);
> + /* we may have lost write_access on this_bh */
> + if (is_handle_aborted(handle))
> + return;
> block_to_free = nr;
> block_to_free_p = p;
> count = 1;
> @@ -2454,6 +2460,9 @@ static void ext3_free_data(handle_t *ha
> if (count > 0)
> ext3_clear_blocks(handle, inode, this_bh, block_to_free,
> count, block_to_free_p, p);
> + /* we may have lost write_access on this_bh */
> + if (is_handle_aborted(handle))
> + return;
>
> if (this_bh) {
> BUFFER_TRACE(this_bh, "call ext3_journal_dirty_metadata");
>
> diff -Nuarp a/fs/ext3/namei.c b/fs/ext3/namei.c
> --- a/fs/ext3/namei.c 2010-11-23 09:38:13.415922664 +0200
> +++ b/fs/ext3/namei.c 2010-11-23 09:38:13.205923227 +0200
> @@ -1649,8 +1649,12 @@ static int ext3_delete_entry (handle_t
> if (!ext3_check_dir_entry("ext3_delete_entry", dir, de, bh, i))
> return -EIO;
> if (de == de_del) {
> + int err;
> +
> BUFFER_TRACE(bh, "get_write_access");
> - ext3_journal_get_write_access(handle, bh);
> + err = ext3_journal_get_write_access(handle, bh);
> + if (err)
> + return err;
> if (pde)
> pde->rec_len = ext3_rec_len_to_disk(
> ext3_rec_len_from_disk(pde->rec_len) +
> @@ -1797,7 +1801,16 @@ retry:
> goto out_stop;
> }
> BUFFER_TRACE(dir_block, "get_write_access");
> - ext3_journal_get_write_access(handle, dir_block);
> + err = ext3_journal_get_write_access(handle, dir_block);
> + if (err) {
> + drop_nlink(inode); /* is this nlink == 0? */
> + unlock_new_inode(inode);
> + /* no need to check for errors - we failed anyway */
> + (void) ext3_mark_inode_dirty(handle, inode);
> + iput(inode);
> + brelse(dir_block);
> + goto out_stop;
> + }
> de = (struct ext3_dir_entry_2 *) dir_block->b_data;
> de->inode = cpu_to_le32(inode->i_ino);
> de->name_len = 1;
>
> @@ -2337,6 +2350,10 @@ static int ext3_rename (struct inode *
> if (!new_inode && new_dir!=old_dir &&
> new_dir->i_nlink >= EXT3_LINK_MAX)
> goto end_rename;
> + BUFFER_TRACE(dir_bh, "get_write_access");
> + retval = ext3_journal_get_write_access(handle, dir_bh);
> + if (retval)
> + goto end_rename;
> }
> if (!new_bh) {
> retval = ext3_add_entry (handle, new_dentry, old_inode);
> @@ -2344,7 +2361,9 @@ static int ext3_rename (struct inode *
> goto end_rename;
> } else {
> BUFFER_TRACE(new_bh, "get write access");
> - ext3_journal_get_write_access(handle, new_bh);
> + retval = ext3_journal_get_write_access(handle, new_bh);
> + if (retval)
> + goto end_rename;
> new_de->inode = cpu_to_le32(old_inode->i_ino);
> if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
> EXT3_FEATURE_INCOMPAT_FILETYPE))
> @@ -2401,8 +2420,6 @@ static int ext3_rename (struct inode *
> old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
> ext3_update_dx_flag(old_dir);
> if (dir_bh) {
> - BUFFER_TRACE(dir_bh, "get_write_access");
> - ext3_journal_get_write_access(handle, dir_bh);
> PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
> BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
> ext3_journal_dirty_metadata(handle, dir_bh);
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue 23-11-10 13:30:33, Namhyung Kim wrote:
> Check return value of ext3_journal_get_write_access() and
> ext3_journal_dirty_metadata().
>
> Signed-off-by: Namhyung Kim <[email protected]>
Thanks. Merged.
Honza
> ---
> fs/ext3/namei.c | 19 +++++++++++++++----
> 1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 672cea1..c8415e7 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -2371,7 +2371,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> goto end_rename;
> } else {
> BUFFER_TRACE(new_bh, "get write access");
> - ext3_journal_get_write_access(handle, new_bh);
> + retval = ext3_journal_get_write_access(handle, new_bh);
> + if (retval)
> + goto journal_error;
> new_de->inode = cpu_to_le32(old_inode->i_ino);
> if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
> EXT3_FEATURE_INCOMPAT_FILETYPE))
> @@ -2380,7 +2382,9 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> new_dir->i_ctime = new_dir->i_mtime = CURRENT_TIME_SEC;
> ext3_mark_inode_dirty(handle, new_dir);
> BUFFER_TRACE(new_bh, "call ext3_journal_dirty_metadata");
> - ext3_journal_dirty_metadata(handle, new_bh);
> + retval = ext3_journal_dirty_metadata(handle, new_bh);
> + if (retval)
> + goto journal_error;
> brelse(new_bh);
> new_bh = NULL;
> }
> @@ -2429,10 +2433,17 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
> ext3_update_dx_flag(old_dir);
> if (dir_bh) {
> BUFFER_TRACE(dir_bh, "get_write_access");
> - ext3_journal_get_write_access(handle, dir_bh);
> + retval = ext3_journal_get_write_access(handle, dir_bh);
> + if (retval)
> + goto journal_error;
> PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
> BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
> - ext3_journal_dirty_metadata(handle, dir_bh);
> + retval = ext3_journal_dirty_metadata(handle, dir_bh);
> + if (retval) {
> +journal_error:
> + ext3_std_error(new_dir->i_sb, retval);
> + goto end_rename;
> + }
> drop_nlink(old_dir);
> if (new_inode) {
> drop_nlink(new_inode);
> --
> 1.7.0.4
>
--
Jan Kara <[email protected]>
SUSE Labs, CR