diff mbox

[PATCH-RFC-RESEND,5/9] nfs42: add .copy_range file operation

Message ID 1440577010-122867-6-git-send-email-tao.peng@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peng Tao Aug. 26, 2015, 8:16 a.m. UTC
Signed-off-by: Peng Tao <tao.peng@primarydata.com>
---
 fs/nfs/nfs4file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Dave Chinner Aug. 26, 2015, 10:48 p.m. UTC | #1
On Wed, Aug 26, 2015 at 04:16:46PM +0800, Peng Tao wrote:
> Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> ---
>  fs/nfs/nfs4file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index dcd39d4..c335cb0 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -4,6 +4,7 @@
>   *  Copyright (C) 1992  Rick Sladkey
>   */
>  #include <linux/fs.h>
> +#include <linux/file.h>
>  #include <linux/falloc.h>
>  #include <linux/nfs_fs.h>
>  #include "internal.h"
> @@ -166,6 +167,54 @@ static long nfs42_fallocate(struct file *filep, int mode, loff_t offset, loff_t
>  		return nfs42_proc_deallocate(filep, offset, len);
>  	return nfs42_proc_allocate(filep, offset, len);
>  }
> +
> +static noinline int
> +nfs42_file_clone_range(struct file *src_file, struct file *dst_file,
> +		       loff_t src_off, size_t dst_off, loff_t count)
> +{
> +	struct inode *dst_inode = file_inode(dst_file);
> +	struct inode *src_inode = file_inode(src_file);
> +	int ret;
> +
> +	/* src and dst must be different files */
> +	if (src_inode == dst_inode)
> +		return -EINVAL;
> +
> +	/* XXX: do we lock at all? what if server needs CB_RECALL_LAYOUT? */
> +	if (dst_inode < src_inode) {
> +		mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_PARENT);
> +		mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_CHILD);
> +	} else {
> +		mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_PARENT);
> +		mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_CHILD);
> +	}

Is that safe? Between two operations, the inode code be reclaimed
and re-instantiated, resulting in the second operation having a
different locking order for the same files compared to the
first operation...

> +out_unlock:
> +	if (dst_inode < src_inode) {
> +		mutex_unlock(&src_inode->i_mutex);
> +		mutex_unlock(&dst_inode->i_mutex);
> +	} else {
> +		mutex_unlock(&dst_inode->i_mutex);
> +		mutex_unlock(&src_inode->i_mutex);
> +	}

You don't have to care about lock order on unlock.

Cheers,

Dave.
Peng Tao Aug. 27, 2015, 6:17 a.m. UTC | #2
On Thu, Aug 27, 2015 at 6:48 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Aug 26, 2015 at 04:16:46PM +0800, Peng Tao wrote:
>> Signed-off-by: Peng Tao <tao.peng@primarydata.com>
>> ---
>>  fs/nfs/nfs4file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>> index dcd39d4..c335cb0 100644
>> --- a/fs/nfs/nfs4file.c
>> +++ b/fs/nfs/nfs4file.c
>> @@ -4,6 +4,7 @@
>>   *  Copyright (C) 1992  Rick Sladkey
>>   */
>>  #include <linux/fs.h>
>> +#include <linux/file.h>
>>  #include <linux/falloc.h>
>>  #include <linux/nfs_fs.h>
>>  #include "internal.h"
>> @@ -166,6 +167,54 @@ static long nfs42_fallocate(struct file *filep, int mode, loff_t offset, loff_t
>>               return nfs42_proc_deallocate(filep, offset, len);
>>       return nfs42_proc_allocate(filep, offset, len);
>>  }
>> +
>> +static noinline int
>> +nfs42_file_clone_range(struct file *src_file, struct file *dst_file,
>> +                    loff_t src_off, size_t dst_off, loff_t count)
>> +{
>> +     struct inode *dst_inode = file_inode(dst_file);
>> +     struct inode *src_inode = file_inode(src_file);
>> +     int ret;
>> +
>> +     /* src and dst must be different files */
>> +     if (src_inode == dst_inode)
>> +             return -EINVAL;
>> +
>> +     /* XXX: do we lock at all? what if server needs CB_RECALL_LAYOUT? */
>> +     if (dst_inode < src_inode) {
>> +             mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_PARENT);
>> +             mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_CHILD);
>> +     } else {
>> +             mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_PARENT);
>> +             mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_CHILD);
>> +     }
>
> Is that safe? Between two operations, the inode code be reclaimed
> and re-instantiated, resulting in the second operation having a
> different locking order for the same files compared to the
> first operation...
Both files are still open so I don't think we need to worry about
inode going away.

