Re: [PATCH] fiemap support for ext3

[Andreas Dilger <adilger@sun.com>]

On Apr 23, 2008  15:39 -0400, Josef Bacik wrote:
> Here is my reworked fiemap patch for ext3.  The generic fiemap handler takes the
> get_block_t from the filesystem and does the fiemap that way, so then adding
> ext2 support should be a snap, as well as any other fs that wants to use this.

The generic mechanism is really good, some minor issues still exist.

> +int generic_block_fiemap(struct inode *inode, unsigned long arg,
> +			 get_block_t *get_block)
> +{
> +	/*
> +	 * if fm_start is in the middle of the current block, get the next
> +	 * block so we don't end up returning a start thats before the given
> +	 * fm_start
> +	 */
> +	start_blk = (fiemap_s->fm_start + (1 << inode->i_blkbits) - 1) >>
> +		inode->i_blkbits;

This should return the start of the current block (so the requested fm_start
is included in the returned mapping), instead of the start of the next block.

> +	/*
> +	 * we want the comparisons to be unsigned, in case somebody passes -1,
> +	 * meaning they want they want the entire file, but the result has to be
> +	 * signed so we can handle the case where we get more blocks than the
> +	 * size of the file
> +	 */
> +	length = (long)min((unsigned long)fiemap_s->fm_length,
> +			   (unsigned long)i_size_read(inode));

This might be written as:

	length = (long)min_t(unsigned long,fiemap_s->fm_len,i_size_read(inode));

Also, what about files that have blocks mapped after i_size?

> +		if (!tmp.b_blocknr) {
> +
> +			if (!hole) {

No empty line above "if (!hole)".

> +			fiemap_e.fe_offset = tmp.b_blocknr <<
> +				inode->i_blkbits;

This doesn't need to wrap.

> +			fiemap_e.fe_length = tmp.b_size;
> +
> +			if (length <= 0)
> +				fiemap_e.fe_flags |= FIEMAP_EXTENT_LAST;

Should the exit condition should be the first hole (block == 0) after i_size?

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