On Feb 18, 2019, at 5:03 AM, Carlos Maiolino <cmaiolino@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>
> 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:
signature.asc
Description: Message signed with OpenPGP
