* 2.6.11-rc2/ext3 quota allocation bug on error path ...
@ 2005-01-22 15:50 Herbert Poetzl
2005-01-24 10:14 ` Jan Kara
2005-01-26 18:32 ` Andreas Gruenbacher
0 siblings, 2 replies; 6+ messages in thread
From: Herbert Poetzl @ 2005-01-22 15:50 UTC (permalink / raw)
To: linux-kernel; +Cc: Jan Kara, Andrew Morton
looking at ext3_xattr_block_set() [fs/ext3/xattr.c] ...
I see that
error = -EDQUOT;
if (DQUOT_ALLOC_BLOCK(inode, 1))
goto cleanup;
allocates a quota block, but right after that several
error echecks happen ...
if (error)
goto cleanup;
and I don't see any DQUOT_FREE_BLOCK() in the errorpath
cleanup:
if (ce)
mb_cache_entry_release(ce);
brelse(new_bh);
if (!(bs->bh && s->base == bs->bh->b_data))
kfree(s->base);
return error;
I'd suggest the attached fix (agains 2.6.11-rc2), comments?
best,
Herbert
--- ./fs/ext3/xattr.c.orig 2005-01-22 15:07:50 +0100
+++ ./fs/ext3/xattr.c 2005-01-22 16:45:09 +0100
@@ -773,7 +773,7 @@ inserted:
error = ext3_journal_get_write_access(handle,
new_bh);
if (error)
- goto cleanup;
+ goto cleanup_dquot;
lock_buffer(new_bh);
BHDR(new_bh)->h_refcount = cpu_to_le32(1 +
le32_to_cpu(BHDR(new_bh)->h_refcount));
@@ -783,7 +783,7 @@ inserted:
error = ext3_journal_dirty_metadata(handle,
new_bh);
if (error)
- goto cleanup;
+ goto cleanup_dquot;
}
mb_cache_entry_release(ce);
ce = NULL;
@@ -844,6 +844,10 @@ cleanup:
return error;
+cleanup_dquot:
+ DQUOT_FREE_BLOCK(inode, 1);
+ goto cleanup;
+
bad_block:
ext3_error(inode->i_sb, __FUNCTION__,
"inode %ld: bad block %d", inode->i_ino,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6.11-rc2/ext3 quota allocation bug on error path ...
2005-01-22 15:50 2.6.11-rc2/ext3 quota allocation bug on error path Herbert Poetzl
@ 2005-01-24 10:14 ` Jan Kara
2005-01-26 18:32 ` Andreas Gruenbacher
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kara @ 2005-01-24 10:14 UTC (permalink / raw)
To: linux-kernel, Andrew Morton; +Cc: Herbert Poetzl
>
> looking at ext3_xattr_block_set() [fs/ext3/xattr.c] ...
> I see that
>
> error = -EDQUOT;
> if (DQUOT_ALLOC_BLOCK(inode, 1))
> goto cleanup;
>
> allocates a quota block, but right after that several
> error echecks happen ...
>
> if (error)
> goto cleanup;
>
> and I don't see any DQUOT_FREE_BLOCK() in the errorpath
>
> cleanup:
> if (ce)
> mb_cache_entry_release(ce);
> brelse(new_bh);
> if (!(bs->bh && s->base == bs->bh->b_data))
> kfree(s->base);
>
> return error;
>
> I'd suggest the attached fix (agains 2.6.11-rc2), comments?
Yes, the patch looks right. Please apply it, Andrew. BTW ext2 needs a
similar fix - will submit in the next mail.
Honza
> --- ./fs/ext3/xattr.c.orig 2005-01-22 15:07:50 +0100
> +++ ./fs/ext3/xattr.c 2005-01-22 16:45:09 +0100
> @@ -773,7 +773,7 @@ inserted:
> error = ext3_journal_get_write_access(handle,
> new_bh);
> if (error)
> - goto cleanup;
> + goto cleanup_dquot;
> lock_buffer(new_bh);
> BHDR(new_bh)->h_refcount = cpu_to_le32(1 +
> le32_to_cpu(BHDR(new_bh)->h_refcount));
> @@ -783,7 +783,7 @@ inserted:
> error = ext3_journal_dirty_metadata(handle,
> new_bh);
> if (error)
> - goto cleanup;
> + goto cleanup_dquot;
> }
> mb_cache_entry_release(ce);
> ce = NULL;
> @@ -844,6 +844,10 @@ cleanup:
>
> return error;
>
> +cleanup_dquot:
> + DQUOT_FREE_BLOCK(inode, 1);
> + goto cleanup;
> +
> bad_block:
> ext3_error(inode->i_sb, __FUNCTION__,
> "inode %ld: bad block %d", inode->i_ino,
>
--
Jan Kara <jack@suse.cz>
SuSE CR Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6.11-rc2/ext3 quota allocation bug on error path ...
2005-01-22 15:50 2.6.11-rc2/ext3 quota allocation bug on error path Herbert Poetzl
2005-01-24 10:14 ` Jan Kara
@ 2005-01-26 18:32 ` Andreas Gruenbacher
2005-01-26 19:24 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Gruenbacher @ 2005-01-26 18:32 UTC (permalink / raw)
To: Herbert Poetzl, Andrew Morton, Linus Torvalds; +Cc: linux-kernel, Jan Kara
Hello,
On Sat, 2005-01-22 at 16:50, Herbert Poetzl wrote:
> --- ./fs/ext3/xattr.c.orig 2005-01-22 15:07:50 +0100
> +++ ./fs/ext3/xattr.c 2005-01-22 16:45:09 +0100
> @@ -773,7 +773,7 @@ inserted:
> error = ext3_journal_get_write_access(handle,
> new_bh);
> if (error)
> - goto cleanup;
> + goto cleanup_dquot;
> lock_buffer(new_bh);
> BHDR(new_bh)->h_refcount = cpu_to_le32(1 +
> le32_to_cpu(BHDR(new_bh)->h_refcount));
> @@ -783,7 +783,7 @@ inserted:
> error = ext3_journal_dirty_metadata(handle,
> new_bh);
> if (error)
> - goto cleanup;
> + goto cleanup_dquot;
> }
> mb_cache_entry_release(ce);
> ce = NULL;
> @@ -844,6 +844,10 @@ cleanup:
>
> return error;
>
> +cleanup_dquot:
> + DQUOT_FREE_BLOCK(inode, 1);
> + goto cleanup;
> +
> bad_block:
> ext3_error(inode->i_sb, __FUNCTION__,
> "inode %ld: bad block %d", inode->i_ino,
looks good. Can this please be added?
THanks,
--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6.11-rc2/ext3 quota allocation bug on error path ...
2005-01-26 18:32 ` Andreas Gruenbacher
@ 2005-01-26 19:24 ` Andrew Morton
2005-01-26 22:41 ` Herbert Poetzl
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2005-01-26 19:24 UTC (permalink / raw)
To: Andreas Gruenbacher; +Cc: herbert, torvalds, linux-kernel, jack
Andreas Gruenbacher <agruen@suse.de> wrote:
>
> > +cleanup_dquot:
> > + DQUOT_FREE_BLOCK(inode, 1);
> > + goto cleanup;
> > +
> > bad_block:
> > ext3_error(inode->i_sb, __FUNCTION__,
> > "inode %ld: bad block %d", inode->i_ino,
>
> looks good. Can this please be added?
Yup. But nobody has sent the equivalent ext2 fix yet?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6.11-rc2/ext3 quota allocation bug on error path ...
2005-01-26 19:24 ` Andrew Morton
@ 2005-01-26 22:41 ` Herbert Poetzl
2005-01-27 10:39 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Poetzl @ 2005-01-26 22:41 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andreas Gruenbacher, torvalds, linux-kernel, jack
On Wed, Jan 26, 2005 at 11:24:30AM -0800, Andrew Morton wrote:
> Andreas Gruenbacher <agruen@suse.de> wrote:
> >
> > > +cleanup_dquot:
> > > + DQUOT_FREE_BLOCK(inode, 1);
> > > + goto cleanup;
> > > +
> > > bad_block:
> > > ext3_error(inode->i_sb, __FUNCTION__,
> > > "inode %ld: bad block %d", inode->i_ino,
> >
> > looks good. Can this please be added?
>
> Yup. But nobody has sent the equivalent ext2 fix yet?
hmm, what about this one?
diff -NurpP --minimal linux-2.6.11-rc2/fs/ext2/xattr.c linux-2.6.11-rc2-fixed/fs/ext2/xattr.c
--- linux-2.6.11-rc2/fs/ext2/xattr.c 2005-01-22 15:07:50 +0100
+++ linux-2.6.11-rc2-fixed/fs/ext2/xattr.c 2005-01-26 22:40:28 +0100
@@ -706,8 +706,11 @@ ext2_xattr_set2(struct inode *inode, str
inode->i_ctime = CURRENT_TIME_SEC;
if (IS_SYNC(inode)) {
error = ext2_sync_inode (inode);
- if (error)
+ if (error) {
+ if (new_bh && new_bh != old_bh)
+ DQUOT_FREE_BLOCK(inode, 1);
goto cleanup;
+ }
} else
mark_inode_dirty(inode);
@@ -748,7 +751,6 @@ ext2_xattr_set2(struct inode *inode, str
cleanup:
brelse(new_bh);
-
return error;
}
and here the ext3 fix again:
Signed-off-by: Herbert Pötzl <herbert@13thfloor.at>
diff -NurpP --minimal linux-2.6.11-rc2/fs/ext3/xattr.c linux-2.6.11-rc2-fixed/fs/ext3/xattr.c
--- linux-2.6.11-rc2/fs/ext3/xattr.c 2005-01-22 15:07:50 +0100
+++ linux-2.6.11-rc2-fixed/fs/ext3/xattr.c 2005-01-26 22:19:29 +0100
@@ -773,7 +773,7 @@ inserted:
error = ext3_journal_get_write_access(handle,
new_bh);
if (error)
- goto cleanup;
+ goto cleanup_dquot;
lock_buffer(new_bh);
BHDR(new_bh)->h_refcount = cpu_to_le32(1 +
le32_to_cpu(BHDR(new_bh)->h_refcount));
@@ -783,7 +783,7 @@ inserted:
error = ext3_journal_dirty_metadata(handle,
new_bh);
if (error)
- goto cleanup;
+ goto cleanup_dquot;
}
mb_cache_entry_release(ce);
ce = NULL;
@@ -844,6 +844,10 @@ cleanup:
return error;
+cleanup_dquot:
+ DQUOT_FREE_BLOCK(inode, 1);
+ goto cleanup;
+
bad_block:
ext3_error(inode->i_sb, __FUNCTION__,
"inode %ld: bad block %d", inode->i_ino,
best,
Herbert
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6.11-rc2/ext3 quota allocation bug on error path ...
2005-01-26 22:41 ` Herbert Poetzl
@ 2005-01-27 10:39 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2005-01-27 10:39 UTC (permalink / raw)
To: Andrew Morton, Andreas Gruenbacher, Herbert Poetzl; +Cc: linux-kernel, torvalds
> On Wed, Jan 26, 2005 at 11:24:30AM -0800, Andrew Morton wrote:
> > Andreas Gruenbacher <agruen@suse.de> wrote:
> > >
> > > > +cleanup_dquot:
> > > > + DQUOT_FREE_BLOCK(inode, 1);
> > > > + goto cleanup;
> > > > +
> > > > bad_block:
> > > > ext3_error(inode->i_sb, __FUNCTION__,
> > > > "inode %ld: bad block %d", inode->i_ino,
> > >
> > > looks good. Can this please be added?
> >
> > Yup. But nobody has sent the equivalent ext2 fix yet?
>
> hmm, what about this one?
I have already made a fix and sent it to Andreas and linux-fsdevel.
For ext2 it's actually a bit more complicated because ext2_sync_inode()
can return with ENOSPC (because do_writepages() need not be able to
write the dirty data of the inode) but inode would be written and hence
in that case we should not release the quota (and we should release the
old xattr block properly). Andreas proposed to just call write_inode()
instead of ext2_sync_inode() to avoid the ENOSPC case but then we might
end up with inconsistency between inode metadata and indirect blocks for
SYNC inode (though the only way I see how this can happen is that we
race against file write and the short time inconsistency might be
bearable...).
I send a combination of mine and Herbert's patch.
Honza
--
Jan Kara <jack@suse.cz>
SuSE CR Labs
Fix a subtle bug in error handling of ext2 xattrs. When ext2_sync_inode() fails
because of ENOSPC (it could not write inode's dirty data) we want to keep
xattrs in a consistent state and release the old block properly.
Signed-off-by: Jan Kara <jack@suse.cz>
--- linux/fs/ext2/xattr.c 2005-01-27 12:56:25.782729816 +0100
+++ linux/fs/ext2/xattr.c 2005-01-27 13:14:21.196242112 +0100
@@ -706,8 +706,14 @@
inode->i_ctime = CURRENT_TIME_SEC;
if (IS_SYNC(inode)) {
error = ext2_sync_inode (inode);
- if (error)
+ /* In case sync failed due to ENOSPC the inode was actually
+ * written (only some dirty data were not) so we just proceed
+ * as if nothing happened and cleanup the unused block */
+ if (error && error != ENOSPC) {
+ if (new_bh && new_bh != old_bh)
+ DQUOT_FREE_BLOCK(inode, 1);
goto cleanup;
+ }
} else
mark_inode_dirty(inode);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-01-27 10:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-01-22 15:50 2.6.11-rc2/ext3 quota allocation bug on error path Herbert Poetzl
2005-01-24 10:14 ` Jan Kara
2005-01-26 18:32 ` Andreas Gruenbacher
2005-01-26 19:24 ` Andrew Morton
2005-01-26 22:41 ` Herbert Poetzl
2005-01-27 10:39 ` 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).