| Message ID | 20181030131823.29040-19-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New ->fiemap infrastructure and ->bmap removal | expand |
On Oct 30, 2018, at 7:18 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote: > > Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls, > from this point, ->bmap() methods can start to be removed. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > This patch can be improved, since fiemap_fill_kernel_extent and > fiemap_fill_usr_extent shares a lot of common code, which still is WIP. > > fs/inode.c | 35 ++++++++++++++++++++++++++++++----- > fs/ioctl.c | 31 +++++++++++++++++++++++++++++++ > include/linux/fs.h | 2 ++ > 3 files changed, 63 insertions(+), 5 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index d09a6f4f0335..389c2165959c 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1591,11 +1591,36 @@ EXPORT_SYMBOL(iput); > */ > int bmap(struct inode *inode, sector_t *block) > { > - if (!inode->i_mapping->a_ops->bmap) > - return -EINVAL; > - > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); > - return 0; > + struct fiemap_ctx f_ctx; > + struct fiemap_extent fextent; > + u64 start = *block << inode->i_blkbits; > + int error = -EINVAL; > + > + if (inode->i_op->fiemap) { > + fextent.fe_logical = 0; > + fextent.fe_physical = 0; > + f_ctx.fc_extents_max = 1; > + f_ctx.fc_extents_mapped = 0; > + f_ctx.fc_data = &fextent; > + f_ctx.fc_start = start; > + f_ctx.fc_len = 1; Should this be "fc_len = 1 << inode->i_blkbits" to map a single block? On the one hand, this might return multiple blocks, but on the other hand FIBMAP shouldn't be allowed if that is the case so this code should detect that situation and consider it an error. Cheers, Andreas > + f_ctx.fc_flags = 0; > + f_ctx.fc_cb = fiemap_fill_kernel_extent; > + > + error = inode->i_op->fiemap(inode, &f_ctx); > + > + if (error) > + goto out; > + > + *block = (fextent.fe_physical + > + (start - fextent.fe_logical)) >> inode->i_blkbits; > + > + } else if (inode->i_mapping->a_ops->bmap) { > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); > + error = 0; > + } > +out: > + return error; > } > EXPORT_SYMBOL(bmap); > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 71d11201a06b..dce710699b82 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -113,6 +113,37 @@ int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > } > > +int fiemap_fill_kernel_extent(struct fiemap_ctx *f_ctx, u64 logical, > + u64 phys, u64 len, u32 flags) > +{ > + struct fiemap_extent *extent = f_ctx->fc_data; > + > + if (f_ctx->fc_extents_max == 0) { > + f_ctx->fc_extents_mapped++; > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > + } > + > + if (f_ctx->fc_extents_mapped >= f_ctx->fc_extents_max) > + return 1; > + > + if (flags & SET_UNKNOWN_FLAGS) > + flags |= FIEMAP_EXTENT_UNKNOWN; > + if (flags & SET_NO_UNMOUNTED_IO_FLAGS) > + flags |= FIEMAP_EXTENT_ENCODED; > + if (flags & SET_NOT_ALIGNED_FLAGS) > + flags |= FIEMAP_EXTENT_NOT_ALIGNED; > + > + extent->fe_logical = logical; > + extent->fe_physical = phys; > + extent->fe_length = len; > + extent->fe_flags = flags; > + > + f_ctx->fc_extents_mapped++; > + > + if (f_ctx->fc_extents_mapped == f_ctx->fc_extents_max) > + return 1; > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > +} > /** > * fiemap_fill_next_extent - Fiemap helper function > * @fieinfo: Fiemap context passed into ->fiemap > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 4c6dee908a38..7f623a434cb0 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1704,6 +1704,8 @@ struct fiemap_ctx { > u64 fc_len; > }; > > +int fiemap_fill_kernel_extent(struct fiemap_ctx *f_ctx, u64 logical, > + u64 phys, u64 len, u32 flags); > int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > u64 phys, u64 len, u32 flags); > int fiemap_check_flags(struct fiemap_ctx *f_ctx, u32 fs_flags); > -- > 2.17.1 > Cheers, Andreas
> > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + f_ctx.fc_extents_max = 1; > > + f_ctx.fc_extents_mapped = 0; > > + f_ctx.fc_data = &fextent; > > + f_ctx.fc_start = start; > > + f_ctx.fc_len = 1; > > Should this be "fc_len = 1 << inode->i_blkbits" to map a single block? D'oh, yeah, I just re-read fiemap implementation, and yeah, len is supposed to be byte-sized, not block sized, so you are right. It doesn't really make much difference here, due the purpose of the fiemap call here, but still it's wrong. I'll update it on a future review. > On the one hand, this might return multiple blocks, but on the other > hand FIBMAP shouldn't be allowed if that is the case so this code should > detect that situation and consider it an error. Well, FIEMAP interface is used internally to get the block mapping requested by the user via FIBMAP, so, yes, FIEMAP interface will return the whole extent, but to the user, the ioctl will return just the mapping of the requested block. Which is basically what we do here: > > + *block = (fextent.fe_physical + > > + (start - fextent.fe_logical)) >> inode->i_blkbits; > > + We only return a single address back, not the whole extent map. Again, thanks for the review, much appreciated
On Tue, Oct 30, 2018 at 02:18:21PM +0100, Carlos Maiolino wrote: > + if (inode->i_op->fiemap) { > + fextent.fe_logical = 0; > + fextent.fe_physical = 0; > + f_ctx.fc_extents_max = 1; > + f_ctx.fc_extents_mapped = 0; > + f_ctx.fc_data = &fextent; > + f_ctx.fc_start = start; > + f_ctx.fc_len = 1; > + f_ctx.fc_flags = 0; > + f_ctx.fc_cb = fiemap_fill_kernel_extent; > + > + error = inode->i_op->fiemap(inode, &f_ctx); > + > + if (error) > + goto out; > + > + *block = (fextent.fe_physical + > + (start - fextent.fe_logical)) >> inode->i_blkbits; > + I think this code needs to be split into a helper. Also we need to check for various "dangerous" flags and fail the call, e.g. FIEMAP_EXTENT_ENCODED, FIEMAP_EXTENT_DELALLOC, FIEMAP_EXTENT_DATA_ENCRYPTED, FIEMAP_EXTENT_DATA_INLINE, FIEMAP_EXTENT_DATA_TAIL, FIEMAP_EXTENT_UNWRITTEN, FIEMAP_EXTENT_SHARED. > + } else if (inode->i_mapping->a_ops->bmap) { > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); Too long line. > + error = 0; > + } > +out: > + return error; No need for a goto label just to return an error. > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; Plain old if statement, please. > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; Same here. But in this case we could actually use a goto to share the code.
On Fri, Nov 16, 2018 at 05:06:43PM +0100, Christoph Hellwig wrote: > On Tue, Oct 30, 2018 at 02:18:21PM +0100, Carlos Maiolino wrote: > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + f_ctx.fc_extents_max = 1; > > + f_ctx.fc_extents_mapped = 0; > > + f_ctx.fc_data = &fextent; > > + f_ctx.fc_start = start; > > + f_ctx.fc_len = 1; > > + f_ctx.fc_flags = 0; > > + f_ctx.fc_cb = fiemap_fill_kernel_extent; > > + > > + error = inode->i_op->fiemap(inode, &f_ctx); > > + > > + if (error) > > + goto out; > > + > > + *block = (fextent.fe_physical + > > + (start - fextent.fe_logical)) >> inode->i_blkbits; > > + > > I think this code needs to be split into a helper. > Yup, my idea is to try to reduce as much as possible the shared code between usr/kernel helpers, I just didn't want to spend more time thinking about it without having a review of the overall design :P > Also we need to check for various "dangerous" flags and fail the > call, e.g. FIEMAP_EXTENT_ENCODED, FIEMAP_EXTENT_DELALLOC, > FIEMAP_EXTENT_DATA_ENCRYPTED, FIEMAP_EXTENT_DATA_INLINE, > FIEMAP_EXTENT_DATA_TAIL, FIEMAP_EXTENT_UNWRITTEN, > FIEMAP_EXTENT_SHARED. > > > + } else if (inode->i_mapping->a_ops->bmap) { > > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); > > Too long line. > > > + error = 0; > > + } > > +out: > > + return error; > > No need for a goto label just to return an error. > > > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > Plain old if statement, please. > > > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > Same here. But in this case we could actually use a goto to share > the code. Ok, fair enough. Thanks a lot for the whole review. Cheers
On Mon, Nov 19, 2018 at 01:50:28PM +0100, Carlos Maiolino wrote: > On Fri, Nov 16, 2018 at 05:06:43PM +0100, Christoph Hellwig wrote: > > On Tue, Oct 30, 2018 at 02:18:21PM +0100, Carlos Maiolino wrote: > > > + if (inode->i_op->fiemap) { > > > + fextent.fe_logical = 0; > > > + fextent.fe_physical = 0; > > > + f_ctx.fc_extents_max = 1; > > > + f_ctx.fc_extents_mapped = 0; > > > + f_ctx.fc_data = &fextent; > > > + f_ctx.fc_start = start; > > > + f_ctx.fc_len = 1; > > > + f_ctx.fc_flags = 0; > > > + f_ctx.fc_cb = fiemap_fill_kernel_extent; > > > + > > > + error = inode->i_op->fiemap(inode, &f_ctx); > > > + > > > + if (error) > > > + goto out; > > > + > > > + *block = (fextent.fe_physical + > > > + (start - fextent.fe_logical)) >> inode->i_blkbits; > > > + > > > > I think this code needs to be split into a helper. > > > > Yup, my idea is to try to reduce as much as possible the shared code between > usr/kernel helpers, I just didn't want to spend more time thinking about it > without having a review of the overall design :P Well, the code in this branch should not change at all by being moved into a helper function..
diff --git a/fs/inode.c b/fs/inode.c index d09a6f4f0335..389c2165959c 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1591,11 +1591,36 @@ EXPORT_SYMBOL(iput); */ int bmap(struct inode *inode, sector_t *block) { - if (!inode->i_mapping->a_ops->bmap) - return -EINVAL; - - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); - return 0; + struct fiemap_ctx f_ctx; + struct fiemap_extent fextent; + u64 start = *block << inode->i_blkbits; + int error = -EINVAL; + + if (inode->i_op->fiemap) { + fextent.fe_logical = 0; + fextent.fe_physical = 0; + f_ctx.fc_extents_max = 1; + f_ctx.fc_extents_mapped = 0; + f_ctx.fc_data = &fextent; + f_ctx.fc_start = start; + f_ctx.fc_len = 1; + f_ctx.fc_flags = 0; + f_ctx.fc_cb = fiemap_fill_kernel_extent; + + error = inode->i_op->fiemap(inode, &f_ctx); + + if (error) + goto out; + + *block = (fextent.fe_physical + + (start - fextent.fe_logical)) >> inode->i_blkbits; + + } else if (inode->i_mapping->a_ops->bmap) { + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); + error = 0; + } +out: + return error; } EXPORT_SYMBOL(bmap); diff --git a/fs/ioctl.c b/fs/ioctl.c index 71d11201a06b..dce710699b82 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -113,6 +113,37 @@ int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; } +int fiemap_fill_kernel_extent(struct fiemap_ctx *f_ctx, u64 logical, + u64 phys, u64 len, u32 flags) +{ + struct fiemap_extent *extent = f_ctx->fc_data; + + if (f_ctx->fc_extents_max == 0) { + f_ctx->fc_extents_mapped++; + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; + } + + if (f_ctx->fc_extents_mapped >= f_ctx->fc_extents_max) + return 1; + + if (flags & SET_UNKNOWN_FLAGS) + flags |= FIEMAP_EXTENT_UNKNOWN; + if (flags & SET_NO_UNMOUNTED_IO_FLAGS) + flags |= FIEMAP_EXTENT_ENCODED; + if (flags & SET_NOT_ALIGNED_FLAGS) + flags |= FIEMAP_EXTENT_NOT_ALIGNED; + + extent->fe_logical = logical; + extent->fe_physical = phys; + extent->fe_length = len; + extent->fe_flags = flags; + + f_ctx->fc_extents_mapped++; + + if (f_ctx->fc_extents_mapped == f_ctx->fc_extents_max) + return 1; + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; +} /** * fiemap_fill_next_extent - Fiemap helper function * @fieinfo: Fiemap context passed into ->fiemap diff --git a/include/linux/fs.h b/include/linux/fs.h index 4c6dee908a38..7f623a434cb0 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1704,6 +1704,8 @@ struct fiemap_ctx { u64 fc_len; }; +int fiemap_fill_kernel_extent(struct fiemap_ctx *f_ctx, u64 logical, + u64 phys, u64 len, u32 flags); int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, u64 phys, u64 len, u32 flags); int fiemap_check_flags(struct fiemap_ctx *f_ctx, u32 fs_flags);
Enables the usage of FIEMAP ioctl infrastructure to handle FIBMAP calls, from this point, ->bmap() methods can start to be removed. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- This patch can be improved, since fiemap_fill_kernel_extent and fiemap_fill_usr_extent shares a lot of common code, which still is WIP. fs/inode.c | 35 ++++++++++++++++++++++++++++++----- fs/ioctl.c | 31 +++++++++++++++++++++++++++++++ include/linux/fs.h | 2 ++ 3 files changed, 63 insertions(+), 5 deletions(-)