>
>> +out_unlock:
>> +     if (dst_inode < src_inode) {
>> +             mutex_unlock(&src_inode->i_mutex);
>> +             mutex_unlock(&dst_inode->i_mutex);
>> +     } else {
>> +             mutex_unlock(&dst_inode->i_mutex);
>> +             mutex_unlock(&src_inode->i_mutex);
>> +     }
>
> You don't have to care about lock order on unlock.
Good point!

Thanks,
Tao
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index dcd39d4..c335cb0 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -4,6 +4,7 @@ 
  *  Copyright (C) 1992  Rick Sladkey
  */
 #include <linux/fs.h>
+#include <linux/file.h>
 #include <linux/falloc.h>
 #include <linux/nfs_fs.h>
 #include "internal.h"
@@ -166,6 +167,54 @@  static long nfs42_fallocate(struct file *filep, int mode, loff_t offset, loff_t
 		return nfs42_proc_deallocate(filep, offset, len);
 	return nfs42_proc_allocate(filep, offset, len);
 }
+
+static noinline int
+nfs42_file_clone_range(struct file *src_file, struct file *dst_file,
+		       loff_t src_off, size_t dst_off, loff_t count)
+{
+	struct inode *dst_inode = file_inode(dst_file);
+	struct inode *src_inode = file_inode(src_file);
+	int ret;
+
+	/* src and dst must be different files */
+	if (src_inode == dst_inode)
+		return -EINVAL;
+
+	/* XXX: do we lock at all? what if server needs CB_RECALL_LAYOUT? */
+	if (dst_inode < src_inode) {
+		mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_CHILD);
+	} else {
+		mutex_lock_nested(&src_inode->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&dst_inode->i_mutex, I_MUTEX_CHILD);
+	}
+
+	/* flush all pending writes on both src and dst so that server
+	 * has the latest data */
+	ret = nfs_sync_inode(src_inode);
+	if (ret)
+		goto out_unlock;
+	ret = nfs_sync_inode(dst_inode);
+	if (ret)
+		goto out_unlock;
+
+	ret = nfs42_proc_clone(src_file, dst_file, src_off, dst_off, count);
+
+	/* truncate inode page cache of the dst range so that future reads can fetch
+	 * new data from server */
+	if (!ret)
+		truncate_inode_pages_range(&dst_inode->i_data, dst_off, dst_off + count - 1);
+
+out_unlock:
+	if (dst_inode < src_inode) {
+		mutex_unlock(&src_inode->i_mutex);
+		mutex_unlock(&dst_inode->i_mutex);
+	} else {
+		mutex_unlock(&dst_inode->i_mutex);
+		mutex_unlock(&src_inode->i_mutex);
+	}
+	return ret;
+}
 #endif /* CONFIG_NFS_V4_2 */
 
 const struct file_operations nfs4_file_operations = {
@@ -187,6 +236,7 @@  const struct file_operations nfs4_file_operations = {
 	.splice_write	= iter_file_splice_write,
 #ifdef CONFIG_NFS_V4_2
 	.fallocate	= nfs42_fallocate,
+	.clone_range	= nfs42_file_clone_range,
 #endif /* CONFIG_NFS_V4_2 */
 	.check_flags	= nfs_check_flags,
 	.setlease	= simple_nosetlease,