On Tue 12-03-13 09:14:21, Lukáš Czerner wrote:
> On Tue, 12 Mar 2013, fanchaoting wrote:
>
> > Date: Tue, 12 Mar 2013 13:06:37 +0800
> > From: fanchaoting <fanchaoting@xxxxxxxxxxxxxx>
> > To: jack@xxxxxxx
> > Cc: tyhicks@xxxxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx,
> > wangshilong1991@xxxxxxxxx
> > Subject: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON
> >
> > From: Wang Shilong <wangsl-fnst@xxxxxxxxxxxxxx>
> >
> > commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into
> > a regression that casue BUG_ON when unlinking inode.
>
> Hi,
>
> it seems to be that we do need to mark the inode dirty, because
> we're changing inode->i_blocks from within
> dquot_free_block_nodirty().
>
> However looking at the code we usually call mark_inode_dirty(inode)
> after we call ext2_free_blocks() except when we're about to remove
> the inode so it seems that having that call within ext2_free_blocks()
> is not necessary.
Yeah. Actually the problem is specifically with ext2_xattr_delete_inode()
marking inode dirty because that is called after clear_inode(). Everything
before clear_inode() call is free to dirty the inode because clear_inode()
clears the dirty flag. I wonder if we shouldn't move that call into
ext2_evict_inode() before clear_inode() and be done with it. Because the
fact that ext2_free_blocks() cannot dirty the inode looks more surprising
than the fact that ext2_free_inode() doesn't automatically free extended
attributes.
> However I am not sure about the error path in ext2_alloc_branch()
> which does not dirty the inode after calling ext2_free_blocks().
> However presumably since we're just undoing the changes we might
> have done and not actually allocating, or freeing any space for
> real, dirtying the inode might not be necessary. Can you confirm
> that ?
I think that needs to dirty the inode. It may be written out in some
intermediate state...
Honza
> > Reported-by: tyhicks@xxxxxxxxxxxxx
> > Signed-off-by: Wang Shilong <wangsl-fnst@xxxxxxxxxxxxxx>
> > ---
> > fs/ext2/balloc.c | 1 -
> > 1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> > index 9f9992b..06d82fc 100644
> > --- a/fs/ext2/balloc.c
> > +++ b/fs/ext2/balloc.c
> > @@ -562,7 +562,6 @@ error_return:
> > if (freed) {
> > percpu_counter_add(&sbi->s_freeblocks_counter, freed);
> > dquot_free_block_nodirty(inode, freed);
> > - mark_inode_dirty(inode);
> > }
> > }
> >
> > -- 1.7.7.6
> >
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
