| 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
| ||
|
Message-ID: <CFC260CA0A89534E9741BAB66E7F46EC01DEA0E5DF@AUSX7MCPC110.AMER.DELL.COM>
Date: Thu, 4 Jul 2013 11:06:51 -0500
From: <Jose_Mario_Gallegos@...l.com>
To: <lczerner@...hat.com>, <jack@...e.cz>
CC: <linux-ext4@...r.kernel.org>, <enwlinux@...il.com>,
<Jordan_Hargrave@...l.com>, <rwheeler@...hat.com>
Subject: RE: [RFC PATCH 1/1] ext4: Try to better reuse recently freed space
Thank you very much for looking into this issue Lukas!
Ric,
What are the odds of getting a patch as a hot fix or beta for a proof of concept?
The deadline is 7/15/2013 and this may eliminate the biggest remaining issue.
Thanks again and best regards,
Mario
> -----Original Message-----
> From: Lukáš Czerner [mailto:lczerner@...hat.com]
> Sent: Thursday, July 04, 2013 10:33 AM
> To: Jan Kara
> Cc: linux-ext4@...r.kernel.org; enwlinux@...il.com; Gallegos, Mario;
> Hargrave, Jordan; rwheeler@...hat.com
> Subject: Re: [RFC PATCH 1/1] ext4: Try to better reuse recently freed space
>
> On Thu, 4 Jul 2013, Jan Kara wrote:
>
> > Date: Thu, 4 Jul 2013 17:09:05 +0200
> > From: Jan Kara <jack@...e.cz>
> > To: Lukas Czerner <lczerner@...hat.com>
> > Cc: linux-ext4@...r.kernel.org, enwlinux@...il.com,
> > Jose_Mario_Gallegos@...l.com, jordan_hargrave@...l.com,
> > rwheeler@...hat.com
> > Subject: Re: [RFC PATCH 1/1] ext4: Try to better reuse recently freed
> > space
> >
> > On Thu 04-07-13 11:11:54, Lukas Czerner wrote:
> > > Currently if the block allocator can not find the goal to allocate
> > > we would use global goal for stream allocation. However the global
> > > goal (s_mb_last_group and s_mb_last_start) will move further every
> > > time such allocation appears and never move backwards.
> > >
> > > This causes several problems in certain scenarios:
> > >
> > > - the goal will move further and further preventing us from reusing
> > > space which might have been freed since then. This is ok from the file
> > > system point of view because we will reuse that space eventually,
> > > however we're allocating block from slower parts of the spinning disk
> > > even though it might not be necessary.
> > > - The above also causes more serious problem for example for thinly
> > > provisioned storage (sparse images backed storage as well), because
> > > instead of reusing blocks which are already provisioned we would try
> > > to use new blocks. This would unnecessarily drain storage free blocks
> > > pool.
> > > - This will also cause blocks to be allocated further from the given
> > > goal than it's necessary. Consider for example truncating, or removing
> > > and rewriting the file in the loop. This workload will never reuse
> > > freed blocks until we continually claim and free all the block in the
> > > file system.
> > >
> > > Note that file systems like xfs, ext3, or btrfs does not have this
> > > problem. This is simply caused by the notion of global pool.
> > >
> > > Fix this by changing the global goal to be goal per inode. This will
> > > allow us to invalidate the goal every time the inode has been
> > > truncated, or newly created, so in those cases we would try to use
> > > the proper more specific goal which is based on inode position.
> > In principle the patch looks fine to me. Just minor style nit below:
> >
> > >
> > > Signed-off-by: Lukas Czerner <lczerner@...hat.com>
> > > ---
> > > fs/ext4/ext4.h | 7 ++++---
> > > fs/ext4/inode.c | 8 ++++++++
> > > fs/ext4/mballoc.c | 20 ++++++++------------
> > > 3 files changed, 20 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 6ed348d..4dffa92
> > > 100644
> > > --- a/fs/ext4/ext4.h
> > > +++ b/fs/ext4/ext4.h
> > > @@ -917,6 +917,10 @@ struct ext4_inode_info {
> > >
> > > /* Precomputed uuid+inum+igen checksum for seeding inode
> checksums */
> > > __u32 i_csum_seed;
> > > +
> > > + /* where last allocation was done - for stream allocation */
> > > + unsigned long i_last_group;
> > > + unsigned long i_last_start;
> > Can we use proper types here please? I think they should be
> > ext4_grpblk_t and ext4_group_t. And maybe we should define something
> > like EXT4_INVAL_GRPNO and use that instead of UINT_MAX.
>
> Sure, it'll look better with named types and defined invalidate value. I'll
> resend the patch soon.
>
> Thanks!
> -Lukas
>
> >
> > Honza
> > > };
> > >
> > > /*
> > > @@ -1242,9 +1246,6 @@ struct ext4_sb_info {
> > > unsigned int s_mb_order2_reqs;
> > > unsigned int s_mb_group_prealloc;
> > > unsigned int s_max_dir_size_kb;
> > > - /* where last allocation was done - for stream allocation */
> > > - unsigned long s_mb_last_group;
> > > - unsigned long s_mb_last_start;
> > >
> > > /* stats for buddy allocator */
> > > atomic_t s_bal_reqs; /* number of reqs with len > 1 */
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index
> > > 0188e65..07d0434 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -3702,6 +3702,10 @@ void ext4_truncate(struct inode *inode)
> > > else
> > > ext4_ind_truncate(handle, inode);
> > >
> > > + /* Invalidate last allocation counters */
> > > + ei->i_last_group = UINT_MAX;
> > > + ei->i_last_start = UINT_MAX;
> > > +
> > And
> > > up_write(&ei->i_data_sem);
> > >
> > > if (IS_SYNC(inode))
> > > @@ -4060,6 +4064,10 @@ struct inode *ext4_iget(struct super_block
> *sb, unsigned long ino)
> > > inode->i_generation = le32_to_cpu(raw_inode->i_generation);
> > > ei->i_block_group = iloc.block_group;
> > > ei->i_last_alloc_group = ~0;
> > > +
> > > + /* Invalidate last allocation counters */
> > > + ei->i_last_group = UINT_MAX;
> > > + ei->i_last_start = UINT_MAX;
> > > /*
> > > * NOTE! The in-memory inode i_data array is in little-endian order
> > > * even on big-endian machines: we do NOT byteswap the block
> numbers!
> > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index
> > > a9ff5e5..6c23666 100644
> > > --- a/fs/ext4/mballoc.c
> > > +++ b/fs/ext4/mballoc.c
> > > @@ -1591,7 +1591,6 @@ static int mb_mark_used(struct ext4_buddy
> > > *e4b, struct ext4_free_extent *ex) static void
> ext4_mb_use_best_found(struct ext4_allocation_context *ac,
> > > struct ext4_buddy *e4b)
> > > {
> > > - struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> > > int ret;
> > >
> > > BUG_ON(ac->ac_b_ex.fe_group != e4b->bd_group); @@ -1622,10
> +1621,8
> > > @@ static void ext4_mb_use_best_found(struct
> ext4_allocation_context *ac,
> > > get_page(ac->ac_buddy_page);
> > > /* store last allocated for subsequent stream allocation */
> > > if (ac->ac_flags & EXT4_MB_STREAM_ALLOC) {
> > > - spin_lock(&sbi->s_md_lock);
> > > - sbi->s_mb_last_group = ac->ac_f_ex.fe_group;
> > > - sbi->s_mb_last_start = ac->ac_f_ex.fe_start;
> > > - spin_unlock(&sbi->s_md_lock);
> > > + EXT4_I(ac->ac_inode)->i_last_group = ac-
> >ac_f_ex.fe_group;
> > > + EXT4_I(ac->ac_inode)->i_last_start = ac->ac_f_ex.fe_start;
> > > }
> > > }
> > >
> > > @@ -2080,13 +2077,12 @@ ext4_mb_regular_allocator(struct
> ext4_allocation_context *ac)
> > > ac->ac_2order = i - 1;
> > > }
> > >
> > > - /* if stream allocation is enabled, use global goal */
> > > - if (ac->ac_flags & EXT4_MB_STREAM_ALLOC) {
> > > - /* TBD: may be hot point */
> > > - spin_lock(&sbi->s_md_lock);
> > > - ac->ac_g_ex.fe_group = sbi->s_mb_last_group;
> > > - ac->ac_g_ex.fe_start = sbi->s_mb_last_start;
> > > - spin_unlock(&sbi->s_md_lock);
> > > + /* if stream allocation is enabled and per inode goal is
> > > + * set, use it */
> > > + if ((ac->ac_flags & EXT4_MB_STREAM_ALLOC) &&
> > > + (EXT4_I(ac->ac_inode)->i_last_start != UINT_MAX)) {
> > > + ac->ac_g_ex.fe_group = EXT4_I(ac->ac_inode)-
> >i_last_group;
> > > + ac->ac_g_ex.fe_start = EXT4_I(ac->ac_inode)->i_last_start;
> > > }
> > >
> > > /* Let's just scan groups to find more-less suitable blocks */
> > > --
> > > 1.8.3.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
> >
--
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