[PATCH 1/3] f2fs: avoid memory allocation failure due to a long length

[PATCH 1/3] f2fs: avoid memory allocation failure due to a long length

[Jaegeuk Kim]

We need to avoid ENOMEM due to unexpected long length.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 17b3059..3573b07 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1039,33 +1039,43 @@ static int __clone_blkaddrs(struct inode *src_inode, struct inode *dst_inode,
 
 static int __exchange_data_block(struct inode *src_inode,
 			struct inode *dst_inode, pgoff_t src, pgoff_t dst,
-			int len, bool full)
+			pgoff_t len, bool full)
 {
 	block_t *src_blkaddr;
 	int *do_replace;
+	pgoff_t olen;
 	int ret;
 
-	src_blkaddr = f2fs_kvzalloc(sizeof(block_t) * len, GFP_KERNEL);
-	if (!src_blkaddr)
-		return -ENOMEM;
+	while (len) {
+		olen = min((pgoff_t)4 * ADDRS_PER_BLOCK, len);
 
-	do_replace = f2fs_kvzalloc(sizeof(int) * len, GFP_KERNEL);
-	if (!do_replace) {
-		kvfree(src_blkaddr);
-		return -ENOMEM;
-	}
+		src_blkaddr = f2fs_kvzalloc(sizeof(block_t) * olen, GFP_KERNEL);
+		if (!src_blkaddr)
+			return -ENOMEM;
 
-	ret = __read_out_blkaddrs(src_inode, src_blkaddr, do_replace, src, len);
-	if (ret)
-		goto roll_back;
+		do_replace = f2fs_kvzalloc(sizeof(int) * olen, GFP_KERNEL);
+		if (!do_replace) {
+			kvfree(src_blkaddr);
+			return -ENOMEM;
+		}
 
-	ret = __clone_blkaddrs(src_inode, dst_inode, src_blkaddr,
-					do_replace, src, dst, len, full);
-	if (ret)
-		goto roll_back;
+		ret = __read_out_blkaddrs(src_inode, src_blkaddr,
+					do_replace, src, olen);
+		if (ret)
+			goto roll_back;
 
-	kvfree(src_blkaddr);
-	kvfree(do_replace);
+		ret = __clone_blkaddrs(src_inode, dst_inode, src_blkaddr,
+					do_replace, src, dst, olen, full);
+		if (ret)
+			goto roll_back;
+
+		src += olen;
+		dst += olen;
+		len -= olen;
+
+		kvfree(src_blkaddr);
+		kvfree(do_replace);
+	}
 	return 0;
 
 roll_back:
-- 
2.8.3

[PATCH 2/3] f2fs: support copy_file_range

[Jaegeuk Kim]

This patch implements copy_file_range in f2fs.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3573b07..c2b7e35 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2215,6 +2215,103 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	return ret;
 }
 
+static int f2fs_clone_files(struct file *file_in, loff_t pos_in,
+				struct file *file_out, loff_t pos_out,
+				size_t len)
+{
+	struct inode *src = file_inode(file_in);
+	struct inode *dst = file_inode(file_out);
+	struct f2fs_sb_info *sbi = F2FS_I_SB(src);
+	size_t olen = len, dst_max_i_size = 0;
+	size_t dst_osize;
+	int ret;
+
+	if (file_in->f_path.mnt != file_out->f_path.mnt ||
+				src->i_sb != dst->i_sb)
+		return -EXDEV;
+
+	if (unlikely(f2fs_readonly(src->i_sb)))
+		return -EROFS;
+
+	if (S_ISDIR(src->i_mode) || S_ISDIR(dst->i_mode))
+		return -EISDIR;
+
+	inode_lock(src);
+	if (src != dst)
+		inode_lock(dst);
+
+	ret = -EINVAL;
+	if (pos_in + len > src->i_size || pos_in + len < pos_in)
+		goto out_unlock;
+	if (len == 0)
+		olen = len = src->i_size - pos_in;
+	if (pos_in + len == src->i_size)
+		len = ALIGN(src->i_size, F2FS_BLKSIZE) - pos_in;
+	if (len == 0) {
+		ret = 0;
+		goto out_unlock;
+	}
+
+	dst_osize = dst->i_size;
+	if (pos_out + olen > dst->i_size)
+		dst_max_i_size = pos_out + olen;
+
+	/* verify the end result is block aligned */
+	if (!IS_ALIGNED(pos_in, F2FS_BLKSIZE) ||
+			!IS_ALIGNED(pos_in + len, F2FS_BLKSIZE) ||
+			!IS_ALIGNED(pos_out, F2FS_BLKSIZE))
+		goto out_unlock;
+
+	ret = f2fs_convert_inline_inode(src);
+	if (ret)
+		goto out_unlock;
+
+	ret = f2fs_convert_inline_inode(dst);
+	if (ret)
+		goto out_unlock;
+
+	/* write out all dirty pages from offset */
+	ret = filemap_write_and_wait_range(src->i_mapping,
+					pos_in, pos_in + len);
+	if (ret)
+		goto out_unlock;
+
+	ret = filemap_write_and_wait_range(dst->i_mapping,
+					pos_out, pos_out + len);
+	if (ret)
+		goto out_unlock;
+
+	f2fs_balance_fs(sbi, true);
+	f2fs_lock_op(sbi);
+	ret = __exchange_data_block(src, dst, pos_in,
+				pos_out, len >> F2FS_BLKSIZE_BITS, false);
+
+	if (!ret) {
+		if (dst_max_i_size)
+			f2fs_i_size_write(dst, dst_max_i_size);
+		else if (dst_osize != dst->i_size)
+			f2fs_i_size_write(dst, dst_osize);
+	}
+	f2fs_unlock_op(sbi);
+out_unlock:
+	if (src != dst)
+		inode_unlock(dst);
+	inode_unlock(src);
+	return ret;
+}
+
+ssize_t f2fs_copy_file_range(struct file *file_in, loff_t pos_in,
+		struct file *file_out, loff_t pos_out,
+		size_t len, unsigned int flags)
+{
+	ssize_t ret;
+
+	ret = f2fs_clone_files(file_in, pos_in, file_out, pos_out, len);
+	if (!ret)
+		ret = len;
+	return ret;
+}
+
 #ifdef CONFIG_COMPAT
 long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -2261,6 +2358,7 @@ const struct file_operations f2fs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= f2fs_compat_ioctl,
 #endif
