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.
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.
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 <[email protected]> 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.
>
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.
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 <[email protected]> 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.
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 <[email protected]>
---------------------------------------------------------------------------
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.