On Oct 18, 2017, at 11:36 AM, Dmitry Monakhov <dmonakhov@xxxxxxxxxx> wrote:
>
>
> ->s_next_generation is protected by s_next_gen_lock but it usage
> pattern is very primitive and can be replaced with atomic_ops
>
> This significantly improve creation/unlink scenario on SMP systems,
> for example lat_fs_create_unlink test [1] on x2 E5-2680 (32vcpu) system
> shows ~20% improvement.
> | nr_tsk | wo/ patch | w/ patch |
> |--------+-----------+----------|
> | 1 | 137 | 140 |
> | 2 | 224 | 233 |
> | 4 | 356 | 372 |
> | 8 | 439 | 519 |
> | 16 | 443 | 585 |
> | 32 | 598 | 695 |
> | 64 | 559 | 707 |
> | 128 | 385 | 437 |
Strictly speaking, we don't need a single global value for i_generation.
These are per-inode values, and just need to be relatively unique compared
to previous values for each inode. There are also potential security
benefits from not having sequential i_generation numbers, since that makes
NFS file handle guessing a lot harder.
You could just increment the previous i_generation value for that inode
(if the old inode was read from disk), or generate a random number (also
likely to be CPU expensive and risk collisions), or use a per-CPU counter
(with some way to ensure threads allocating/freeing inodes on different
cores do not allocate the same generation in close proximity), like:
cpuid = smp_processor_id();
i_generation = sb->s_generation[cpuid] | cpuid;
sb->s_generation[cpuid] += num_possible_cpus();
or whatever is fastest. The above doesn't even need {get,put}_cpu(),
since it doesn't matter if there is a race in the update.
Since the locking of this one field shows up at a macro level, it would
be interesting if you took out the assignment completely, to see if this
shows further improvements, which would indicate we can still come up with
a better solution than the atomic you have proposed here.
Cheers, Andreas
> Footnotes:
> [1]https://github.com/dmonakhov/lmbench/blob/master/src/lat_fs_create_unlink.c
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
> ---
> fs/ext4/ext4.h | 3 +--
> fs/ext4/ialloc.c | 4 +---
> fs/ext4/ioctl.c | 6 ++----
> fs/ext4/super.c | 8 ++++----
> 4 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index e2abe01..6be1aa8 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1392,8 +1392,7 @@ struct ext4_sb_info {
> int s_first_ino;
> unsigned int s_inode_readahead_blks;
> unsigned int s_inode_goal;
> - spinlock_t s_next_gen_lock;
> - u32 s_next_generation;
> + atomic_t s_next_generation;
> u32 s_hash_seed[4];
> int s_def_hash_version;
> int s_hash_unsigned; /* 3 if hash should be signed, 0 if not */
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index ee82302..d12dabc 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1138,9 +1138,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
> inode->i_ino);
> goto out;
> }
> - spin_lock(&sbi->s_next_gen_lock);
> - inode->i_generation = sbi->s_next_generation++;
> - spin_unlock(&sbi->s_next_gen_lock);
> + inode->i_generation = atomic_inc_return(&sbi->s_next_generation);
>
> /* Precompute checksum seed for inode metadata */
> if (ext4_has_metadata_csum(sb)) {
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index afb66d4..7d8b1a5 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -157,10 +157,8 @@ static long swap_inode_boot_loader(struct super_block *sb,
>
> inode->i_ctime = inode_bl->i_ctime = current_time(inode);
>
> - spin_lock(&sbi->s_next_gen_lock);
> - inode->i_generation = sbi->s_next_generation++;
> - inode_bl->i_generation = sbi->s_next_generation++;
> - spin_unlock(&sbi->s_next_gen_lock);
> + inode_bl->i_generation = atomic_add_return(2, &sbi->s_next_generation);
> + inode->i_generation = inode_bl->i_generation -1;
>
> ext4_discard_preallocations(inode);
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b104096..bfc6d2e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3419,7 +3419,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> int err = 0;
> unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> ext4_group_t first_not_zeroed;
> -
> + u32 igen;
> +
> if ((data && !orig_data) || !sbi)
> goto out_free_base;
>
> @@ -3977,9 +3978,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> }
>
> sbi->s_gdb_count = db_count;
> - get_random_bytes(&sbi->s_next_generation, sizeof(u32));
> - spin_lock_init(&sbi->s_next_gen_lock);
> -
> + get_random_bytes(&igen, sizeof(u32));
> + atomic_set(&sbi->s_next_generation, igen);
> setup_timer(&sbi->s_err_report, print_daily_error_info,
> (unsigned long) sb);
>
> --
> 1.8.3.1
>
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
