Re: [PATCH 1/2] xfs_spaceman: XFS_IOC_FIEMAPFS ioctl replacing previous FS_IOC_FIEMAPFS

Dave Chinner
2015-01-28T01:28:08Z

On Fri, Jan 23, 2015 at 09:03:44AM +0530, Dhruvesh Rathore wrote:
> The original kernel patch that Dave wrote for xfs_spaceman implemented
> FS_IOC_FIEMAPFS ioctl, here is the original fiemap extension patch:
>
> http://oss.sgi.com/archives/xfs/2012-10/msg00363.html
>
> These patches were not posted and were just forward ported to a current
> 3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in Dave's
> kernel tree at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git
>
> The original xfs_spaceman tool that Dave wrote to call the fiemap
> interface and make use of it is here:
>
> http://oss.sgi.com/archives/xfs/2012-10/msg00366.html
>
> Dave just updated it to the 3.2.2 code base and pushed it to the
> spaceman branch in this tree:
>
> git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git
>
> This patch is concerned with turning FS_IOC_FIEMAPFS, present in the earlier
> version of xfs_spaceman into an XFS specific ioctl called XFS_IOC_FIEMAPFS
> that uses the fiemap plumbing. Please do comment.

So this commentary belongs in a patch series description, usually
titled "[PATCH 0/X] <patch series title>".

Then the patch itself will have a title/ description along the lines
of:

[PATCH 1/X] xfs: add XFS_IOC_FIEMAPFS

The FS_IOC_FIEMAPFS ioctl is not going to be merged, so convert the
existing code to use an XFS specific ioctl named XFS_IOC_FIEMAPFS
and plumb the code to use this ioctl.

> Signed-off-by:
> Dhruvesh Rathore <dhruvesh_r@outlook.com>
> Amey Ruikar <ameyruikar@yahoo.com>
> Somdeep Dey <somdeepdey10@gmail.com>

One name per s-o-b. i.e.

Signed-off-by: Dhruvesh Rathore <dhruvesh_r@outlook.com>
Signed-off-by: Amey Ruikar <ameyruikar@yahoo.com>
Signed-off-by: Somdeep Dey <somdeepdey10@gmail.com>

> ---
> linux-xfs/fs/xfs/xfs_fs.h | 1 +
> linux-xfs/fs/xfs/xfs_ioctl.c | 71 ++++-
> xfsprogs-3.2.2/include/xfs_fs.h | 1 +
> xfsprogs-3.2.2/spaceman/freesp.c | 10 ++++++---
> 4 files changed, 79 insertions(+), 4 deletions(-)

This needs to be separated into two patches - one for the kernel
changes, and one for the userspace changes. What I'd suggest that
you do is get the changes into a git tree with the correct commit
message that you want, then use git-send-email to send the commit.
That way you'll be sending patches that correctly formatted and
separated by tree.

> --- a/linux-xfs/fs/xfs/xfs_ioctl.c 2015-01-04 15:45:26.518359725 +0530
> +++ b/linux-xfs/fs/xfs/xfs_ioctl.c 2015-01-04 15:24:06.030407817 +0530
> @@ -49,6 +49,8 @@
> #include <linux/exportfs.h>
>
>
> +/* So that the fiemap access checks can't overflow on 32 bit machines. */
> +#define FIEMAP_MAX_EXTENTS (UINT_MAX / sizeof(struct fiemap_extent))

Just move to include/linux/fs.h where the FS_IOC_FIEMAP definitions
are.

Also, there is trailing whitespace on the lines. I'd suggest that
you might like to use scripts/checkpatch.pl to find these simple
whitespace issues. if you use vim, then add this to your local
.vimrc file and it will highlight trailing whitespace in red:

" highlight whitespace damage
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/

> +static int fiemap_check_ranges(struct super_block *sb,
> + u64 start, u64 len, u64 *new_len)
> +{
> + u64 maxbytes = (u64) sb->s_maxbytes;
> +
> + *new_len = len;
> +
> + if (len == 0)
> + return -EINVAL;
> +
> + if (start > maxbytes)
> + return -EFBIG;
> +
> + /*
> + * Shrink request scope to what the fs can actually handle.
> + */
> + if (len > maxbytes || (maxbytes - len) < start)
> + *new_len = maxbytes - start;
>
> + return 0;
> +}

Rather than copying this code, I'd suggest that you remove the "static"
declaration, export the symbol and add a prototype to
include/linux/fs.h similar to fiemap_check_flags().

> +static int xfsctl_fiemapfs(struct super_block *sb, unsigned long arg)

