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,
>
> 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 <[email protected]>
SuSE CR Labs
Andreas Gruenbacher <[email protected]> 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?
On Wed, Jan 26, 2005 at 11:24:30AM -0800, Andrew Morton wrote:
> Andreas Gruenbacher <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
SUSE Labs, SUSE LINUX GMBH
> On Wed, Jan 26, 2005 at 11:24:30AM -0800, Andrew Morton wrote:
> > Andreas Gruenbacher <[email protected]> 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 <[email protected]>
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 <[email protected]>
--- 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);