linux-ext4 - Re: [PATCH] ext4: Always journal quota file modifications

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100602234658.GA26830@quack.suse.cz>
Date:	Thu, 3 Jun 2010 01:46:59 +0200
From:	Jan Kara <jack@...e.cz>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	Jan Kara <jack@...e.cz>, tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: Always journal quota file modifications

On Wed 02-06-10 16:39:59, Eric Sandeen wrote:
> Jan Kara wrote:
> > When journaled quota options are not specified, we do writes
> > to quota files just in data=ordered mode. This actually causes
> > warnings from JBD2 about dirty journaled buffer because ext4_getblk
> > unconditionally treats a block allocated by it as metadata. Since
> > quota actually is filesystem metadata, the easiest way to get rid
> > of the warning is to always treat quota writes as metadata...
> > 
> > Signed-off-by: Jan Kara <jack@...e.cz>
> 
> Seems like a good approach to me, thanks Jan.
> 
> (aside: how big an impact does this have on journal traffic for a busy
> quota-controlled fs?)
  With quota a bit tricky thing is that amount of load depends on the
amount of different groups / users doing IO (and to some extent on how
much these users / groups share blocks of quota file). One can make a
simplistic upper estimate: 1 journalled block per user / group who modified
something in a transaction...
  Just out of curiosity I have created 10000 files in 10 directories, each
file belonging to a different user. Then I run:
sync; time { for (( i = 0; i < 10000; i++ )); do echo "aaaaaaaa"
>dir$((i/1000))/file$((i%1000)); done; sync; }

and

sync; time { for (( i = 0; i < 10000; i++ )); do
>dir$((i/1000))/file$((i%1000)); done; sync; }

  With journaled quotas enabled the times were pretty consistently around
2.9s for writes and 2.1s for truncates, without quotas the times were
around 2.7s for writes and 1.7s for truncates. So this gives some rough
idea how much quota costs for this rather extreme use case.

  Another case where quota impact is measurable is when several threads
bang files of the same user. Then we hit lock contention on the single dquot
structure, most notably on journaling it whenever it gets modified. Dmitry
was looking into addressing this...

									Honza

> Reviewed-by: Eric Sandeen <sandeen@...hat.com>
> 
> > ---
> >  fs/ext4/super.c |   19 +++++--------------
> >  1 files changed, 5 insertions(+), 14 deletions(-)
> > 
> >   Ted, this patch fixes some JBD2 warning for me when running XFSQA
> > with quotas enabled. I think this is a move into a direction you are
> > trying to achieve as well. Will you merge the patch or should I do it?
> > 
> > 								Honza
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 4e8983a..a8cea08 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -4030,7 +4030,6 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> >  	ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
> >  	int err = 0;
> >  	int offset = off & (sb->s_blocksize - 1);
> > -	int journal_quota = EXT4_SB(sb)->s_qf_names[type] != NULL;
> >  	struct buffer_head *bh;
> >  	handle_t *handle = journal_current_handle();
> >  
> > @@ -4055,24 +4054,16 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> >  	bh = ext4_bread(handle, inode, blk, 1, &err);
> >  	if (!bh)
> >  		goto out;
> > -	if (journal_quota) {
> > -		err = ext4_journal_get_write_access(handle, bh);
> > -		if (err) {
> > -			brelse(bh);
> > -			goto out;
> > -		}
> > +	err = ext4_journal_get_write_access(handle, bh);
> > +	if (err) {
> > +		brelse(bh);
> > +		goto out;
> >  	}
> >  	lock_buffer(bh);
> >  	memcpy(bh->b_data+offset, data, len);
> >  	flush_dcache_page(bh->b_page);
> >  	unlock_buffer(bh);
> > -	if (journal_quota)
> > -		err = ext4_handle_dirty_metadata(handle, NULL, bh);
> > -	else {
> > -		/* Always do at least ordered writes for quotas */
> > -		err = ext4_jbd2_file_inode(handle, inode);
> > -		mark_buffer_dirty(bh);
> > -	}
> > +	err = ext4_handle_dirty_metadata(handle, NULL, bh);
> >  	brelse(bh);
> >  out:
> >  	if (err) {
> 
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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