* 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@vger.kernel.org, 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.