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@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