+	.copy_file_range = f2fs_copy_file_range,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 };
-- 
2.8.3

Re: [PATCH 2/3] f2fs: support copy_file_range

[Jaegeuk Kim]

Hi,

Please ignore copy/clone_file_range patches, which are wrong implementation.
Sorry for the noise.

Thanks,

On Sat, Jul 16, 2016 at 11:24:26PM -0700, Jaegeuk Kim wrote:
> This patch implements copy_file_range in f2fs.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/file.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3573b07..c2b7e35 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2215,6 +2215,103 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	return ret;
>  }
>  
> +static int f2fs_clone_files(struct file *file_in, loff_t pos_in,
> +				struct file *file_out, loff_t pos_out,
> +				size_t len)
> +{
> +	struct inode *src = file_inode(file_in);
> +	struct inode *dst = file_inode(file_out);
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(src);
> +	size_t olen = len, dst_max_i_size = 0;
> +	size_t dst_osize;
> +	int ret;
> +
> +	if (file_in->f_path.mnt != file_out->f_path.mnt ||
> +				src->i_sb != dst->i_sb)
> +		return -EXDEV;
> +
> +	if (unlikely(f2fs_readonly(src->i_sb)))
> +		return -EROFS;
> +
> +	if (S_ISDIR(src->i_mode) || S_ISDIR(dst->i_mode))
> +		return -EISDIR;
> +
> +	inode_lock(src);
> +	if (src != dst)
> +		inode_lock(dst);
> +
> +	ret = -EINVAL;
> +	if (pos_in + len > src->i_size || pos_in + len < pos_in)
> +		goto out_unlock;
> +	if (len == 0)
> +		olen = len = src->i_size - pos_in;
> +	if (pos_in + len == src->i_size)
> +		len = ALIGN(src->i_size, F2FS_BLKSIZE) - pos_in;
> +	if (len == 0) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> +
> +	dst_osize = dst->i_size;
> +	if (pos_out + olen > dst->i_size)
> +		dst_max_i_size = pos_out + olen;
> +
> +	/* verify the end result is block aligned */
> +	if (!IS_ALIGNED(pos_in, F2FS_BLKSIZE) ||
> +			!IS_ALIGNED(pos_in + len, F2FS_BLKSIZE) ||
> +			!IS_ALIGNED(pos_out, F2FS_BLKSIZE))
> +		goto out_unlock;
> +
> +	ret = f2fs_convert_inline_inode(src);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = f2fs_convert_inline_inode(dst);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* write out all dirty pages from offset */
> +	ret = filemap_write_and_wait_range(src->i_mapping,
> +					pos_in, pos_in + len);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = filemap_write_and_wait_range(dst->i_mapping,
> +					pos_out, pos_out + len);
> +	if (ret)
> +		goto out_unlock;
> +
> +	f2fs_balance_fs(sbi, true);
> +	f2fs_lock_op(sbi);
> +	ret = __exchange_data_block(src, dst, pos_in,
> +				pos_out, len >> F2FS_BLKSIZE_BITS, false);
> +
> +	if (!ret) {
> +		if (dst_max_i_size)
> +			f2fs_i_size_write(dst, dst_max_i_size);
> +		else if (dst_osize != dst->i_size)
> +			f2fs_i_size_write(dst, dst_osize);
> +	}
> +	f2fs_unlock_op(sbi);
> +out_unlock:
> +	if (src != dst)
> +		inode_unlock(dst);
> +	inode_unlock(src);
> +	return ret;
> +}
> +
> +ssize_t f2fs_copy_file_range(struct file *file_in, loff_t pos_in,
> +		struct file *file_out, loff_t pos_out,
> +		size_t len, unsigned int flags)
> +{
> +	ssize_t ret;
> +
> +	ret = f2fs_clone_files(file_in, pos_in, file_out, pos_out, len);
> +	if (!ret)
> +		ret = len;
> +	return ret;
> +}
> +
>  #ifdef CONFIG_COMPAT
>  long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
> @@ -2261,6 +2358,7 @@ const struct file_operations f2fs_file_operations = {
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl	= f2fs_compat_ioctl,
>  #endif
> +	.copy_file_range = f2fs_copy_file_range,
>  	.splice_read	= generic_file_splice_read,
>  	.splice_write	= iter_file_splice_write,
>  };
> -- 
> 2.8.3

