* Re: Errors reported by Coverity in ext3
@ 2009-04-12 11:58 Amir Goldor
2009-04-13 22:02 ` Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldor @ 2009-04-12 11:58 UTC (permalink / raw)
To: Andreas Dilger; +Cc: Andrew Morton, linux-ext4
[-- Attachment #1: Type: text/plain, Size: 267 bytes --]
Hi Andreas,
in reply to your message from 2006
(http://marc.info/?l=linux-ext4&m=116310729115980&w=2)
I have prepared a small patch to check jbd errors on 4 occurrences in namei.c
could you please review my patch
please CC me personally in your reply
Thanks,
Amir.
[-- Attachment #2: ext3-namei-check-jbd-errors-2.6.28.patch --]
[-- Type: application/octet-stream, Size: 2151 bytes --]
diff -aur linux-2.6.28.orig/fs/ext3/namei.c linux-2.6.28/fs/ext3/namei.c
--- linux-2.6.28.orig/fs/ext3/namei.c 2009-04-12 11:58:41.000000000 +0300
+++ linux-2.6.28/fs/ext3/namei.c 2009-04-12 14:15:55.000000000 +0300
@@ -1616,7 +1616,7 @@
struct buffer_head * bh)
{
struct ext3_dir_entry_2 * de, * pde;
- int i;
+ int i, err;
i = 0;
pde = NULL;
@@ -1626,7 +1626,9 @@
return -EIO;
if (de == de_del) {
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) +
@@ -1770,7 +1772,13 @@
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? */
+ ext3_mark_inode_dirty(handle, inode);
+ iput (inode);
+ 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;
@@ -2302,7 +2310,9 @@
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))
@@ -2360,7 +2370,14 @@
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) {
+ ext3_warning(old_dir->i_sb, "ext3_rename",
+ "Updating new directory (%lu) parent link, %d, error=%d",
+ new_dir->i_ino, new_dir->i_nlink, retval);
+ }
+ }
+ if (dir_bh && !retval) {
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] 6+ messages in thread
* Re: Errors reported by Coverity in ext3
2009-04-12 11:58 Errors reported by Coverity in ext3 Amir Goldor
@ 2009-04-13 22:02 ` Andreas Dilger
2009-04-16 9:30 ` Amir Goldor
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2009-04-13 22:02 UTC (permalink / raw)
To: Amir Goldor; +Cc: linux-ext4
On Apr 12, 2009 14:58 +0300, Amir Goldor wrote:
> in reply to your message from 2006
> (http://marc.info/?l=linux-ext4&m=116310729115980&w=2)
> I have prepared a small patch to check jbd errors on 4 occurrences in namei.c
> could you please review my patch
Could you please regenerate your patch with "diff -up" so that the
function names are included into the patch context.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Errors reported by Coverity in ext3
2009-04-13 22:02 ` Andreas Dilger
@ 2009-04-16 9:30 ` Amir Goldor
2009-04-20 22:14 ` Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldor @ 2009-04-16 9:30 UTC (permalink / raw)
To: Andreas Dilger; +Cc: linux-ext4
[-- Attachment #1: Type: text/plain, Size: 837 bytes --]
Here it is.
Also, the patch that you posted back in 2006 mostly handled
jbd error check for "retaking write access" in ext3_clear_blocks().
Do you have a newer patch for that? are you still using that old patch?
Thanks,
Amir.
On Tue, Apr 14, 2009 at 1:02 AM, Andreas Dilger <adilger@sun.com> wrote:
>
> On Apr 12, 2009 14:58 +0300, Amir Goldor wrote:
> > in reply to your message from 2006
> > (http://marc.info/?l=linux-ext4&m=116310729115980&w=2)
> > I have prepared a small patch to check jbd errors on 4 occurrences in namei.c
> > could you please review my patch
>
> Could you please regenerate your patch with "diff -up" so that the
> function names are included into the patch context.
>
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
>
[-- Attachment #2: ext3-namei-check-jbd-errors-2.6.28.patch --]
[-- Type: application/octet-stream, Size: 2321 bytes --]
diff -up linux-2.6.28.orig/fs/ext3/namei.c linux-2.6.28/fs/ext3/namei.c
--- linux-2.6.28.orig/fs/ext3/namei.c 2009-04-16 12:11:44.000000000 +0300
+++ linux-2.6.28/fs/ext3/namei.c 2009-04-16 12:13:32.000000000 +0300
@@ -1616,7 +1616,7 @@ static int ext3_delete_entry (handle_t *
struct buffer_head * bh)
{
struct ext3_dir_entry_2 * de, * pde;
- int i;
+ int i, err;
i = 0;
pde = NULL;
@@ -1626,7 +1626,9 @@ static int ext3_delete_entry (handle_t *
return -EIO;
if (de == de_del) {
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) +
@@ -1770,7 +1772,13 @@ 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? */
+ ext3_mark_inode_dirty(handle, inode);
+ iput (inode);
+ 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;
@@ -2302,7 +2310,9 @@ static int ext3_rename (struct inode * o
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))
@@ -2360,7 +2370,14 @@ static int ext3_rename (struct inode * o
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) {
+ ext3_warning(old_dir->i_sb, "ext3_rename",
+ "Updating new directory (%lu) parent link, %d, error=%d",
+ new_dir->i_ino, new_dir->i_nlink, retval);
+ }
+ }
+ if (dir_bh && !retval) {
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] 6+ messages in thread
* Re: Errors reported by Coverity in ext3
2009-04-16 9:30 ` Amir Goldor
@ 2009-04-20 22:14 ` Andreas Dilger
2009-04-21 9:01 ` Amir Goldor
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2009-04-20 22:14 UTC (permalink / raw)
To: Amir Goldor; +Cc: linux-ext4
Thanks for the patch, unfortunately it is a NACK, since there are a few
bugs in the error handling, and some additional cleanups that can be done.
On Apr 16, 2009 12:30 +0300, Amir Goldor wrote:
> Also, the patch that you posted back in 2006 mostly handled
> jbd error check for "retaking write access" in ext3_clear_blocks().
> Do you have a newer patch for that? are you still using that old patch?
I don't recall which patch you refer to.
More comments inline.
> diff -up linux-2.6.28.orig/fs/ext3/namei.c linux-2.6.28/fs/ext3/namei.c
> --- linux-2.6.28.orig/fs/ext3/namei.c 2009-04-16 12:11:44.000000000 +0300
> +++ linux-2.6.28/fs/ext3/namei.c 2009-04-16 12:13:32.000000000 +0300
> @@ -1770,7 +1772,13 @@ static int ext3_mkdir(struct inode *dir
> 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? */
> + ext3_mark_inode_dirty(handle, inode);
> + iput (inode);
> + goto out_stop;
> + }
For the 2.6.29+ kernels you need to also include "unlock_new_inode(inode)"
after drop_nlink() in the error handling path.
It seems in all kernels you need to add "brelse(dir_block)" here as well,
because there was a reference gotten in ext3_bread() just above. It might
be enough to do:
if (err) {
brelse(dir_block);
goto out_clear_inode;
}
> @@ -2302,7 +2310,9 @@ static int ext3_rename (struct inode * o
> 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;
Similarly, this also needs a "brelse(new_bh)" before "goto end_rename".
> @@ -2360,7 +2370,14 @@ static int ext3_rename (struct inode * o
> ext3_update_dx_flag(old_dir);
> if (dir_bh) {
> BUFFER_TRACE(dir_bh, "get_write_access");
> + retval = ext3_journal_get_write_access(handle, dir_bh);
> + if (retval) {
> + ext3_warning(old_dir->i_sb, "ext3_rename",
> + "Updating new directory (%lu) parent link, %d, error=%d",
> + new_dir->i_ino, new_dir->i_nlink, retval);
> + }
> + }
At this point, if we cannot get write access to the directory buffer, it
is really a bit too late to do much about it. It may make more sense to
instead call ext3_journal_get_write_access() right after ext3_bread() is
called dor this buffer, so that the error can be checked and the rename
aborted before any changes are made.
Note also that there are some places where ext3_journal_dirty_metadata()
are called in these same code paths, but the error is not checked by the
caller.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Errors reported by Coverity in ext3
2009-04-20 22:14 ` Andreas Dilger
@ 2009-04-21 9:01 ` Amir Goldor
2009-04-21 11:59 ` [PATCH] " Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldor @ 2009-04-21 9:01 UTC (permalink / raw)
To: Andreas Dilger; +Cc: linux-ext4
[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]
Thanks for your corrections.
Attached is a new patch against kernel 2.6.29.1.
I applied most of your corrections, see my comments inline.
Please review take #2.
Thanks,
Amir.
On Tue, Apr 21, 2009 at 1:14 AM, Andreas Dilger <adilger@sun.com> wrote:
>
> Thanks for the patch, unfortunately it is a NACK, since there are a few
> bugs in the error handling, and some additional cleanups that can be done.
>
> More comments inline.
>
>> @@ -2302,7 +2310,9 @@ static int ext3_rename (struct inode * o
>> 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;
>
> Similarly, this also needs a "brelse(new_bh)" before "goto end_rename".
brelse(new_bh) as well as brelse(dir_bh) are called on end_rename.
>
>> @@ -2360,7 +2370,14 @@ static int ext3_rename (struct inode * o
>> ext3_update_dx_flag(old_dir);
>> if (dir_bh) {
>> BUFFER_TRACE(dir_bh, "get_write_access");
>> + retval = ext3_journal_get_write_access(handle, dir_bh);
>> + if (retval) {
>> + ext3_warning(old_dir->i_sb, "ext3_rename",
>> + "Updating new directory (%lu) parent link, %d, error=%d",
>> + new_dir->i_ino, new_dir->i_nlink, retval);
>> + }
>> + }
>
> At this point, if we cannot get write access to the directory buffer, it
> is really a bit too late to do much about it. It may make more sense to
> instead call ext3_journal_get_write_access() right after ext3_bread() is
> called dor this buffer, so that the error can be checked and the rename
> aborted before any changes are made.
>
good idea. I moved it up.
> Note also that there are some places where ext3_journal_dirty_metadata()
> are called in these same code paths, but the error is not checked by the
> caller.
>
true, but as with the case above, handling errors before the changes
are made is much easier,
and I am only aiming for 'best effort' in this patch.
[-- Attachment #2: ext3-namei-check-jbd-errors-2.6.29.1.patch --]
[-- Type: application/octet-stream, Size: 2552 bytes --]
diff -up linux-2.6.29.1.orig/fs/ext3/namei.c linux-2.6.29.1/fs/ext3/namei.c
--- linux-2.6.29.1.orig/fs/ext3/namei.c 2009-04-21 10:44:01.000000000 +0300
+++ linux-2.6.29.1/fs/ext3/namei.c 2009-04-21 11:35:12.000000000 +0300
@@ -1627,7 +1627,7 @@ static int ext3_delete_entry (handle_t *
struct buffer_head * bh)
{
struct ext3_dir_entry_2 * de, * pde;
- int i;
+ int i, err;
i = 0;
pde = NULL;
@@ -1637,7 +1637,9 @@ static int ext3_delete_entry (handle_t *
return -EIO;
if (de == de_del) {
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) +
@@ -1784,7 +1786,15 @@ 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);
+ 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;
@@ -2318,6 +2328,10 @@ static int ext3_rename (struct inode * o
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);
@@ -2325,7 +2339,9 @@ static int ext3_rename (struct inode * o
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))
@@ -2382,8 +2398,6 @@ static int ext3_rename (struct inode * o
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] 6+ messages in thread
* Re: [PATCH] Errors reported by Coverity in ext3
2009-04-21 9:01 ` Amir Goldor
@ 2009-04-21 11:59 ` Andreas Dilger
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2009-04-21 11:59 UTC (permalink / raw)
To: Amir Goldor; +Cc: linux-ext4
On Apr 21, 2009 12:01 +0300, Amir Goldor wrote:
> Attached is a new patch against kernel 2.6.29.1.
> I applied most of your corrections, see my comments inline.
Patch looks good. Full patch included below, because it was attached
as "application/octet-stream" and might have been missed by patchworks.
Acked-by: Andreas Dilger <adilger@sun.com>
---------------------------------------------------------------------------
diff -up linux-2.6.29.1.orig/fs/ext3/namei.c linux-2.6.29.1/fs/ext3/namei.c
--- linux-2.6.29.1.orig/fs/ext3/namei.c 2009-04-21 10:44:01.000000000 +0300
+++ linux-2.6.29.1/fs/ext3/namei.c 2009-04-21 11:35:12.000000000 +0300
@@ -1627,7 +1627,7 @@ static int ext3_delete_entry (handle_t *
struct buffer_head * bh)
{
struct ext3_dir_entry_2 * de, * pde;
- int i;
+ int i, err;
i = 0;
pde = NULL;
@@ -1637,7 +1637,9 @@ static int ext3_delete_entry (handle_t *
return -EIO;
if (de == de_del) {
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) +
@@ -1784,7 +1786,15 @@ 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);
+ 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;
@@ -2318,6 +2328,10 @@ static int ext3_rename (struct inode * o
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);
@@ -2325,7 +2339,9 @@ static int ext3_rename (struct inode * o
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))
@@ -2382,8 +2398,6 @@ static int ext3_rename (struct inode * o
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);
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-21 11:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-12 11:58 Errors reported by Coverity in ext3 Amir Goldor
2009-04-13 22:02 ` Andreas Dilger
2009-04-16 9:30 ` Amir Goldor
2009-04-20 22:14 ` Andreas Dilger
2009-04-21 9:01 ` Amir Goldor
2009-04-21 11:59 ` [PATCH] " Andreas Dilger
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).