linux-ext4 - Re: [PATCH 1/8 bigalloc] ext4: get blocks from ext4_ext_get_actual

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:	Wed, 2 Nov 2011 12:29:12 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Robin Dong <hao.bigrat@...il.com>
Cc:	linux-ext4@...r.kernel.org, Robin Dong <sanbai@...bao.com>
Subject: Re: [PATCH 1/8 bigalloc] ext4: get blocks from ext4_ext_get_actual_len

On 2011-11-01, at 4:53 AM, Robin Dong wrote:
> From: Robin Dong <sanbai@...bao.com>
> 
> Since ee_len's unit change to cluster, it need to transform from clusters
> to blocks when use ext4_ext_get_actual_len.

Robin,
thanks for working on and submitting these patches so quickly.

> struct ext4_extent {
> 	__le32	ee_block;	/* first logical block extent covers */
> -	__le16	ee_len;		/* number of blocks covered by extent */
> +	__le16	ee_len;		/* number of clusters covered by extent */

It would make sense that ee_block should also be changed to be measured
in units of clusters instead of blocks, since there is no value to
using extents with cluster size if they are not also cluster aligned.

I think this would also simplify some of the code.

> static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
> {
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);

Why allocate "*sbi" on the stack in all of these functions for a
single use?  This provides no benefit, but can increase the stack
usage considerably due to repeated allocations.

> 	ext4_fsblk_t block = ext4_ext_pblock(ext);
> +	int len = EXT4_C2B(sbi, ext4_ext_get_actual_len(ext));

It probably makes more sense to pass "sb" or "sbi" as a parameter to 
ext4_ext_get_actual_len() and then have it return the proper length
in blocks (i.e. call EXT4_C2B() internally), which will simplify all
of the callers and avoid potential bugs if some code does not use it.

> @@ -1523,7 +1534,7 @@ ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
> 	ext1_ee_len = ext4_ext_get_actual_len(ex1);
> 	ext2_ee_len = ext4_ext_get_actual_len(ex2);
> 
> -	if (le32_to_cpu(ex1->ee_block) + ext1_ee_len !=
> +	if (le32_to_cpu(ex1->ee_block) + EXT4_C2B(sbi, ext1_ee_len) !=
> 			le32_to_cpu(ex2->ee_block))

If both ee_len and ee_block are in the same units (blocks or clusters),
then there is no need to convert units for this function at all.

Cheers, Andreas

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