linux-ext4 - Re: [Ext4 punch hole 1/5] Ext4 Punch Hole Support: Convert Blocks to Uninit Exts

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:	Mon, 28 Feb 2011 23:42:48 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Allison Henderson <achender@...ux.vnet.ibm.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [Ext4 punch hole 1/5] Ext4 Punch Hole Support: Convert Blocks to Uninit Exts

On 2011-02-28, at 8:08 PM, Allison Henderson wrote:
> This first patch adds a function to convert a range of blocks
> to an uninitialized extent.  This function will
> be used to first convert the blocks to extents before
> punching them out.

This was proposed as a separate function for FALLOCATE by Dave Chinner (based on XFS_IOC_ZERO_RANGE), so this is useful as a standalone function.

> +/*
> + * ext4_split_extents() Splits a given extent at the block "split"
> + * @handle: The journal handle
> + * @inode: The file inode
> + * @split: The block where the extent will be split
> + * @path: The path to the extent
> + * @flags: flags used to insert the new extent
> + */
> +static int ext4_split_extents(handle_t *handle,
> +					struct inode *inode,
> +					ext4_lblk_t split,
> +					struct ext4_ext_path *path,
> +					int flags)

Isn't this already done when converting an uninitialized extent into a regular extent when it is being written to?  Hmm, ext4_ext_convert_to_initialized() is splitting the extent, but it is doing that "by hand".  It might make sense to convert ext4_ext_convert_to_initialized() to use this routine, but this depends on whether it makes the code simpler or not.

> +	/* if the block is not actually in the extent, go to out */
> +	if(split > ee_block+ee_len || split < ee_block)
> +		goto out;

Is this actually needed?

> +	ext_debug("The new block is %llu, the orig block is: %llu\n", newblock, origblock);  

(style) please wrap lines at 80 columns.  This patch should be run through checkpatch.pl, which should catch issues like this.
 
> +	if(EXT_LAST_EXTENT(eh) >=  EXT_MAX_EXTENT(eh)){
> +
> +	/* otherwise just scoot all ther other extents down  */
> +	else{
> +		for(i_ex = EXT_LAST_EXTENT(eh); i_ex > ex1; i_ex-- ){
> +			memcpy(i_ex+1, i_ex, sizeof(struct ext4_extent));
> +		}
> +		memcpy(ex1+1, ex2, sizeof(struct ext4_extent));

(style) the whitespace in several other places is also not following the Linux coding style


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