* [PATCH v2] ext3: Add journal error check into ext3_rename()
@ 2010-11-23 4:30 Namhyung Kim
2010-11-23 7:58 ` Amir Goldstein
2010-11-23 12:58 ` Jan Kara
0 siblings, 2 replies; 4+ messages in thread
From: Namhyung Kim @ 2010-11-23 4:30 UTC (permalink / raw)
To: Jan Kara, Andrew Morton, Andreas Dilger; +Cc: linux-ext4, linux-kernel
Check return value of ext3_journal_get_write_access() and
ext3_journal_dirty_metadata().
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext3: Add journal error check into ext3_rename()
2010-11-23 4:30 [PATCH v2] ext3: Add journal error check into ext3_rename() Namhyung Kim
@ 2010-11-23 7:58 ` Amir Goldstein
2010-11-23 12:57 ` Jan Kara
2010-11-23 12:58 ` Jan Kara
1 sibling, 1 reply; 4+ messages in thread
From: Amir Goldstein @ 2010-11-23 7:58 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jan Kara, Andrew Morton, Andreas Dilger, linux-ext4, linux-kernel
On Tue, Nov 23, 2010 at 6:30 AM, Namhyung Kim <namhyung@gmail.com> wrote:
> Check return value of ext3_journal_get_write_access() and
> ext3_journal_dirty_metadata().
>
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
> 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 <amir73il@users.sf.net>
--------------------------------------------------------------------------------
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext3: Add journal error check into ext3_rename()
2010-11-23 7:58 ` Amir Goldstein
@ 2010-11-23 12:57 ` Jan Kara
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2010-11-23 12:57 UTC (permalink / raw)
To: Amir Goldstein
Cc: Namhyung Kim, Jan Kara, Andrew Morton, Andreas Dilger,
linux-ext4, linux-kernel
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 <amir73il@users.sf.net>
>
> --------------------------------------------------------------------------------
> 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 <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext3: Add journal error check into ext3_rename()
2010-11-23 4:30 [PATCH v2] ext3: Add journal error check into ext3_rename() Namhyung Kim
2010-11-23 7:58 ` Amir Goldstein
@ 2010-11-23 12:58 ` Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: Jan Kara @ 2010-11-23 12:58 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jan Kara, Andrew Morton, Andreas Dilger, linux-ext4, linux-kernel
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 <namhyung@gmail.com>
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 <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-23 12:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-23 4:30 [PATCH v2] ext3: Add journal error check into ext3_rename() Namhyung Kim
2010-11-23 7:58 ` Amir Goldstein
2010-11-23 12:57 ` Jan Kara
2010-11-23 12:58 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).