[PATCH 3/3] f2fs: support clone_file_range

[Jaegeuk Kim]

This patch implements clone_file_range in f2fs.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index c2b7e35..37480f3 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2312,6 +2312,12 @@ ssize_t f2fs_copy_file_range(struct file *file_in, loff_t pos_in,
 	return ret;
 }
 
+int f2fs_clone_file_range(struct file *file_in, loff_t pos_in,
+		struct file *file_out, loff_t pos_out, u64 len)
+{
+	return f2fs_clone_files(file_in, pos_in, file_out, pos_out, len);
+}
+
 #ifdef CONFIG_COMPAT
 long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -2359,6 +2365,7 @@ const struct file_operations f2fs_file_operations = {
 	.compat_ioctl	= f2fs_compat_ioctl,
 #endif
 	.copy_file_range = f2fs_copy_file_range,
+	.clone_file_range = f2fs_clone_file_range,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 };
-- 
2.8.3

Re: [PATCH 3/3] f2fs: support clone_file_range

[Christoph Hellwig]

On Sat, Jul 16, 2016 at 11:24:27PM -0700, Jaegeuk Kim wrote:
> This patch implements clone_file_range in f2fs.

[...]

> +int f2fs_clone_file_range(struct file *file_in, loff_t pos_in,
> +		struct file *file_out, loff_t pos_out, u64 len)
> +{
> +	return f2fs_clone_files(file_in, pos_in, file_out, pos_out, len);
> +}
> +

Falling back from copy to clone should be done in the VFS.  I look into
implementing the fallback, and the code is trivial, but we don't seem
to have any useful coverage for copy_file_range yet, so I didn't dare
to add it yet.  How did you test copy and clone in f2fs?  And how did
you handle the difference in corner cases (e.g. the lacking 0 special
case in copy)?

Re: [PATCH 3/3] f2fs: support clone_file_range

[Jaegeuk Kim]

On Mon, Jul 18, 2016 at 08:47:36PM -0700, Christoph Hellwig wrote:
> On Sat, Jul 16, 2016 at 11:24:27PM -0700, Jaegeuk Kim wrote:
> > This patch implements clone_file_range in f2fs.
> 
> [...]
> 
> > +int f2fs_clone_file_range(struct file *file_in, loff_t pos_in,
> > +		struct file *file_out, loff_t pos_out, u64 len)
> > +{
> > +	return f2fs_clone_files(file_in, pos_in, file_out, pos_out, len);
> > +}
> > +
> 
> Falling back from copy to clone should be done in the VFS.  I look into
> implementing the fallback, and the code is trivial, but we don't seem
> to have any useful coverage for copy_file_range yet, so I didn't dare
> to add it yet.  How did you test copy and clone in f2fs?  And how did
> you handle the difference in corner cases (e.g. the lacking 0 special
> case in copy)?

Frankly speaking, I confused the behaviors without being aware of corner cases.
And thus, I dropped the patches already. Instead, I realized that what I did
was moving a range of blocks from one file to another file. So, I've been
testing a new ioctl, F2FS_IOC_MOVE_RANGE.
In terms of coverage, I just did some unit tests only.

Thanks,