Re: [PATCH 09/15] fs: iomap based fiemap implementation

[Bob Peterson <rpeterso@redhat.com>]

----- Original Message -----
| Add a simple fiemap implementation based on iomap_ops, partially based
| on a previous implementation from Bob Peterson <rpeterso@redhat.com>.
| 
| Signed-off-by: Christoph Hellwig <hch@lst.de>
| ---
|  fs/iomap.c            | 90
|  +++++++++++++++++++++++++++++++++++++++++++++++++++
|  include/linux/iomap.h |  3 ++
|  2 files changed, 93 insertions(+)
| 
| diff --git a/fs/iomap.c b/fs/iomap.c
| index f84c6eb..7e639bf 100644
| --- a/fs/iomap.c
| +++ b/fs/iomap.c
| @@ -405,3 +405,93 @@ out_unlock:
|  	return ret;
|  }
|  EXPORT_SYMBOL_GPL(iomap_page_mkwrite);
| +
| +struct fiemap_ctx {
| +	struct fiemap_extent_info *fi;
| +	struct iomap prev;
| +};
| +
| +static int iomap_to_fiemap(struct fiemap_extent_info *fi,
| +		struct iomap *iomap, u32 flags)
| +{
| +	switch (iomap->type) {
| +	case IOMAP_HOLE:
| +		/* skip holes */
| +		return 0;
| +	case IOMAP_DELALLOC:
| +		flags |= FIEMAP_EXTENT_DELALLOC | FIEMAP_EXTENT_UNKNOWN;
| +		break;
| +	case IOMAP_UNWRITTEN:
| +		flags |= FIEMAP_EXTENT_UNWRITTEN;
| +		break;
| +	case IOMAP_MAPPED:
| +		break;
| +	}
| +
| +	return fiemap_fill_next_extent(fi, iomap->offset,
| +			iomap->blkno != IOMAP_NULL_BLOCK ? iomap->blkno << 9: 0,
| +			iomap->length, flags | FIEMAP_EXTENT_MERGED);
| +
| +}
| +
| +static loff_t
| +iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void
| *data,
| +		struct iomap *iomap)
| +{
| +	struct fiemap_ctx *ctx = data;
| +	loff_t ret = length;
| +
| +	if (iomap->type == IOMAP_HOLE)
| +		return length;
| +
| +	ret = iomap_to_fiemap(ctx->fi, &ctx->prev, 0);
| +	ctx->prev = *iomap;
| +	switch (ret) {
| +	case 0:		/* success */
| +		return length;
| +	case 1:		/* extent array full */
| +		return 0;
| +	default:
| +		return ret;
| +	}
| +}
| +
| +int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
| +		loff_t start, loff_t len, struct iomap_ops *ops)
| +{
| +	struct fiemap_ctx ctx;
| +	loff_t ret;
| +
| +	memset(&ctx, 0, sizeof(ctx));
| +	ctx.fi = fi;
| +	ctx.prev.type = IOMAP_HOLE;
| +
| +	ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
| +	if (ret)
| +		return ret;
| +
| +	ret = filemap_write_and_wait(inode->i_mapping);
| +	if (ret)
| +		return ret;
| +
| +	while (len > 0) {
| +		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
| +				iomap_fiemap_actor);
| +		if (ret < 0)
| +			return ret;
| +		if (ret == 0)
| +			break;
| +
| +		start += ret;
| +		len -= ret;
| +	}
| +
| +	if (ctx.prev.type != IOMAP_HOLE) {
| +		ret = iomap_to_fiemap(fi, &ctx.prev, FIEMAP_EXTENT_LAST);
| +		if (ret < 0)
| +			return ret;
| +	}
| +
| +	return 0;
| +}
| +EXPORT_SYMBOL_GPL(iomap_fiemap);
| diff --git a/include/linux/iomap.h b/include/linux/iomap.h
| index 854766f..b3deee1 100644
| --- a/include/linux/iomap.h
| +++ b/include/linux/iomap.h
| @@ -3,6 +3,7 @@
|  
|  #include <linux/types.h>
|  
| +struct fiemap_extent_info;
|  struct inode;
|  struct iov_iter;
|  struct kiocb;
| @@ -63,5 +64,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos,
| bool *did_zero,
|  		struct iomap_ops *ops);
|  int iomap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
|  		struct iomap_ops *ops);
| +int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
| +		loff_t start, loff_t len, struct iomap_ops *ops);
|  
|  #endif /* LINUX_IOMAP_H */
| --
| 2.1.4
| 
| 

Hi Christoph,

I've been looking at this again. Where are the calls to the fs-specific bits
for fiemap? It looks like iomap_fiemap calls iomap_apply, which calls
iomap_fiemap_actor, but that doesn't call any ops->iomap_get_iomap or similar.
It calls the iomap_begin (which BTW has a comment that says "Execute a iomap
write" which is probably wrong and should be more generic, as for cases like
fiemap) and it calls iomap_end. But it never calls an fs-specific actor
anywhere. Am I missing something? My earlier version passed in the actor
function, as per Dave Chinner's request, but yours doesn't.

Regards,

Bob Peterson
Red Hat File Systems