Re: [PATCH RFC 8/9] Use FIEMAP for FIBMAP calls

[Carlos Maiolino <cmaiolino@redhat.com>]

> > Comments are much appreciated.
> > 
> > Cheers
> 
> Looks pretty reasonable.  A few minor nits, but nothing serious.

Thanks for the review...

> 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index db681d310465..323dfe3d26fd 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > 
> > +static int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo,
> > +			u64 logical, u64 phys, u64 len, u32 flags)
> > +{
> > +
> > +out:
> > +	if (flags & FIEMAP_EXTENT_LAST)
> > +		return 1;
> > +	return 0;
> 
> Why not just the straight return:
> 
>         return !!(flags & FIEMAP_EXTENT_LAST);
> 
> or if this function is only returning 0 or 1 then it could return bool and be:
> 
>         return flags & FIEMAP_EXTENT_LAST;

Christoph asked to leave it on the good if () style :P

> 
> > @@ -1594,10 +1666,14 @@ EXPORT_SYMBOL(iput);
> >  */
> > int bmap(struct inode *inode, sector_t *block)
> > {
> > -	if (!inode->i_mapping->a_ops->bmap)
> > +	if (inode->i_op->fiemap)
> > +		return bmap_fiemap(inode, block);
> > +	else if (inode->i_mapping->a_ops->bmap)
> > +		*block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> > +						       *block);
> 
> Typically there is no "else" after return.

Makes sense.

> 
> > @@ -113,7 +110,11 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> > 	fieinfo->fi_extents_mapped++;
> > 	if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> > 		return 1;
> > -	return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > +
> > +out:
> > +	if (flags & FIEMAP_EXTENT_LAST)
> > +		return 1;
> > +	return 0;
> > }
> 
> As above, the simpler code would be:
> 
>         return !!(flags & FIEMAP_EXTENT_LAST);

Same here, discussed on the last thread (I guess)

> 
> Cheers, Andreas

Thanks again for the review



-- 
Carlos