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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
