Re: [PATCH 17/20] fiemap: Get rid of fiemap_extent_info

[Christoph Hellwig <hch@lst.de>]

On Tue, Oct 30, 2018 at 02:18:20PM +0100, Carlos Maiolino wrote:
> With the goal of using FIEMAP infra-structure for FIBMAP ioctl, and with
> the updates being done previously into the FIEMAP interface, the
> fiemap_extent_info structure is no longer necessary, so, move
> fi_extents_mapped and fi_extents_max up to fiemap_ctx. and use
> fiemap_ctx.fc_data to hold a pointer to struct fiemap_extent.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/btrfs/extent_io.c  |  3 +--
>  fs/ioctl.c            | 29 +++++++++++++----------------
>  include/linux/fs.h    |  9 ++-------
>  include/linux/iomap.h |  1 -
>  4 files changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 6a8bc502bc04..88b8a4416dfc 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4436,7 +4436,6 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx)
>  	struct btrfs_path *path;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	struct fiemap_cache cache = { 0 };
> -	struct fiemap_extent_info *fieinfo = f_ctx->fc_data;
>  	int end = 0;
>  	u64 em_start = 0;
>  	u64 em_len = 0;
> @@ -4561,7 +4560,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx)
>  		} else if (em->block_start == EXTENT_MAP_DELALLOC) {
>  			flags |= (FIEMAP_EXTENT_DELALLOC |
>  				  FIEMAP_EXTENT_UNKNOWN);
> -		} else if (fieinfo->fi_extents_max) {
> +		} else if (f_ctx->fc_extents_max) {
>  			u64 bytenr = em->block_start -
>  				(em->start - em->orig_start);

Shouldn't this be earlier, when or just after when we introduce 
f_ctx->fc_extents_max?

>  		return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;

Can you use a plain old if statment here to make the code a little
more readable?

		if (flags & FIEMAP_EXTENT_LAST)
			return 1;
		return 0;

>  struct fiemap_ctx {
>  	unsigned int fc_flags;	/* Flags as passed from user */
> +	unsigned int fc_extents_mapped;	/* Number of mapped extents */
> +	unsigned int fc_extents_max;	/* Size of fiemap_extent array */
>  	void *fc_data;
>  	fiemap_fill_cb fc_cb;
>  	u64 fc_start;

This makes me wonder if we shouldn't just have kept the existing
structure and massaged it into the new form.  The two just look
very, very similar..