* [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON @ 2013-03-12 5:06 fanchaoting 2013-03-12 8:14 ` Lukáš Czerner 2013-03-13 23:09 ` Jan Kara 0 siblings, 2 replies; 5+ messages in thread From: fanchaoting @ 2013-03-12 5:06 UTC (permalink / raw) To: jack; +Cc: tyhicks, linux-ext4, wangshilong1991 From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into a regression that casue BUG_ON when unlinking inode. Reported-by: tyhicks@canonical.com Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON 2013-03-12 5:06 [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON fanchaoting @ 2013-03-12 8:14 ` Lukáš Czerner 2013-03-12 10:13 ` Jan Kara 2013-03-13 23:09 ` Jan Kara 1 sibling, 1 reply; 5+ messages in thread From: Lukáš Czerner @ 2013-03-12 8:14 UTC (permalink / raw) To: fanchaoting; +Cc: jack, tyhicks, linux-ext4, wangshilong1991 On Tue, 12 Mar 2013, fanchaoting wrote: > Date: Tue, 12 Mar 2013 13:06:37 +0800 > From: fanchaoting <fanchaoting@cn.fujitsu.com> > To: jack@suse.cz > Cc: tyhicks@canonical.com, linux-ext4@vger.kernel.org, > wangshilong1991@gmail.com > Subject: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON > > From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > 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. 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 ? Thanks! -Lukas > > Reported-by: tyhicks@canonical.com > Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > --- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON 2013-03-12 8:14 ` Lukáš Czerner @ 2013-03-12 10:13 ` Jan Kara 0 siblings, 0 replies; 5+ messages in thread From: Jan Kara @ 2013-03-12 10:13 UTC (permalink / raw) To: Lukáš Czerner Cc: fanchaoting, jack, tyhicks, linux-ext4, wangshilong1991 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@cn.fujitsu.com> > > To: jack@suse.cz > > Cc: tyhicks@canonical.com, linux-ext4@vger.kernel.org, > > wangshilong1991@gmail.com > > Subject: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON > > > > From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > > > 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@canonical.com > > Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > --- > > 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@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Jan Kara <jack@suse.cz> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON 2013-03-12 5:06 [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON fanchaoting 2013-03-12 8:14 ` Lukáš Czerner @ 2013-03-13 23:09 ` Jan Kara 2013-03-14 0:43 ` Wang Shilong 1 sibling, 1 reply; 5+ messages in thread From: Jan Kara @ 2013-03-13 23:09 UTC (permalink / raw) To: fanchaoting; +Cc: jack, tyhicks, linux-ext4, wangshilong1991 [-- Attachment #1: Type: text/plain, Size: 939 bytes --] On Tue 12-03-13 13:06:37, fanchaoting wrote: > From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into > a regression that casue BUG_ON when unlinking inode. > > Reported-by: tyhicks@canonical.com > Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> I ended up fixing the problem by the attached patch. It looks cleaner to me that way... Thanks for your fix anyway. Honza > --- > 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 > > > > -- Jan Kara <jack@suse.cz> SUSE Labs, CR [-- Attachment #2: 0001-ext2-Fix-BUG_ON-in-evict-on-inode-deletion.patch --] [-- Type: text/x-patch, Size: 1869 bytes --] >From c288d2969627be7ffc90904ac8c6aae0295fbf9f Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@suse.cz> Date: Wed, 13 Mar 2013 12:57:08 +0100 Subject: [PATCH] ext2: Fix BUG_ON in evict() on inode deletion Commit 8e3dffc6 introduced a regression where deleting inode with large extended attributes leads to triggering BUG_ON(inode->i_state != (I_FREEING | I_CLEAR)) in fs/inode.c:evict(). That happens because freeing of xattr block dirtied the inode and it happened after clear_inode() has been called. Fix the issue by moving removal of xattr block into ext2_evict_inode() before clear_inode() call close to a place where data blocks are truncated. That is also more logical place and removes surprising requirement that ext2_free_blocks() mustn't dirty the inode. Reported-by: Tyler Hicks <tyhicks@canonical.com> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext2/ialloc.c | 1 - fs/ext2/inode.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c index 8f370e0..7cadd82 100644 --- a/fs/ext2/ialloc.c +++ b/fs/ext2/ialloc.c @@ -118,7 +118,6 @@ void ext2_free_inode (struct inode * inode) * as writing the quota to disk may need the lock as well. */ /* Quota is already initialized in iput() */ - ext2_xattr_delete_inode(inode); dquot_free_inode(inode); dquot_drop(inode); diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index c3881e5..fe60cc1 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -34,6 +34,7 @@ #include "ext2.h" #include "acl.h" #include "xip.h" +#include "xattr.h" static int __ext2_write_inode(struct inode *inode, int do_sync); @@ -88,6 +89,7 @@ void ext2_evict_inode(struct inode * inode) inode->i_size = 0; if (inode->i_blocks) ext2_truncate_blocks(inode, 0); + ext2_xattr_delete_inode(inode); } invalidate_inode_buffers(inode); -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON 2013-03-13 23:09 ` Jan Kara @ 2013-03-14 0:43 ` Wang Shilong 0 siblings, 0 replies; 5+ messages in thread From: Wang Shilong @ 2013-03-14 0:43 UTC (permalink / raw) To: Jan Kara, fanchaoting; +Cc: tyhicks, linux-ext4 在 2013-3-14，上午7:09，Jan Kara <jack@suse.cz> 写道： > On Tue 12-03-13 13:06:37, fanchaoting wrote: >> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> >> >> commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into >> a regression that casue BUG_ON when unlinking inode. >> >> Reported-by: tyhicks@canonical.com >> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > I ended up fixing the problem by the attached patch. It looks cleaner to > me that way... Thanks for your fix anyway. Sorry for delay reply, I am busy with my graduation project these two days. However, your patch looks more reasonable than mine. Thanks very much to fanchaoting sending my patch out anyway. Thanks, Wang > > Honza >> --- >> 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 >> >> >> >> > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR > <0001-ext2-Fix-BUG_ON-in-evict-on-inode-deletion.patch> -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-14 0:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-12 5:06 [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON fanchaoting 2013-03-12 8:14 ` Lukáš Czerner 2013-03-12 10:13 ` Jan Kara 2013-03-13 23:09 ` Jan Kara 2013-03-14 0:43 ` Wang Shilong
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).