From: Wang Shilong <[email protected]>
commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into
a regression that casue BUG_ON when unlinking inode.
Reported-by: [email protected]
Signed-off-by: Wang Shilong <[email protected]>
---
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
On Tue, 12 Mar 2013, fanchaoting wrote:
> Date: Tue, 12 Mar 2013 13:06:37 +0800
> From: fanchaoting <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected],
> [email protected]
> Subject: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON
>
> From: Wang Shilong <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Wang Shilong <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> > To: [email protected]
> > Cc: [email protected], [email protected],
> > [email protected]
> > Subject: [PATCH] Ext2: do not mark_inode_dirty to avoid BUG_ON
> >
> > From: Wang Shilong <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Wang Shilong <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue 12-03-13 13:06:37, fanchaoting wrote:
> From: Wang Shilong <[email protected]>
>
> commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into
> a regression that casue BUG_ON when unlinking inode.
>
> Reported-by: [email protected]
> Signed-off-by: Wang Shilong <[email protected]>
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 <[email protected]>
SUSE Labs, CR
?? 2013-3-14??????7:09??Jan Kara <[email protected]> д????
> On Tue 12-03-13 13:06:37, fanchaoting wrote:
>> From: Wang Shilong <[email protected]>
>>
>> commit 8e3dffc651cb668e1ff4d8b89cc1c3dde7540d3b leads into
>> a regression that casue BUG_ON when unlinking inode.
>>
>> Reported-by: [email protected]
>> Signed-off-by: Wang Shilong <[email protected]>
> 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 <[email protected]>
> SUSE Labs, CR
> <0001-ext2-Fix-BUG_ON-in-evict-on-inode-deletion.patch>