Re: [PATCH 09/10 V2] Use FIEMAP for FIBMAP calls

[Carlos Maiolino <cmaiolino@redhat.com>]

Hi Christoph.

On Mon, Jan 14, 2019 at 05:56:17PM +0100, Christoph Hellwig wrote:
> > +int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > +			    u64 phys, u64 len, u32 flags)
> 
> Any reason this function isn't in inode.c next to the caller and marked
> static?
> 

No reason other than to keep it close to its peer fiemap_fill_user_extent(), I
honestly do prefer to keep both together than in separated files. But, I'm up
to move it to fs/inode.c if required.

> Otherwise looks fine except for the additional sanity checking pointed
> out by Darrick.

Working on that.

> 
> > +	/* only count the extents */
> > +	if (fieinfo->fi_extents_max == 0) {
> > +		fieinfo->fi_extents_mapped++;
> > +		return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> 
> Maybe do a 'goto out' here?
> 
> > +	return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> 
> And reuse this return.   Bonus points for using a good old
> if here:
> 
> 	if (flags & FIEMAP_EXTENT_LAST)
> 		return 1;
> 	return 0;

Ok, will be in the new version, thanks for the review :)

-- 
Carlos