Re: [PATCH 5/5] generic block based fiemap implementation

[Andreas Dilger <adilger@sun.com>]

On May 28, 2008  11:31 -0400, Josef Bacik wrote:
> Signed-off-by: Josef Bacik <jbacik@redhat.com>
> +/*
> + * @inode - the inode to map
> + * @arg - the pointer to userspace where we copy everything to
> + * @get_block - the fs's get_block function
> + *
> + * This does FIEMAP for block based inodes.  Basically it will just loop
> + * through get_block until we hit the number of extents we want to map, or we
> + * go past the end of the file and hit a hole.
> + *
> + * If it is possible to have holes and data past EOF then please don't use
> + * this function, it will do wrong things.
> + */

I think it would be clearer to write at the end "If it is possible to have
data blocks beyond a hole past @inode->i_size, then please do not use this
function, it will stop at the first unmapped block beyond i_size."

> +int generic_block_fiemap(struct inode *inode,
> +			 struct fiemap_extent_info *fieinfo, u64 start,
> +			 u64 len, get_block_t *get_block)
> +{
> +	struct buffer_head tmp;
> +	unsigned int start_blk;
> +	long long length = 0, map_len = 0;
> +	u64 logical = 0, phys = 0, size = 0;
> +	u32 flags = 0;
> +	int ret = 0;
> +
> +	start_blk = logical_to_blk(inode, start);
> +
> +	/* guard against change */
> +	mutex_lock(&inode->i_mutex);
> +
> +	length = (long long)min_t(u64, len, i_size_read(inode));
> +	map_len = length;
> +
> +	do {
> +		/*
> +		 * we set b_size to the total size we want so it will map as
> +		 * many contiguous blocks as possible at once
> +		 */
> +		memset(&tmp, 0, sizeof(struct buffer_head));
> +		tmp.b_size = map_len;
> +
> +		ret = get_block(inode, start_blk, &tmp, 0);
> +		if (ret)
> +			break;

Shouldn't map_len be reduced as the file is traversed?  Otherwise the
filesystem may be doing a lot more work than it needs to for block mapping
if the requested length is not the whole file.  If, for example, the first
1GB of the file is requested (map_len = 2 << 30) and 1022MB are mapped, the
last 1MB get_block() request will still request 1GB mapping.

It isn't a major problem (only a corner case really), and the map_len
can't be exactly the same as "length", because it still needs to be
positive after EOF so that blocks will continue to be mapped.

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