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@xxxxxxxxxxx>
> Amey Ruikar <ameyruikar@xxxxxxxxx>
> Somdeep Dey <somdeepdey10@xxxxxxxxx>
One name per s-o-b. i.e.
Signed-off-by: Dhruvesh Rathore <dhruvesh_r@xxxxxxxxxxx>
Signed-off-by: Amey Ruikar <ameyruikar@xxxxxxxxx>
Signed-off-by: Somdeep Dey <somdeepdey10@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
|