- more trailing whitespace.
- xfs_ioctl_fiemapfs() follows the naming convention used in the
rest of the file
- this is an XFS specific function now, so should pass the xfs_mount
rather than the VFS super_block.
- need a comment saying it was mostly copied from fs/ioctl.c::ioctl_fiemap().
- arg has already marked as __user converted in the calling
function, so we need to retain the __user annotations here.
- format for XFS functions is this:

/*
* Mostly copied from ioctl_fiemap()
*/
static int
xfs_ioctl_fiemapfs(
struct xfs_mount *mp,
void __user *arg)

> +{
> + struct fiemap fiemap;
> + struct fiemap __user *ufiemap = (struct fiemap __user *) arg;
> + struct fiemap_extent_info fieinfo = { 0, };
> + u64 len;
> + int error;

formatting of declarations is usually aligned to that of the
function declaration. i.e:

struct fiemap fiemap;
struct fiemap __user *ufiemap = (struct fiemap __user *) arg;
struct fiemap_extent_info fieinfo = { 0, };
u64 len;
int error;

> + if (!sb->s_op->fiemapfs)
> + return -EOPNOTSUPP;

No need to check this - we can call the XFS function directly.

>
> + if (copy_from_user(&fiemap, ufiemap, sizeof(fiemap)))
> + return -EFAULT;
> +
> + if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> + return -EINVAL;
> +
> + error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> + &len);
> + if (error)
> + return error;
> +
> + fieinfo.fi_flags = fiemap.fm_flags;
> + fieinfo.fi_extents_max = fiemap.fm_extent_count;
> + fieinfo.fi_extents_start = ufiemap->fm_extents;
> +
> + if (fiemap.fm_extent_count != 0 &&
> + !access_ok(VERIFY_WRITE, fieinfo.fi_extents_start,
> + fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> + return -EFAULT;
> +
> + if (fiemap.fm_extent_count != 0 &&
> + (fiemap.fm_flags & FIEMAPFS_FLAG_FREESP_SIZE_HINT) &&
> + !access_ok(VERIFY_READ, fieinfo.fi_extents_start,
> + sizeof(struct fiemap_extent)))
> + return -EFAULT;
> +
> + error = sb->s_op->fiemapfs(sb, &fieinfo, fiemap.fm_start, len);

Call the xfs function directly.

And, in doing so, this will mean you can remove the ->fiemapfs
definitions from include/linux/fs.h

> + fiemap.fm_flags = fieinfo.fi_flags;
> + fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> + if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> + error = -EFAULT;
> +
> + return error;
> +}
>
> /*
> * Note: some of the ioctl's return positive numbers as a
> @@ -1570,7 +1637,9 @@
> return 0;
> }
>
> -
> + case XFS_IOC_FIEMAPFS:
> + return xfsctl_fiemapfs(inode->i_sb, p);

8 space tabs and lots of trailing whitespace.

return xfs_ioctl_fiemapfs(mp, arg);

> --- a/xfsprogs-3.2.2/spaceman/freesp.c 2015-01-04 15:39:58.446372047 +0530
> +++ b/xfsprogs-3.2.2/spaceman/freesp.c 2015-01-04 15:38:30.294375357 +0530
> @@ -31,7 +31,7 @@
> #define FIEMAPFS_FLAG_FREESP_SIZE_HINT 0x20000000
> #define FIEMAPFS_FLAG_FREESP_CONTINUE 0x10000000
>
> -#define FS_IOC_FIEMAPFS _IOWR('f', 12, struct fiemap)
> +#define XFS_IOC_FIEMAPFS _IOWR('X', 33, struct fiemap)
> #endif
>
> typedef struct histent
> @@ -201,9 +201,9 @@
> fiemap->fm_length = length;
> fiemap->fm_extent_count = NR_EXTENTS;
>
> - ret = ioctl(file->fd, XFS_IOC_FIEMAPFS, (unsigned long)fiemap);
> + ret = xfsctl(file->name,file->fd, XFS_IOC_FIEMAPFS, (unsigned long)fiemap);
> if (ret < 0) {
> - fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAPFS) [\"%s\"]: "
> + fprintf(stderr, "%s: xfsctl(XFS_IOC_FIEMAPFS) [\"%s\"]: "
> "%s\n", progname, file->name, strerror(errno));

Also, whitespace damage. There's already some in that patch hunk, so
you may as well fix it while you are changing that code.

> free(fiemap);
> exitcode = 1;
> @@ -338,6 +338,10 @@
>
> if (!init(argc, argv))
> return 0;
> +
> + if (dumpflag)
> + printf("%8s %8s %8s\n", "agno", "agbno", "len");

Added functionality? That should be in a separate patch ;)

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com