linux-ext4 - Re: [PATCH][RFC] ext4: avoid taking down_read(&grp->alloc

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: <AANLkTimgCcW0fQzQ3W=vhkTSWt=0=xPoaBucrM=mfHXG@mail.gmail.com>
Date:	Mon, 14 Feb 2011 09:52:25 +0200
From:	"Amir G." <amir73il@...rs.sourceforge.net>
To:	Theodore Tso <tytso@....edu>
Cc:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH][RFC] ext4: avoid taking down_read(&grp->alloc_sem)

Hi Ted,

I have checked myself over and over again and I always end up at the
same conclusion:

grp->alloc_sem is NOT NEEDED after buddy of non-last group has been initialized!

I see you have been busy with cleaning up the buffered write submissions path,
removing unneeded locks among other improvements.

I would help reviewing your patches, but I find following delayed
allocation code
way more complicated than following mballoc code ;-).

Anyway, if you have some time now, please try to verify my (simple)
patch, which:
"should improve performance and improve SMP scalability ...
 in the best way possible --- don't take locks when they aren't needed!"

I believe that in some workloads the buddy cache can be frequently loaded
without eventually allocating blocks from that group (no matching
extent found etc)
and it is in those  workloads, where we should see the most benefit
from not taking
grp->alloc_sem on mb_load_buddy().

Amir.

On Wed, Feb 9, 2011 at 12:05 PM, Amir G. <amir73il@...rs.sourceforge.net> wrote:
> Hi Aneesh,
>
> As you are signed off on most of the recent alloc_sem related code changes,
> can you please comment on the patch below, which tries to avoid taking
> the read lock most of the times on a 4K block fs.
>
> Can anyone tell what performance impact (if any) will be caused by avoiding
> the read lock on most allocations? group spin lock will still be taken, but for
> much shorter periods of time (cycles).
>
> Any ideas how this patch can be properly tested?
>
> Thanks,
> Amir.
>
> grp->alloc_sem is used to synchronize buddy cache users with buddy cache init
> of other groups that use the same buddy cache page and with adding blocks to
> group on online resize.
>
> When blocks_per_page <= 2, each group has it's own private buddy cache page
> so taking the read lock for every allocation is futile and can be avoided for
> every group, but the last one.
>
> The write lock is taken in ext4_mb_init_group() and in ext4_add_groupblocks()
> to synchronize the buddy cache init of a group on first time allocation after
> mount and after extending the last group.
>
> Signed-off-by: Amir Goldstein <amir73il@...rs.sf.net>
> ---
>  fs/ext4/mballoc.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 1b3256b..22a5251 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1160,7 +1160,15 @@ ext4_mb_load_buddy(struct super_block *sb,
> ext4_group_t group,
>        e4b->bd_group = group;
>        e4b->bd_buddy_page = NULL;
>        e4b->bd_bitmap_page = NULL;
> -       e4b->alloc_semp = &grp->alloc_sem;
> +       /*
> +        * We only need to take the read lock if other groups share the buddy
> +        * page with this group or if blocks may be added to this (last) group
> +        * by ext4_group_extend().
> +        */
> +       if (blocks_per_page > 2 || group == sbi->s_groups_count - 1)
> +               e4b->alloc_semp = &grp->alloc_sem;
> +       else
> +               e4b->alloc_semp = NULL;
>
>        /* Take the read lock on the group alloc
>         * sem. This would make sure a parallel
> @@ -1169,7 +1177,8 @@ ext4_mb_load_buddy(struct super_block *sb,
> ext4_group_t group,
>         * till we are done with allocation
>         */
>  repeat_load_buddy:
> -       down_read(e4b->alloc_semp);
> +       if (e4b->alloc_semp)
> +               down_read(e4b->alloc_semp);
>
>        if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
>                /* we need to check for group need init flag
> @@ -1177,7 +1186,8 @@ repeat_load_buddy:
>                 * that new blocks didn't get added to the group
>                 * when we are loading the buddy cache
>                 */
> -               up_read(e4b->alloc_semp);
> +               if (e4b->alloc_semp)
> +                       up_read(e4b->alloc_semp);
>                /*
>                 * we need full data about the group
>                 * to make a good selection
> @@ -1277,7 +1287,8 @@ err:
>        e4b->bd_bitmap = NULL;
>
>        /* Done with the buddy cache */
> -       up_read(e4b->alloc_semp);
> +       if (e4b->alloc_semp)
> +               up_read(e4b->alloc_semp);
>        return ret;
>  }
>
> --
> 1.7.0.4
>
--
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