Re: [vfs PATCH v3 2/4] VFS: Add new __generic_iomap_fiemap interface

[Jan Kara <jack@suse.cz>]

On Fri 04-03-16 15:11:38, Bob Peterson wrote:
> @@ -251,6 +253,100 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
>  }
>  
>  /**
> + * __generic_iomap_fiemap - FIEMAP for iomap based inodes (no locking)
> + * @inode: the inode to map
> + * @fieinfo: the fiemap info struct that will be passed back to userspace
> + * @start: where to start mapping in the inode
> + * @len: how much space to map

You are missing @iomap description here.

> + *
> + * This does FIEMAP for iomap based inodes.  Basically it will just loop
> + * through iomap 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 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.

Line over 80 chars here.

> + *
> + * If you use this function directly, you need to do your own locking. Use
> + * generic_iomap_fiemap if you want the locking done for you.
> + */
> +
> +int __generic_iomap_fiemap(struct inode *inode,
> +			   struct fiemap_extent_info *fieinfo, loff_t start,
> +			   loff_t len, iomap_get_t iomap)
> +{
> +	struct iomap iom, prev_iom;
> +	loff_t isize = i_size_read(inode);
> +	int ret = 0;
> +
> +	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> +	memset(&prev_iom, 0, sizeof(prev_iom));
> +	if (len >= isize)
> +		len = isize;
> +
> +	while ((ret == 0) && (start < isize) && len) {
> +		memset(&iom, 0, sizeof(iom));
> +		ret = iomap(inode->i_mapping, start, len, &iom);
> +		if (ret)
> +			break;
> +
> +		if (!iomap_needs_allocation(&iom)) {

Why not directly check for hole here? The name iomap_needs_allocation()
only obscures what's going on here. At least to me... ;)

> +			if (prev_iom.blkno)
> +				ret = fiemap_fill_next_extent(fieinfo,
> +							      prev_iom.offset,
> +							      blk_to_logical(inode,
> +							      prev_iom.blkno),
> +							      prev_iom.length,
> +							      FIEMAP_EXTENT_MERGED);

I'd just format this as 
				ret = fiemap_fill_next_extent(fieinfo,
					prev_iom.offset,
					blk_to_logical(inode, prev_iom.blkno),
					prev_iom.length,
					FIEMAP_EXTENT_MERGED);

IMHO it is more readable and doesn't overflow 80 chars.

> +			prev_iom = iom;
> +		}
> +		start += iom.length;
> +		if (len < iom.length)
> +			break;
> +		len -= iom.length;
> +		cond_resched();

Add the fatal_signal_pending() check we have in __generic_block_fiemap()?

> +	}
> +
> +	if (prev_iom.blkno)
> +		ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset,
> +					      blk_to_logical(inode,
> +							     prev_iom.blkno),
> +					      prev_iom.length,
> +					      FIEMAP_EXTENT_MERGED |
> +					      FIEMAP_EXTENT_LAST);

FIEMAP_EXTENT_LAST should only be set if this is the last extent in the
file. Here you set it in other cases as well.

Also in some cases - e.g. when fiemap_fill_next_extent() in the loop above
runs out of space in fieinfo, or in case of error, you shouldn't try to
fill next extent, should you?

>  typedef int (get_block_t)(struct inode *inode, sector_t iblock,
>  			struct buffer_head *bh_result, int create);
> +typedef int (iomap_get_t)(struct address_space *mapping, loff_t pos,
> +			  ssize_t length, struct iomap *iomap);

Why do you pass 'struct address_space' and not 'struct inode'? That would
look more natural to me...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR