[RFC,02/11] vfs/btrfs: add .clone_range file operation
diff mbox

Message ID 1440516829-116041-3-git-send-email-tao.peng@primarydata.com
State New
Headers show

Commit Message

Peng Tao Aug. 25, 2015, 3:33 p.m. UTC
Also add vfs callers in do_vfs_ioctl(). Now btrfs CLONE
ioctl goes through vfs_file_clone_range().

Cc: linux-btrfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Peng Tao <tao.peng@primarydata.com>
---
 fs/btrfs/ctree.h           |  2 ++
 fs/btrfs/file.c            |  1 +
 fs/btrfs/ioctl.c           | 68 +++++-----------------------------------------
 fs/ioctl.c                 |  6 ++++
 include/uapi/linux/btrfs.h | 10 -------
 5 files changed, 16 insertions(+), 71 deletions(-)

Comments

David Sterba Aug. 26, 2015, 8:01 a.m. UTC | #1
On Tue, Aug 25, 2015 at 11:33:40PM +0800, Peng Tao wrote:
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -316,12 +316,6 @@ struct btrfs_ioctl_search_args_v2 {
>  	__u64 buf[0];                       /* out - found items */
>  };
>  
> -struct btrfs_ioctl_clone_range_args {
> -  __s64 src_fd;
> -  __u64 src_offset, src_length;
> -  __u64 dest_offset;
> -};

For backward compatibility and not-breaking-builds reasons, do not
remove anything from this file.

> -
>  /* flags for the defrag range ioctl */
>  #define BTRFS_DEFRAG_RANGE_COMPRESS 1
>  #define BTRFS_DEFRAG_RANGE_START_IO 2
> @@ -548,7 +542,6 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
>  #define BTRFS_IOC_TRANS_END    _IO(BTRFS_IOCTL_MAGIC, 7)
>  #define BTRFS_IOC_SYNC         _IO(BTRFS_IOCTL_MAGIC, 8)
>  
> -#define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)

The ioctl definition reuses the BTRFS_IOCTL_MAGIC (0x94), which is IMHO
wrong.

I'll comment more once the whole series is posted to fsdevel.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peng Tao Aug. 26, 2015, 8:31 a.m. UTC | #2
On Wed, Aug 26, 2015 at 4:01 PM, David Sterba <dsterba@suse.cz> wrote:
> On Tue, Aug 25, 2015 at 11:33:40PM +0800, Peng Tao wrote:
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -316,12 +316,6 @@ struct btrfs_ioctl_search_args_v2 {
>>       __u64 buf[0];                       /* out - found items */
>>  };
>>
>> -struct btrfs_ioctl_clone_range_args {
>> -  __s64 src_fd;
>> -  __u64 src_offset, src_length;
>> -  __u64 dest_offset;
>> -};
>
> For backward compatibility and not-breaking-builds reasons, do not
> remove anything from this file.
>
Got you. I'll keep it unchanged in the next version.

>> -
>>  /* flags for the defrag range ioctl */
>>  #define BTRFS_DEFRAG_RANGE_COMPRESS 1
>>  #define BTRFS_DEFRAG_RANGE_START_IO 2
>> @@ -548,7 +542,6 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
>>  #define BTRFS_IOC_TRANS_END    _IO(BTRFS_IOCTL_MAGIC, 7)
>>  #define BTRFS_IOC_SYNC         _IO(BTRFS_IOCTL_MAGIC, 8)
>>
>> -#define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
>
> The ioctl definition reuses the BTRFS_IOCTL_MAGIC (0x94), which is IMHO
> wrong.
>
I thought it breaks ABI if we choose a different value for the type
field of the ioctl. Am I misunderstanding it?

Thanks,
Tao
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Aug. 26, 2015, 1 p.m. UTC | #3
On Wed, Aug 26, 2015 at 04:31:09PM +0800, Peng Tao wrote:
> >> -#define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
> >
> > The ioctl definition reuses the BTRFS_IOCTL_MAGIC (0x94), which is IMHO
> > wrong.
> >
> I thought it breaks ABI if we choose a different value for the type
> field of the ioctl. Am I misunderstanding it?

The ioctl is now private to btrfs, you're going to define the new ABI so
it IMHO should use the generic filesystem ioctl magic/namespace.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Aug. 26, 2015, 1:07 p.m. UTC | #4
On Wed, Aug 26, 2015 at 03:00:57PM +0200, David Sterba wrote:
> > I thought it breaks ABI if we choose a different value for the type
> > field of the ioctl. Am I misunderstanding it?
> 
> The ioctl is now private to btrfs, you're going to define the new ABI so
> it IMHO should use the generic filesystem ioctl magic/namespace.

No, that's completly backwards.  We move up ioctls that have prooven
to be generally useful to common code, and we've done that with various
extN and XFS ones already.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index aac314e..af3e224 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3969,6 +3969,8 @@  void btrfs_get_block_group_info(struct list_head *groups_list,
 				struct btrfs_ioctl_space_info *space);
 void update_ioctl_balance_args(struct btrfs_fs_info *fs_info, int lock,
 			       struct btrfs_ioctl_balance_args *bargs);
+int btrfs_file_clone_range(struct file *src_file, struct file *dst_file,
+			   loff_t off, size_t olen, loff_t destoff);
 
 
 /* file.c */
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index b823fac..c9170fd 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2813,6 +2813,7 @@  const struct file_operations btrfs_file_operations = {
 	.fsync		= btrfs_sync_file,
 	.fallocate	= btrfs_fallocate,
 	.unlocked_ioctl	= btrfs_ioctl,
+	.clone_range	= btrfs_file_clone_range,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= btrfs_ioctl,
 #endif
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0770c91..3d026c8 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3719,13 +3719,12 @@  out:
 	return ret;
 }
 
-static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
-				       u64 off, u64 olen, u64 destoff)
+int btrfs_file_clone_range(struct file *src_file, struct file *dst_file,
+			   loff_t off, size_t olen, loff_t destoff)
 {
-	struct inode *inode = file_inode(file);
+	struct inode *inode = file_inode(dst_file);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct fd src_file;
-	struct inode *src;
+	struct inode *src = file_inode(src_file);
 	int ret;
 	u64 len = olen;
 	u64 bs = root->fs_info->sb->s_blocksize;
@@ -3742,49 +3741,16 @@  static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
 	 *   be either compressed or non-compressed.
 	 */
 
-	/* the destination must be opened for writing */
-	if (!(file->f_mode & FMODE_WRITE) || (file->f_flags & O_APPEND))
-		return -EINVAL;
-
 	if (btrfs_root_readonly(root))
 		return -EROFS;
 
-	ret = mnt_want_write_file(file);
-	if (ret)
-		return ret;
-
-	src_file = fdget(srcfd);
-	if (!src_file.file) {
-		ret = -EBADF;
-		goto out_drop_write;
-	}
-
-	ret = -EXDEV;
-	if (src_file.file->f_path.mnt != file->f_path.mnt)
-		goto out_fput;
-
-	src = file_inode(src_file.file);
-
-	ret = -EINVAL;
-	if (src == inode)
-		same_inode = 1;
-
-	/* the src must be open for reading */
-	if (!(src_file.file->f_mode & FMODE_READ))
-		goto out_fput;
-
 	/* don't make the dst file partly checksummed */
 	if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !=
 	    (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
-		goto out_fput;
-
-	ret = -EISDIR;
-	if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -EINVAL;
 
-	ret = -EXDEV;
-	if (src->i_sb != inode->i_sb)
-		goto out_fput;
+	if (src == inode)
+		same_inode = 1;
 
 	if (!same_inode) {
 		if (inode < src) {
@@ -3800,8 +3766,6 @@  static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
 
 	/* determine range to clone */
 	ret = -EINVAL;
-	if (off + len > src->i_size || off + len < off)
-		goto out_unlock;
 	if (len == 0)
 		olen = len = src->i_size - off;
 	/* if we extend to eof, continue to block boundary */
@@ -3877,23 +3841,9 @@  out_unlock:
 	} else {
 		mutex_unlock(&src->i_mutex);
 	}
-out_fput:
-	fdput(src_file);
-out_drop_write:
-	mnt_drop_write_file(file);
 	return ret;
 }
 
-static long btrfs_ioctl_clone_range(struct file *file, void __user *argp)
-{
-	struct btrfs_ioctl_clone_range_args args;
-
-	if (copy_from_user(&args, argp, sizeof(args)))
-		return -EFAULT;
-	return btrfs_ioctl_clone(file, args.src_fd, args.src_offset,
-				 args.src_length, args.dest_offset);
-}
-
 /*
  * there are many ways the trans_start and trans_end ioctls can lead
  * to deadlocks.  They should only be used by applications that
@@ -5433,10 +5383,6 @@  long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_dev_info(root, argp);
 	case BTRFS_IOC_BALANCE:
 		return btrfs_ioctl_balance(file, NULL);
-	case BTRFS_IOC_CLONE:
-		return btrfs_ioctl_clone(file, arg, 0, 0, 0);
-	case BTRFS_IOC_CLONE_RANGE:
-		return btrfs_ioctl_clone_range(file, argp);
 	case BTRFS_IOC_TRANS_START:
 		return btrfs_ioctl_trans_start(file);
 	case BTRFS_IOC_TRANS_END:
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 726c5d7..41c6080 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -624,6 +624,12 @@  int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 	case FIGETBSZ:
 		return put_user(inode->i_sb->s_blocksize, argp);
 
+	case FICLONE:
+		return ioctl_file_clone(filp, arg, 0, 0, 0);
+
+	case FICLONERANGE:
+		return ioctl_file_clone_range(filp, argp);
+
 	default:
 		if (S_ISREG(inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index b6dec05..c3a23d3 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -316,12 +316,6 @@  struct btrfs_ioctl_search_args_v2 {
 	__u64 buf[0];                       /* out - found items */
 };
 
-struct btrfs_ioctl_clone_range_args {
-  __s64 src_fd;
-  __u64 src_offset, src_length;
-  __u64 dest_offset;
-};
-
 /* flags for the defrag range ioctl */
 #define BTRFS_DEFRAG_RANGE_COMPRESS 1
 #define BTRFS_DEFRAG_RANGE_START_IO 2
@@ -548,7 +542,6 @@  static inline char *btrfs_err_str(enum btrfs_err_code err_code)
 #define BTRFS_IOC_TRANS_END    _IO(BTRFS_IOCTL_MAGIC, 7)
 #define BTRFS_IOC_SYNC         _IO(BTRFS_IOCTL_MAGIC, 8)
 
-#define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
 #define BTRFS_IOC_ADD_DEV _IOW(BTRFS_IOCTL_MAGIC, 10, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_RM_DEV _IOW(BTRFS_IOCTL_MAGIC, 11, \
@@ -556,9 +549,6 @@  static inline char *btrfs_err_str(enum btrfs_err_code err_code)
 #define BTRFS_IOC_BALANCE _IOW(BTRFS_IOCTL_MAGIC, 12, \
 				   struct btrfs_ioctl_vol_args)
 
-#define BTRFS_IOC_CLONE_RANGE _IOW(BTRFS_IOCTL_MAGIC, 13, \
-				  struct btrfs_ioctl_clone_range_args)
-
 #define BTRFS_IOC_SUBVOL_CREATE _IOW(BTRFS_IOCTL_MAGIC, 14, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_SNAP_DESTROY _IOW(BTRFS_IOCTL_MAGIC, 15, \