| Message ID | 20181030131823.29040-16-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: > > Now that fiemap methods are updated to work based on the new fiemap_ctx > structure, update the code to use the new callback. > The current fiemap_fill_next_extent will be used for user calls of > ->fiemap methods, as its done today, but passed in as a callback to > fiemap_ctx. So, rename it to ifemap_fill_usr_extent(). Minor typo here s/ifemap/fiemap/ Couldn't this series have (mostly?) been achieved by just adding the callback to fiemap_extent_info and using that everywhere? Cheers, Andreas > The 'new' fiemap_fill_next_extent() will now be just a helper to call > fiemap_ctx callback. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/ioctl.c | 42 +++++++++++++++++++++++++----------------- > include/linux/fs.h | 2 +- > 2 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 27f79b29cb07..85a7aec40e3d 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -68,25 +68,10 @@ static int ioctl_fibmap(struct file *filp, int __user *p) > return put_user(res, p); > } > > -/** > - * fiemap_fill_next_extent - Fiemap helper function > - * @fieinfo: Fiemap context passed into ->fiemap > - * @logical: Extent logical start offset, in bytes > - * @phys: Extent physical start offset, in bytes > - * @len: Extent length, in bytes > - * @flags: FIEMAP_EXTENT flags that describe this extent > - * > - * Called from file system ->fiemap callback. Will populate extent > - * info as passed in via arguments and copy to user memory. On > - * success, extent count on fieinfo is incremented. > - * > - * Returns 0 on success, -errno on error, 1 if this was the last > - * extent that will fit in user array. > - */ > #define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC) > #define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED) > #define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE) > -int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > +int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, > u64 phys, u64 len, u32 flags) > { > struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > @@ -124,8 +109,29 @@ int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > return 1; > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > } > -EXPORT_SYMBOL(fiemap_fill_next_extent); > > +/** > + * fiemap_fill_next_extent - Fiemap helper function > + * @fieinfo: Fiemap context passed into ->fiemap > + * @logical: Extent logical start offset, in bytes > + * @phys: Extent physical start offset, in bytes > + * @len: Extent length, in bytes > + * @flags: FIEMAP_EXTENT flags that describe this extent > + * > + * Called from file system ->fiemap callback. Will populate extent > + * info as passed in via arguments and copy to user memory. On > + * success, extent count on fieinfo is incremented. > + * > + * Returns 0 on success, -errno on error, 1 if this was the last > + * extent that will fit in user array. > + */ > + > +int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > + u64 phys, u64 len, u32 flags) > +{ > + return f_ctx->fc_cb(f_ctx, logical, phys, len, flags); > +} > +EXPORT_SYMBOL(fiemap_fill_next_extent); > /** > * fiemap_check_flags - check validity of requested flags for fiemap > * @fieinfo: Fiemap context passed into ->fiemap > @@ -208,6 +214,8 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg) > f_ctx.fc_start = fiemap.fm_start; > f_ctx.fc_len = len; > > + f_ctx.fc_cb = fiemap_fill_usr_extent; > + > if (fiemap.fm_extent_count != 0 && > !access_ok(VERIFY_WRITE, fieinfo.fi_extents_start, > fieinfo.fi_extents_max * sizeof(struct fiemap_extent))) > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 945cfb3e06e4..9a538bc0dac2 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1704,7 +1704,7 @@ typedef int (*fiemap_fill_cb)(struct fiemap_ctx *f_ctx, u64 logical, > struct fiemap_ctx { > unsigned int fc_flags; /* Flags as passed from user */ > void *fc_data; > - fiemap_fill_cb fc_cb; /* Unused by now */ > + fiemap_fill_cb fc_cb; > u64 fc_start; > u64 fc_len; > }; > -- > 2.17.1 > Cheers, Andreas
On Mon, Nov 05, 2018 at 03:14:02PM -0700, Andreas Dilger wrote: > On Oct 30, 2018, at 7:18 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote: > > > > Now that fiemap methods are updated to work based on the new fiemap_ctx > > structure, update the code to use the new callback. > > The current fiemap_fill_next_extent will be used for user calls of > > ->fiemap methods, as its done today, but passed in as a callback to > > fiemap_ctx. So, rename it to ifemap_fill_usr_extent(). > > Minor typo here s/ifemap/fiemap/ > > Couldn't this series have (mostly?) been achieved by just adding the > callback to fiemap_extent_info and using that everywhere? Not really, because fiemap_extent_info has (had now) a userspace pointer, which isn't used in FIBMAP calls in later patches, so the idea of using callbacks started as a need to use different code workflow, depending on the user of (now deprecated) fiemap_extent_info > > Cheers, Andreas > > > The 'new' fiemap_fill_next_extent() will now be just a helper to call > > fiemap_ctx callback. > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > fs/ioctl.c | 42 +++++++++++++++++++++++++----------------- > > include/linux/fs.h | 2 +- > > 2 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/fs/ioctl.c b/fs/ioctl.c > > index 27f79b29cb07..85a7aec40e3d 100644 > > --- a/fs/ioctl.c > > +++ b/fs/ioctl.c > > @@ -68,25 +68,10 @@ static int ioctl_fibmap(struct file *filp, int __user *p) > > return put_user(res, p); > > } > > > > -/** > > - * fiemap_fill_next_extent - Fiemap helper function > > - * @fieinfo: Fiemap context passed into ->fiemap > > - * @logical: Extent logical start offset, in bytes > > - * @phys: Extent physical start offset, in bytes > > - * @len: Extent length, in bytes > > - * @flags: FIEMAP_EXTENT flags that describe this extent > > - * > > - * Called from file system ->fiemap callback. Will populate extent > > - * info as passed in via arguments and copy to user memory. On > > - * success, extent count on fieinfo is incremented. > > - * > > - * Returns 0 on success, -errno on error, 1 if this was the last > > - * extent that will fit in user array. > > - */ > > #define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC) > > #define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED) > > #define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE) > > -int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > > +int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, > > u64 phys, u64 len, u32 flags) > > { > > struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > > @@ -124,8 +109,29 @@ int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > > return 1; > > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > } > > -EXPORT_SYMBOL(fiemap_fill_next_extent); > > > > +/** > > + * fiemap_fill_next_extent - Fiemap helper function > > + * @fieinfo: Fiemap context passed into ->fiemap > > + * @logical: Extent logical start offset, in bytes > > + * @phys: Extent physical start offset, in bytes > > + * @len: Extent length, in bytes > > + * @flags: FIEMAP_EXTENT flags that describe this extent > > + * > > + * Called from file system ->fiemap callback. Will populate extent > > + * info as passed in via arguments and copy to user memory. On > > + * success, extent count on fieinfo is incremented. > > + * > > + * Returns 0 on success, -errno on error, 1 if this was the last > > + * extent that will fit in user array. > > + */ > > + > > +int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > > + u64 phys, u64 len, u32 flags) > > +{ > > + return f_ctx->fc_cb(f_ctx, logical, phys, len, flags); > > +} > > +EXPORT_SYMBOL(fiemap_fill_next_extent); > > /** > > * fiemap_check_flags - check validity of requested flags for fiemap > > * @fieinfo: Fiemap context passed into ->fiemap > > @@ -208,6 +214,8 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg) > > f_ctx.fc_start = fiemap.fm_start; > > f_ctx.fc_len = len; > > > > + f_ctx.fc_cb = fiemap_fill_usr_extent; > > + > > if (fiemap.fm_extent_count != 0 && > > !access_ok(VERIFY_WRITE, fieinfo.fi_extents_start, > > fieinfo.fi_extents_max * sizeof(struct fiemap_extent))) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 945cfb3e06e4..9a538bc0dac2 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1704,7 +1704,7 @@ typedef int (*fiemap_fill_cb)(struct fiemap_ctx *f_ctx, u64 logical, > > struct fiemap_ctx { > > unsigned int fc_flags; /* Flags as passed from user */ > > void *fc_data; > > - fiemap_fill_cb fc_cb; /* Unused by now */ > > + fiemap_fill_cb fc_cb; > > u64 fc_start; > > u64 fc_len; > > }; > > -- > > 2.17.1 > > > > > Cheers, Andreas > > > > >
On Tue, Nov 06, 2018 at 09:52:03AM +0100, Carlos Maiolino wrote: > Not really, because fiemap_extent_info has (had now) a userspace pointer, which > isn't used in FIBMAP calls in later patches, so the idea of using callbacks > started as a need to use different code workflow, depending on the user of (now > deprecated) fiemap_extent_info Well, we could also change that member of fiemap_extent_info if we really wanted to. Not sure that would be the better route, though.
> +int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, Please spell out user. > +int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > + u64 phys, u64 len, u32 flags) > +{ > + return f_ctx->fc_cb(f_ctx, logical, phys, len, flags); > +} > +EXPORT_SYMBOL(fiemap_fill_next_extent); Do we really need this wrapper? Which reminds me that we usually pass the callbacks as direct function arguments. Any good reason it is in the context here?
On Fri, Nov 16, 2018 at 04:55:42PM +0100, Christoph Hellwig wrote: > On Tue, Nov 06, 2018 at 09:52:03AM +0100, Carlos Maiolino wrote: > > Not really, because fiemap_extent_info has (had now) a userspace pointer, which > > isn't used in FIBMAP calls in later patches, so the idea of using callbacks > > started as a need to use different code workflow, depending on the user of (now > > deprecated) fiemap_extent_info > > Well, we could also change that member of fiemap_extent_info if we > really wanted to. Not sure that would be the better route, though. I think there are some downsides in keep using fiemap_extent_info. I'd rather prefer use this new fiemap_ctx, which we can use to embed most of the arguments needed for fiemap, like the flags which were passed separated. At all, most of the fields would fit either in fiemap_extent_info, or the new fiemap_ctx. I'd prefer the latter, due simplicity. We could probably simplify it using the void pointer directly into fiemap_extent_info and using flags to discern between user/kernel space pointer. But that would drop one of the goals of this patchset, which is adding a new and better interface for FIEMAP. Preferentially, I'd stick with fiemap_ctx. Cheers and thanks for the review
On Fri, Nov 16, 2018 at 04:57:21PM +0100, Christoph Hellwig wrote: > > +int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, > > Please spell out user. Sure, will do. > > > +int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, > > + u64 phys, u64 len, u32 flags) > > +{ > > + return f_ctx->fc_cb(f_ctx, logical, phys, len, flags); > > +} > > +EXPORT_SYMBOL(fiemap_fill_next_extent); > > Do we really need this wrapper? Which reminds me that we usually > pass the callbacks as direct function arguments. Any good reason it > is in the context here? The reason I left the wrapper is to avoid replacing all fiemap_fill_next_extent() calls by the callback, may not be the best idea though. About using the callback in the context, I thought it would be the easiest way to pass the callbacks around, without adding a function pointer to many functions. The callback is set in ioctl_fiemap() (for user context) or bmap(), if we pass the callback via function argument, many filesystems would need to juggle around with the function pointer, once, some of them does not call the fiemap_fill_next_extent directly from the fiemap method, but into other sub-functions, for example, in ext4, we would need the function pointer in ext4_fiemap, and other functions like ext4_fill_fiemap_extents() and ext4_xattr_fiemap(), etc. We could even call fiemap_fill_{usr,kernel}_extent() directly from fiemap_fill_next_extent, and use a flag to differentiate both use cases, but this will kill the transparent implementation IMHO.
diff --git a/fs/ioctl.c b/fs/ioctl.c index 27f79b29cb07..85a7aec40e3d 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -68,25 +68,10 @@ static int ioctl_fibmap(struct file *filp, int __user *p) return put_user(res, p); } -/** - * fiemap_fill_next_extent - Fiemap helper function - * @fieinfo: Fiemap context passed into ->fiemap - * @logical: Extent logical start offset, in bytes - * @phys: Extent physical start offset, in bytes - * @len: Extent length, in bytes - * @flags: FIEMAP_EXTENT flags that describe this extent - * - * Called from file system ->fiemap callback. Will populate extent - * info as passed in via arguments and copy to user memory. On - * success, extent count on fieinfo is incremented. - * - * Returns 0 on success, -errno on error, 1 if this was the last - * extent that will fit in user array. - */ #define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC) #define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED) #define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE) -int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, +int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical, u64 phys, u64 len, u32 flags) { struct fiemap_extent_info *fieinfo = f_ctx->fc_data; @@ -124,8 +109,29 @@ int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, return 1; return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; } -EXPORT_SYMBOL(fiemap_fill_next_extent); +/** + * fiemap_fill_next_extent - Fiemap helper function + * @fieinfo: Fiemap context passed into ->fiemap + * @logical: Extent logical start offset, in bytes + * @phys: Extent physical start offset, in bytes + * @len: Extent length, in bytes + * @flags: FIEMAP_EXTENT flags that describe this extent + * + * Called from file system ->fiemap callback. Will populate extent + * info as passed in via arguments and copy to user memory. On + * success, extent count on fieinfo is incremented. + * + * Returns 0 on success, -errno on error, 1 if this was the last + * extent that will fit in user array. + */ + +int fiemap_fill_next_extent(struct fiemap_ctx *f_ctx, u64 logical, + u64 phys, u64 len, u32 flags) +{ + return f_ctx->fc_cb(f_ctx, logical, phys, len, flags); +} +EXPORT_SYMBOL(fiemap_fill_next_extent); /** * fiemap_check_flags - check validity of requested flags for fiemap * @fieinfo: Fiemap context passed into ->fiemap @@ -208,6 +214,8 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg) f_ctx.fc_start = fiemap.fm_start; f_ctx.fc_len = len; + f_ctx.fc_cb = fiemap_fill_usr_extent; + if (fiemap.fm_extent_count != 0 && !access_ok(VERIFY_WRITE, fieinfo.fi_extents_start, fieinfo.fi_extents_max * sizeof(struct fiemap_extent))) diff --git a/include/linux/fs.h b/include/linux/fs.h index 945cfb3e06e4..9a538bc0dac2 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1704,7 +1704,7 @@ typedef int (*fiemap_fill_cb)(struct fiemap_ctx *f_ctx, u64 logical, struct fiemap_ctx { unsigned int fc_flags; /* Flags as passed from user */ void *fc_data; - fiemap_fill_cb fc_cb; /* Unused by now */ + fiemap_fill_cb fc_cb; u64 fc_start; u64 fc_len; };
Now that fiemap methods are updated to work based on the new fiemap_ctx structure, update the code to use the new callback. The current fiemap_fill_next_extent will be used for user calls of ->fiemap methods, as its done today, but passed in as a callback to fiemap_ctx. So, rename it to ifemap_fill_usr_extent(). The 'new' fiemap_fill_next_extent() will now be just a helper to call fiemap_ctx callback. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/ioctl.c | 42 +++++++++++++++++++++++++----------------- include/linux/fs.h | 2 +- 2 files changed, 26 insertions(+), 18 deletions(-)