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: <alpine.LFD.2.00.1407140959450.13527@localhost.localdomain>
Date:	Mon, 14 Jul 2014 10:04:47 +0200 (CEST)
From:	Lukáš Czerner <lczerner@...hat.com>
To:	"Theodore Ts'o" <tytso@....edu>
cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH -v2] ext4: rearrange initialization to fix EXT4FS_DEBUG

On Sat, 12 Jul 2014, Theodore Ts'o wrote:

> Date: Sat, 12 Jul 2014 12:10:15 -0400
> From: Theodore Ts'o <tytso@....edu>
> To: Ext4 Developers List <linux-ext4@...r.kernel.org>
> Cc: Theodore Ts'o <tytso@....edu>
> Subject: [PATCH -v2] ext4: rearrange initialization to fix EXT4FS_DEBUG
> 
> The EXT4FS_DEBUG is a *very* developer specific #ifdef designed for
> ext4 developers only.  (You have to modify fs/ext4/ext4.h to enable
> it.)
> 
> Rearrange how we initialize data structures to avoid calling
> ext4_count_free_clusters() until the multiblock allocator has been
> initialized.
> 
> This also allows us to only call ext4_count_free_clusters() once, and
> simplifies the code somewhat.
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> ---
> 
> [ This will teach me not to try to push out a patch in a hurry before
>   running to catch an airplane...  and again, I don't plan to include
>   this until the next merge window.  -- Ted ]
> 
>  fs/ext4/super.c | 87 +++++++++++++++++++++++++--------------------------------
>  1 file changed, 38 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6df7bc6..30767cd 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2142,10 +2142,6 @@ static int ext4_check_descriptors(struct super_block *sb,
>  	}
>  	if (NULL != first_not_zeroed)
>  		*first_not_zeroed = grp;
> -
> -	ext4_free_blocks_count_set(sbi->s_es,
> -				   EXT4_C2B(sbi, ext4_count_free_clusters(sb)));
> -	sbi->s_es->s_free_inodes_count =cpu_to_le32(ext4_count_free_inodes(sb));
>  	return 1;
>  }
>  
> @@ -3883,13 +3879,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  		ext4_msg(sb, KERN_ERR, "group descriptors corrupted!");
>  		goto failed_mount2;
>  	}
> -	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
> -		if (!ext4_fill_flex_info(sb)) {
> -			ext4_msg(sb, KERN_ERR,
> -			       "unable to initialize "
> -			       "flex_bg meta info!");
> -			goto failed_mount2;
> -		}
>  
>  	sbi->s_gdb_count = db_count;
>  	get_random_bytes(&sbi->s_next_generation, sizeof(u32));
> @@ -3902,23 +3891,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	/* Register extent status tree shrinker */
>  	ext4_es_register_shrinker(sbi);
>  
> -	err = percpu_counter_init(&sbi->s_freeclusters_counter,
> -			ext4_count_free_clusters(sb));
> -	if (!err) {
> -		err = percpu_counter_init(&sbi->s_freeinodes_counter,
> -				ext4_count_free_inodes(sb));
> -	}
> -	if (!err) {
> -		err = percpu_counter_init(&sbi->s_dirs_counter,
> -				ext4_count_dirs(sb));
> -	}
> -	if (!err) {
> -		err = percpu_counter_init(&sbi->s_dirtyclusters_counter, 0);
> -	}
> -	if (!err) {
> -		err = percpu_counter_init(&sbi->s_extent_cache_cnt, 0);
> -	}
> -	if (err) {
> +	if ((err = percpu_counter_init(&sbi->s_extent_cache_cnt, 0)) != 0) {
>  		ext4_msg(sb, KERN_ERR, "insufficient memory");
>  		goto failed_mount3;
>  	}
> @@ -4022,18 +3995,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	sbi->s_journal->j_commit_callback = ext4_journal_commit_callback;
>  
> -	/*
> -	 * The journal may have updated the bg summary counts, so we
> -	 * need to update the global counters.
> -	 */
> -	percpu_counter_set(&sbi->s_freeclusters_counter,
> -			   ext4_count_free_clusters(sb));
> -	percpu_counter_set(&sbi->s_freeinodes_counter,
> -			   ext4_count_free_inodes(sb));
> -	percpu_counter_set(&sbi->s_dirs_counter,
> -			   ext4_count_dirs(sb));
> -	percpu_counter_set(&sbi->s_dirtyclusters_counter, 0);
> -
>  no_journal:
>  	if (ext4_mballoc_ready) {
>  		sbi->s_mb_cache = ext4_xattr_create_cache(sb->s_id);
> @@ -4141,6 +4102,32 @@ no_journal:
>  		goto failed_mount5;
>  	}
>  
> +	block = ext4_count_free_clusters(sb);
> +	ext4_free_blocks_count_set(sbi->s_es, block);


This does not seem right. block is in cluster units so we need to
convert.

	ext4_free_blocks_count_set(sbi->s_es, EXT4_C2B(sbi, blocks));

Otherwise it looks good to me.

Thanks!
-Lukas


> +	err = percpu_counter_init(&sbi->s_freeclusters_counter, block);
> +	if (!err) {
> +		unsigned long freei = ext4_count_free_inodes(sb);
> +		sbi->s_es->s_free_inodes_count = cpu_to_le32(freei);
> +		err = percpu_counter_init(&sbi->s_freeinodes_counter, freei);
> +	}
> +	if (!err)
> +		err = percpu_counter_init(&sbi->s_dirs_counter,
> +					  ext4_count_dirs(sb));
> +	if (!err)
> +		err = percpu_counter_init(&sbi->s_dirtyclusters_counter, 0);
> +	if (err) {
> +		ext4_msg(sb, KERN_ERR, "insufficient memory");
> +		goto failed_mount6;
> +	}
> +
> +	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
> +		if (!ext4_fill_flex_info(sb)) {
> +			ext4_msg(sb, KERN_ERR,
> +			       "unable to initialize "
> +			       "flex_bg meta info!");
> +			goto failed_mount6;
> +		}
> +
>  	err = ext4_register_li_request(sb, first_not_zeroed);
>  	if (err)
>  		goto failed_mount6;
> @@ -4215,6 +4202,12 @@ failed_mount7:
>  	ext4_unregister_li_request(sb);
>  failed_mount6:
>  	ext4_mb_release(sb);
> +	if (sbi->s_flex_groups)
> +		ext4_kvfree(sbi->s_flex_groups);
> +	percpu_counter_destroy(&sbi->s_freeclusters_counter);
> +	percpu_counter_destroy(&sbi->s_freeinodes_counter);
> +	percpu_counter_destroy(&sbi->s_dirs_counter);
> +	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
>  failed_mount5:
>  	ext4_ext_release(sb);
>  	ext4_release_system_zone(sb);
> @@ -4233,12 +4226,6 @@ failed_mount_wq:
>  failed_mount3:
>  	ext4_es_unregister_shrinker(sbi);
>  	del_timer_sync(&sbi->s_err_report);
> -	if (sbi->s_flex_groups)
> -		ext4_kvfree(sbi->s_flex_groups);
> -	percpu_counter_destroy(&sbi->s_freeclusters_counter);
> -	percpu_counter_destroy(&sbi->s_freeinodes_counter);
> -	percpu_counter_destroy(&sbi->s_dirs_counter);
> -	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
>  	percpu_counter_destroy(&sbi->s_extent_cache_cnt);
>  	if (sbi->s_mmp_tsk)
>  		kthread_stop(sbi->s_mmp_tsk);
> @@ -4556,11 +4543,13 @@ static int ext4_commit_super(struct super_block *sb, int sync)
>  	else
>  		es->s_kbytes_written =
>  			cpu_to_le64(EXT4_SB(sb)->s_kbytes_written);
> -	ext4_free_blocks_count_set(es,
> +	if (EXT4_SB(sb)->s_freeclusters_counter.counters)
> +		ext4_free_blocks_count_set(es,
>  			EXT4_C2B(EXT4_SB(sb), percpu_counter_sum_positive(
>  				&EXT4_SB(sb)->s_freeclusters_counter)));
> -	es->s_free_inodes_count =
> -		cpu_to_le32(percpu_counter_sum_positive(
> +	if (EXT4_SB(sb)->s_freeinodes_counter.counters)
> +		es->s_free_inodes_count =
> +			cpu_to_le32(percpu_counter_sum_positive(
>  				&EXT4_SB(sb)->s_freeinodes_counter));
>  	BUFFER_TRACE(sbh, "marking dirty");
>  	ext4_superblock_csum_set(sb);
> 
--
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