On Mon, Nov 05, 2018 at 03:14:38PM -0700, Andreas Dilger wrote:
> On Oct 30, 2018, at 7:18 AM, Carlos Maiolino <cmaiolino@xxxxxxxxxx> wrote:
> >
> > With the goal of using FIEMAP infra-structure for FIBMAP ioctl, and with
> > the updates being done previously into the FIEMAP interface, the
> > fiemap_extent_info structure is no longer necessary, so, move
> > fi_extents_mapped and fi_extents_max up to fiemap_ctx. and use
> > fiemap_ctx.fc_data to hold a pointer to struct fiemap_extent.
>
> Having the "void *fc_data" pointer point to fiemap_extent is not
> necessarily intuitive. Why not just make it directly point to it:
>
> struct fiemap_extent *fc_extent;
Both data structures pointed by *fc_data contains a fiemap_extent struct, but
one is a __user annotated address while other is a kernel space address, I don't
know if I can use a non __user pointer as you suggested to store the address of
a __user pointer, I actually don't think I can without getting warnings, but I
honestly don't remember.
>
> Cheers, Andreas
>
> > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > ---
> > fs/btrfs/extent_io.c | 3 +--
> > fs/ioctl.c | 29 +++++++++++++----------------
> > include/linux/fs.h | 9 ++-------
> > include/linux/iomap.h | 1 -
> > 4 files changed, 16 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 6a8bc502bc04..88b8a4416dfc 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -4436,7 +4436,6 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx)
> > struct btrfs_path *path;
> > struct btrfs_root *root = BTRFS_I(inode)->root;
> > struct fiemap_cache cache = { 0 };
> > - struct fiemap_extent_info *fieinfo = f_ctx->fc_data;
> > int end = 0;
> > u64 em_start = 0;
> > u64 em_len = 0;
> > @@ -4561,7 +4560,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx)
> > } else if (em->block_start == EXTENT_MAP_DELALLOC) {
> > flags |= (FIEMAP_EXTENT_DELALLOC |
> > FIEMAP_EXTENT_UNKNOWN);
> > - } else if (fieinfo->fi_extents_max) {
> > + } else if (f_ctx->fc_extents_max) {
> > u64 bytenr = em->block_start -
> > (em->start - em->orig_start);
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index cbc1b21c4f7c..71d11201a06b 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -78,17 +78,16 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > 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;
> > struct fiemap_extent extent;
> > - struct fiemap_extent __user *dest = fieinfo->fi_extents_start;
> > + struct fiemap_extent __user *dest = f_ctx->fc_data;
> >
> > /* only count the extents */
> > - if (fieinfo->fi_extents_max == 0) {
> > - fieinfo->fi_extents_mapped++;
> > + if (f_ctx->fc_extents_max == 0) {
> > + f_ctx->fc_extents_mapped++;
> > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
> > }
> >
> > - if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max)
> > + if (f_ctx->fc_extents_mapped >= f_ctx->fc_extents_max)
> > return 1;
> >
> > if (flags & SET_UNKNOWN_FLAGS)
> > @@ -104,12 +103,12 @@ int fiemap_fill_usr_extent(struct fiemap_ctx *f_ctx, u64 logical,
> > extent.fe_length = len;
> > extent.fe_flags = flags;
> >
> > - dest += fieinfo->fi_extents_mapped;
> > + dest += f_ctx->fc_extents_mapped;
> > if (copy_to_user(dest, &extent, sizeof(extent)))
> > return -EFAULT;
> >
> > - fieinfo->fi_extents_mapped++;
> > - if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> > + 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;
> > }
> > @@ -189,7 +188,6 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> > {
> > struct fiemap fiemap;
> > struct fiemap __user *ufiemap = (struct fiemap __user *) arg;
> > - struct fiemap_extent_info fieinfo = { 0, };
> > struct inode *inode = file_inode(filp);
> > struct super_block *sb = inode->i_sb;
> > struct fiemap_ctx f_ctx;
> > @@ -210,19 +208,18 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> > if (error)
> > return error;
> >
> > - fieinfo.fi_extents_max = fiemap.fm_extent_count;
> > - fieinfo.fi_extents_start = ufiemap->fm_extents;
> > -
> > + f_ctx.fc_extents_max = fiemap.fm_extent_count;
> > + f_ctx.fc_data = ufiemap->fm_extents;
> > + f_ctx.fc_extents_mapped = 0;
> > f_ctx.fc_flags = fiemap.fm_flags;
> > - f_ctx.fc_data = &fieinfo;
> > 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)))
> > + !access_ok(VERIFY_WRITE, f_ctx.fc_data,
> > + f_ctx.fc_extents_max * sizeof(struct fiemap_extent)))
> > return -EFAULT;
> >
> > if (f_ctx.fc_flags & FIEMAP_FLAG_SYNC)
> > @@ -230,7 +227,7 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> >
> > error = inode->i_op->fiemap(inode, &f_ctx);
> > fiemap.fm_flags = f_ctx.fc_flags;
> > - fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> > + fiemap.fm_mapped_extents = f_ctx.fc_extents_mapped;
> > if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> > error = -EFAULT;
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9a538bc0dac2..4c6dee908a38 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1691,18 +1691,13 @@ extern bool may_open_dev(const struct path *path);
> >
> > struct fiemap_ctx;
> >
> > -struct fiemap_extent_info {
> > - unsigned int fi_extents_mapped; /* Number of mapped extents */
> > - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> > - struct fiemap_extent __user *fi_extents_start; /* Start of
> > - fiemap_extent array */
> > -};
> > -
> > typedef int (*fiemap_fill_cb)(struct fiemap_ctx *f_ctx, u64 logical,
> > u64 phys, u64 len, u32 flags);
> >
> > struct fiemap_ctx {
> > unsigned int fc_flags; /* Flags as passed from user */
> > + unsigned int fc_extents_mapped; /* Number of mapped extents */
> > + unsigned int fc_extents_max; /* Size of fiemap_extent array */
> > void *fc_data;
> > fiemap_fill_cb fc_cb;
> > u64 fc_start;
> > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > index 7ce84be499d6..21643bcf6104 100644
> > --- a/include/linux/iomap.h
> > +++ b/include/linux/iomap.h
> > @@ -9,7 +9,6 @@
> > #include <linux/mm_types.h>
> >
> > struct address_space;
> > -struct fiemap_extent_info;
> > struct inode;
> > struct iov_iter;
> > struct kiocb;
> > --
> > 2.17.1
> >
>
>
> Cheers, Andreas
>
>
>
>
>
--
Carlos
