linux-ext4 - Re: [PATCH] ext4: fix online resize with mballoc

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]
Date:	Thu, 26 Jun 2008 17:26:59 -0600
From:	Andreas Dilger <adilger@....com>
To:	Frédéric Bohé <frederic.bohe@...l.net>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: fix online resize with mballoc

On Jun 26, 2008  16:58 +0200, Frďż˝dďż˝ric Bohďż˝ wrote:
> From: Frederic Bohe <frederic.bohe@...l.net>
> 
> Update group infos when updating a group's descriptor.
> Add group infos when adding a group's descriptor.
> Refresh cache pages used by mb_alloc when changes occur.

Excellent, thank you for fixing this issue.  Minor comments below.

> +/* Create and initialize ext4_group_info data */
> +/* for the given group. */
> +
> +int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
> +struct ext4_group_desc *desc)

Please indent the continued line to match '(' on previous line.

> +	if (group % EXT4_DESC_PER_BLOCK(sb) == 0) {
> +		metalen = sizeof(*meta_group_info)
> +			<< EXT4_DESC_PER_BLOCK_BITS(sb);

Please put operator "<<" at end of previous line.

> +		meta_group_info = kmalloc(metalen, GFP_KERNEL);
> +		if (meta_group_info == NULL) {
> +			printk(KERN_ERR "EXT4-fs: can't allocate mem for a "
> +			"buddy group\n");

Indent continued line to '('

> +			goto exit_meta_group_info;
> +		}
> +		sbi->s_group_info[group >>
> +			EXT4_DESC_PER_BLOCK_BITS(sb)] = meta_group_info;

Prefer keeping related operations on the same line, like:

		sbi->s_group_info[group >> EXT4_DESC_PER_BLOCK_BITS(sb)] =
			meta_group_info;

> +	/*
> +	 * calculate needed size. if change bb_counters size,
> +	 * don't forget about ext4_mb_generate_buddy()
> +	 */
> +	len = sizeof(struct ext4_group_info);
> +	len += sizeof(unsigned short) * (sb->s_blocksize_bits + 2);

It would be good to use an actual structure and not "sizeof(unsigned short)"
because if the structure changes then this calculation will be wrong.
Something like the following is much safer and will always be correct:
	
	len = offsetof(typeof(*meta_group_info),
		       bb_counters[sb->s_blocksize_bits + 2]);

I see that your code is copied from ext4_mb_init_backend(), so fault is
not yours, but it is still good to make the code safe against future bugs.

> +	/*
> +	 * initialize bb_free to be able to skip
> +	 * empty groups without initialization
> +	 */
> +	if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> +		meta_group_info[i]->bb_free =
> +		ext4_free_blocks_after_init(sb, group, desc);
> +	} else {
> +		meta_group_info[i]->bb_free =
> +		le16_to_cpu(desc->bg_free_blocks_count);
> +	}

Please indent continued lines.

> +	INIT_LIST_HEAD(&meta_group_info[i]->bb_prealloc_list);
> +
> +#ifdef DOUBLE_CHECK
> +	{
> +	struct buffer_head *bh;
> +	meta_group_info[i]->bb_bitmap =
> +		kmalloc(sb->s_blocksize, GFP_KERNEL);
> +	BUG_ON(meta_group_info[i]->bb_bitmap == NULL);
> +	bh = ext4_read_block_bitmap(sb, group);
> +	BUG_ON(bh == NULL);
> +	memcpy(meta_group_info[i]->bb_bitmap, bh->b_data,
> +		sb->s_blocksize);
> +	put_bh(bh);
> +	}

Please indent inside this scope.

> +#endif
> +
> +	return 0;
> +exit_group_info:

Blank line after "return"

> +int ext4_mb_add_more_groupinfo(struct super_block *sb, ext4_group_t group,
> +				struct ext4_group_desc *desc)
> +{
> +	/* Cache pages containing dynamic mb_alloc datas (buddy and bitmap
> +	* datas) are set not up to date so that they will be re-initilaized
> +	* during the next call to ext4_mb_load_buddy
> +	*/

Comment continuation should line up '*' in a single column.

>  static int ext4_mb_init_backend(struct super_block *sb)
>  {
> +	/* This is the total number of blocks used by GDT including
> +	* the number of reserved blocks for GDT.
> +	* The s_group_info array is allocated with this value
> +	* to allow a clean online resize without a complex
> +	* manipulation of pointer.
> +	* The drawback is the unused memory when no resize
> +	* occurs but it's very low in terms of pages
> +	* (see comments below)
> +	*/

Also align comment '*' in the same columns.

> +	num_meta_group_infos_max = num_meta_group_infos +
> +				le16_to_cpu(es->s_reserved_gdt_blocks);

The only drawback of NOT handling this properly is that once (eventually)
we allow resizing with META_BG this code will be broken again.  It at
least deserves a comment like "Need to handle this properly when META_BG
resizing is allowed" so that it will show up on any grep for META_BG.

It probably also makes sense to round this up to the next power-of-two
value, since kmalloc will do that internally anyways, and it gives us
some resizing headroom for no cost.

> @@ -2276,62 +2431,15 @@ static int ext4_mb_init_backend(struct s
>  		sbi->s_group_info[i] = meta_group_info;
>  	}
>  
>  	for (i = 0; i < sbi->s_groups_count; i++) {
> +		if (ext4_mb_add_groupinfo(sb, i, desc) != 0)
> +			goto err_freebuddy;

This is much nicer, great.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
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