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

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]

On Feb 18, 2019, at 5:03 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> 
> Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls.
> From now on, ->bmap() methods can start to be removed from filesystems
> which already provides ->fiemap().
> 
> This adds a new helper - bmap_fiemap() - which is used to fill in the
> fiemap request, call into the underlying filesystem and check the flags
> set in the extent requested.
> 
> Add a new fiemap fill extent callback to handle the in-kernel only
> fiemap_extent structure used for FIBMAP.
> 
> The new FIEMAP_KERNEL_FIBMAP flag, is used to tell the filesystem
> ->fiemap interface, that the call is coming from ioctl_fibmap. The
> addition of this new flag, requires an update to fiemap_check_flags(),
> so it doesn't treat FIBMAP requests as invalid.
> 
> V3:
> 	- Add FIEMAP_EXTENT_SHARED to the list of invalid extents in
> 	  bmap_fiemap()
> 	- Rename fi_extents_start to fi_cb_data
> 	- Use if conditional instead of ternary operator
> 	- Make fiemap_fill_* callbacks static (which required the
> 	  removal of some macros
> 	- Set FIEMAP_FLAG_SYNC when calling in ->fiemap method from
> 	  fibmap
> 	- Add FIEMAP_KERNEL_FIBMAP flag, to identify the usage of fiemap
> 	  infrastructure for fibmap calls, defined in fs.h so it's not
> 	  exported to userspace.
> 	- Update fiemap_check_flags() to understand FIEMAP_KERNEL_FIBMAP
> 	- Update filesystems supporting both FIBMAP and FIEMAP, which
> 	  need extra checks on FIBMAP calls
> 
> V2:
> 	- Now based on the updated fiemap_extent_info,
> 	- move the fiemap call itself to a new helper
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> I'm flagging this patch as RFC, because here lies the big discussion about how
> to prevent filesystems without FIBMAP support, to keep not supporting it once
> this patchset is integrated.
> 
> Christoph suggested the use of a flag, so, the main difference between this RFC
> V3 and the previous one, is the inclusion of the flag FIEMAP_KERNEL_FIBMAP,
> which let the underlying filesystem know if the call is coming from which ioctl
> the call is coming from (fibmap or fiemap).
> 
> Note that I didn't bother to look too deep if some filesystems like ocfs2, f2fs,
> etc, (filesystems I don't use to work on) does require extra checks than those I
> added to this patch. The goal of this patch is to discuss the generic
> implementation of FIEMAP_KERNEL_FIBMAP flag. If this implementation is ok, I'll
> work on the filesystems specifically.
> 
> Note the bmap call is able to handle errors now, so, invalid fibmap requests
> can now return -EINVAL instead of 0.
> 
> I added modifications to specific filesystems in this patch, otherwise there
> would be a gap between the implementation of the feature (fibmap via fiemap)
> where filesystems without FIBMAP support, would suddenly start to support it
> 
> The remaining of the patches in this patchset are either a copy from the
> previous series (with the respective Reviewed-by tags), and/or a new updated
> version containing the changes suggested previously.
> 
> Comments are much appreciated.
> 
> Cheers

Looks pretty reasonable.  A few minor nits, but nothing serious.

> 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;

> @@ -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.

> @@ -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);

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]