Date:	Fri, 1 Apr 2011 02:54:42 -0700
From:	Amir Goldstein <amir73il@...il.com>
To:	Jan Kara <jack@...e.cz>, Yongqiang Yang <xiaoqiangnk@...il.com>
Cc:	Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org,
	tytso@....edu, Eric Sandeen <esandeen@...hat.com>
Subject: Re: kernel BUG at fs/jbd2/transaction.c:1086!

On Thu, Mar 31, 2011 at 3:23 PM, Jan Kara <jack@...e.cz> wrote:
> On Thu 31-03-11 22:59:24, Lukas Czerner wrote:
>> On Thu, 31 Mar 2011, Jan Kara wrote:
>>
>> > On Wed 30-03-11 17:32:06, Lukas Czerner wrote:
>> > > Hello,
>> > >
>> > > I have hit BUG_ON while running xfstest 234 in the loop
>> > > on the ext4. Backtrace below .
>> > >
>> > > I thought that this problem was solved with
>> > > 67eeb5685d2a211c0252ac7884142e503c759500 however it is still present.
>> > > I might be a bit hard to hit, but once you run
>> > >
>> > > while true; do sync; sleep 0.3; done
>> > >
>> > > concurrently it is reproducible almost all the time. I have tried to
>> > > understand what is going on but only thing I can see is this course
>> > > of action:
>> > >
>> > > ext4_write_dquot
>> > >   ext4_journal_start <- h_buffer_credits = 2
>> > >   dquot_commit
>> > >     v2_write_dquot
>> > >       qtree_write_dquot
>> > >         ext4_quota_write
>> > >           ext4_handle_dirty_metadata      <- this takes one block reserved
>> > >                                      for journal
>> > >                                      h_buffer_credits--
>> > >     v2_write_file_info
>> > >       ext4_quota_write
>> > >         ext4_handle_dirty_metadata        <- this takes second block reserved
>> > >                                      for journal
>> > >                                      h_buffer_credits--
>> > >   ext4_journal_stop
>> > >
>> > > However apparently I am missing something, because according to the
>> > > backtrace the second call of jbd2_journal_dirty_metadata() may end up
>> > > with BUG_ON -> J_ASSERT_JH(jh, handle->h_buffer_credits > 0);
>> > >
>> > > So someone else is touching our handle apparently, but I go not exacly
>> > > know what is going on. From my simple debugging printk information I
>> > > have put with before ext4_handle_dirty_metadata() in ext4_quota_write()
>> > > we can see:
>> > >
>> > > [  226.450263] ext4_quota_write: len 48, off 5136, type 0 h_ref 1
>> > > credits 2
>> > > [  226.458950] ext4_quota_write: len 24, off 8, type 0 h_ref 1 credits 0
>> > >
>> > > It is clear someone changed handle between v2_write_dquot() and
>> > > v2_write_file_info() does anyone have idea what is going on ?
>> >   It's simpler than that I believe. ext4_write_dquot() does also
>> >         inode->i_mtime = inode->i_ctime = CURRENT_TIME;
>> >         ext4_mark_inode_dirty(handle, inode);
>> > and that eats another credit. Looking at the comment before
>> > EXT4_QUOTA_TRANS_BLOCKS, we in fact counted with inode and data update but
>> > didn't count with the info update. It's actually a race that we ended up
>> > doing info update in our transaction - someone marked info dirty and before
>> > he managed to write it, we went in, saw the dirty flag and wrote it for
>> > him. So the attached patch should fix the problem for you.
>> >
>>
>> The patch fixes the problem, you can add
>>
>> Reported-and-tested-by: Lukas Czerner <lczerner@...hat.com>
>>
>> Ted, could you pick this up ASAP ? Also I think this needs to go into
>> stable as well.
>  I'll merge it from my tree next week, OK? I don't think there's so much
> hurry as the bug has been there for years and you're the first one to
> report it :).
>
> BTW, I've also created the patch below which can slightly improve
> performance of quotas on ext4. The modification of the inode was
> unnecessary as well... Ted, can you merge this patch? It's independent from
> the previous one.
>
>                                                                Honza
> --
> From 51741760fe3682ea37bd264471f897bda9d5bd6d Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@...e.cz>
> Date: Thu, 31 Mar 2011 23:12:14 +0200
> Subject: [PATCH] ext4: Remove unnecessary [cm]time update of quota file
>
> It is not necessary to update [cm]time of quota file on each quota file
> write and it wastes journal space and IO throughput with inode writes. So
> just remove the updating from ext4_quota_write() and only update times when
> quotas are being turned off. Userspace cannot get anything reliable from
> quota files while they are used by the kernel anyway.
>
> Signed-off-by: Jan Kara <jack@...e.cz>
> ---
>  fs/ext4/ext4_jbd2.h |    4 ++--
>  fs/ext4/super.c     |   16 ++++++++++++++--
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index d8b992e..3163583 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -86,8 +86,8 @@
>
>  #ifdef CONFIG_QUOTA
>  /* Amount of blocks needed for quota update - we know that the structure was
> - * allocated so we need to update only inode+data */
> -#define EXT4_QUOTA_TRANS_BLOCKS(sb) (test_opt(sb, QUOTA) ? 2 : 0)
> + * allocated so we need to update only data block */
> +#define EXT4_QUOTA_TRANS_BLOCKS(sb) (test_opt(sb, QUOTA) ? 1 : 0)

Cool :-)
Yongqiang,
Now we only need to account for 2 (user+group) snapshot quota credits for COW.

>  /* Amount of blocks needed for quota insert/delete - we do some block writes
>  * but inode, sb and group updates are done only once */
>  #define EXT4_QUOTA_INIT_BLOCKS(sb) (test_opt(sb, QUOTA) ? (DQUOT_INIT_ALLOC*\
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 1f0acd4..baee1b4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4608,11 +4608,24 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>
>  static int ext4_quota_off(struct super_block *sb, int type)
>  {
> +       struct inode *inode = sb_dqopt(sb)->files[type];
> +       handle_t *handle;
> +
>        /* Force all delayed allocation blocks to be allocated.
>         * Caller already holds s_umount sem */
>        if (test_opt(sb, DELALLOC))
>                sync_filesystem(sb);
>
> +       /* Update modification times of quota files when userspace can
> +        * start looking at them */
> +       handle = ext4_journal_start(inode, 1);
> +       if (IS_ERR(handle))
> +               goto out;
> +       inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> +       ext4_mark_inode_dirty(handle, inode);
> +       ext4_journal_stop(handle);
> +
> +out:
>        return dquot_quota_off(sb, type);
>  }
>
> @@ -4708,9 +4721,8 @@ out:
>        if (inode->i_size < off + len) {
>                i_size_write(inode, off + len);
>                EXT4_I(inode)->i_disksize = inode->i_size;
> +               ext4_mark_inode_dirty(handle, inode);
>        }
> -       inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> -       ext4_mark_inode_dirty(handle, inode);
>        mutex_unlock(&inode->i_mutex);
>        return len;
>  }
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists