[PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
option of a file.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/f2fs.h |  7 +++++++
 fs/f2fs/file.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 53fe2853579c..a33c90cf979b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -433,6 +433,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
 					_IOR(F2FS_IOCTL_MAGIC, 19, __u64)
 #define F2FS_IOC_SEC_TRIM_FILE		_IOW(F2FS_IOCTL_MAGIC, 20,	\
 						struct f2fs_sectrim_range)
+#define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
+						struct f2fs_comp_option)
 
 /*
  * should be same as XFS_IOC_GOINGDOWN.
@@ -481,6 +483,11 @@ struct f2fs_sectrim_range {
 	u64 flags;
 };
 
+struct f2fs_comp_option {
+	u8 algorithm;
+	u8 log_cluster_size;
+};
+
 /* for inline stuff */
 #define DEF_INLINE_RESERVED_SIZE	1
 static inline int get_extra_isize(struct inode *inode);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ef5a844de53f..7e64259f6f5e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3936,6 +3936,33 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
 	return ret;
 }
 
+static int f2fs_ioc_get_compress_option(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = file_inode(filp);
+	struct f2fs_comp_option option;
+
+	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
+		return -EOPNOTSUPP;
+
+	inode_lock(inode);
+
+	if (!f2fs_compressed_file(inode)) {
+		inode_unlock(inode);
+		return -EINVAL;
+	}
+
+	option.algorithm = F2FS_I(inode)->i_compress_algorithm;
+	option.log_cluster_size = F2FS_I(inode)->i_log_cluster_size;
+
+	inode_unlock(inode);
+
+	if (copy_to_user((struct f2fs_comp_option __user *)arg, &option,
+				sizeof(option)))
+		return -EFAULT;
+
+	return 0;
+}
+
 long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(filp)))))
@@ -4024,6 +4051,8 @@ long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_reserve_compress_blocks(filp, arg);
 	case F2FS_IOC_SEC_TRIM_FILE:
 		return f2fs_sec_trim_file(filp, arg);
+	case F2FS_IOC_GET_COMPRESS_OPTION:
+		return f2fs_ioc_get_compress_option(filp, arg);
 	default:
 		return -ENOTTY;
 	}
@@ -4194,6 +4223,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RELEASE_COMPRESS_BLOCKS:
 	case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
 	case F2FS_IOC_SEC_TRIM_FILE:
+	case F2FS_IOC_GET_COMPRESS_OPTION:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.28.0.1011.ga647a8990f-goog

[f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
option of a file.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/f2fs.h |  7 +++++++
 fs/f2fs/file.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 53fe2853579c..a33c90cf979b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -433,6 +433,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
 					_IOR(F2FS_IOCTL_MAGIC, 19, __u64)
 #define F2FS_IOC_SEC_TRIM_FILE		_IOW(F2FS_IOCTL_MAGIC, 20,	\
 						struct f2fs_sectrim_range)
+#define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
+						struct f2fs_comp_option)
 
 /*
  * should be same as XFS_IOC_GOINGDOWN.
@@ -481,6 +483,11 @@ struct f2fs_sectrim_range {
 	u64 flags;
 };
 
+struct f2fs_comp_option {
+	u8 algorithm;
+	u8 log_cluster_size;
+};
+
 /* for inline stuff */
 #define DEF_INLINE_RESERVED_SIZE	1
 static inline int get_extra_isize(struct inode *inode);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ef5a844de53f..7e64259f6f5e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3936,6 +3936,33 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
 	return ret;
 }
 
+static int f2fs_ioc_get_compress_option(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = file_inode(filp);
+	struct f2fs_comp_option option;
+
+	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
+		return -EOPNOTSUPP;
+
+	inode_lock(inode);
+
+	if (!f2fs_compressed_file(inode)) {
+		inode_unlock(inode);
+		return -EINVAL;
+	}
+
+	option.algorithm = F2FS_I(inode)->i_compress_algorithm;
+	option.log_cluster_size = F2FS_I(inode)->i_log_cluster_size;
+
+	inode_unlock(inode);
+
+	if (copy_to_user((struct f2fs_comp_option __user *)arg, &option,
+				sizeof(option)))
+		return -EFAULT;
+
+	return 0;
+}
+
 long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(filp)))))
@@ -4024,6 +4051,8 @@ long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_reserve_compress_blocks(filp, arg);
 	case F2FS_IOC_SEC_TRIM_FILE:
 		return f2fs_sec_trim_file(filp, arg);
+	case F2FS_IOC_GET_COMPRESS_OPTION:
+		return f2fs_ioc_get_compress_option(filp, arg);
 	default:
 		return -ENOTTY;
 	}
@@ -4194,6 +4223,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RELEASE_COMPRESS_BLOCKS:
 	case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
 	case F2FS_IOC_SEC_TRIM_FILE:
+	case F2FS_IOC_GET_COMPRESS_OPTION:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.28.0.1011.ga647a8990f-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Added a new F2FS_IOC_SET_COMPRESS_OPTION ioctl to change file
compression option of a file.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/f2fs.h |  2 ++
 fs/f2fs/file.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a33c90cf979b..5ee8a4859b62 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -435,6 +435,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
 						struct f2fs_sectrim_range)
 #define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
 						struct f2fs_comp_option)
+#define F2FS_IOC_SET_COMPRESS_OPTION	_IOW(F2FS_IOCTL_MAGIC, 22,	\
+						struct f2fs_comp_option)
 
 /*
  * should be same as XFS_IOC_GOINGDOWN.
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7e64259f6f5e..6c265c66ddd4 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3963,6 +3963,59 @@ static int f2fs_ioc_get_compress_option(struct file *filp, unsigned long arg)
 	return 0;
 }
 
+static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = file_inode(filp);
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct f2fs_comp_option option;
+	int ret;
+	int writecount;
+
+	if (!f2fs_sb_has_compression(sbi))
+		return -EOPNOTSUPP;
+
+	if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
+		return -EINVAL;
+
+	if (f2fs_readonly(sbi->sb))
+		return -EROFS;
+
+	if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
+				sizeof(option)))
+		return -EFAULT;
+
+	if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
+			option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
+			option.algorithm >= COMPRESS_MAX)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;
+
+	inode_lock(inode);
+
+	writecount = atomic_read(&inode->i_writecount);
+	if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
+			(!(filp->f_mode & FMODE_WRITE) && writecount)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (get_dirty_pages(inode) || inode->i_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	F2FS_I(inode)->i_compress_algorithm = option.algorithm;
+	F2FS_I(inode)->i_log_cluster_size = option.log_cluster_size;
+	F2FS_I(inode)->i_cluster_size = 1 << option.log_cluster_size;
+	f2fs_mark_inode_dirty_sync(inode, true);
+out:
+	inode_unlock(inode);
+	return ret;
+}
+
 long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(filp)))))
@@ -4053,6 +4106,8 @@ long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_sec_trim_file(filp, arg);
 	case F2FS_IOC_GET_COMPRESS_OPTION:
 		return f2fs_ioc_get_compress_option(filp, arg);
+	case F2FS_IOC_SET_COMPRESS_OPTION:
+		return f2fs_ioc_set_compress_option(filp, arg);
 	default:
 		return -ENOTTY;
 	}
@@ -4224,6 +4279,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
 	case F2FS_IOC_SEC_TRIM_FILE:
 	case F2FS_IOC_GET_COMPRESS_OPTION:
+	case F2FS_IOC_SET_COMPRESS_OPTION:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.28.0.1011.ga647a8990f-goog

[f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Added a new F2FS_IOC_SET_COMPRESS_OPTION ioctl to change file
compression option of a file.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/f2fs.h |  2 ++
 fs/f2fs/file.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a33c90cf979b..5ee8a4859b62 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -435,6 +435,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
 						struct f2fs_sectrim_range)
 #define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
 						struct f2fs_comp_option)
+#define F2FS_IOC_SET_COMPRESS_OPTION	_IOW(F2FS_IOCTL_MAGIC, 22,	\
+						struct f2fs_comp_option)
 
 /*
  * should be same as XFS_IOC_GOINGDOWN.
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7e64259f6f5e..6c265c66ddd4 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3963,6 +3963,59 @@ static int f2fs_ioc_get_compress_option(struct file *filp, unsigned long arg)
 	return 0;
 }
 
+static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = file_inode(filp);
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct f2fs_comp_option option;
+	int ret;
+	int writecount;
+
+	if (!f2fs_sb_has_compression(sbi))
+		return -EOPNOTSUPP;
+
+	if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
+		return -EINVAL;
+
+	if (f2fs_readonly(sbi->sb))
+		return -EROFS;
+
+	if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
+				sizeof(option)))
+		return -EFAULT;
+
+	if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
+			option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
+			option.algorithm >= COMPRESS_MAX)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;
+
+	inode_lock(inode);
+
+	writecount = atomic_read(&inode->i_writecount);
+	if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
+			(!(filp->f_mode & FMODE_WRITE) && writecount)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (get_dirty_pages(inode) || inode->i_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	F2FS_I(inode)->i_compress_algorithm = option.algorithm;
+	F2FS_I(inode)->i_log_cluster_size = option.log_cluster_size;
+	F2FS_I(inode)->i_cluster_size = 1 << option.log_cluster_size;
+	f2fs_mark_inode_dirty_sync(inode, true);
+out:
+	inode_unlock(inode);
+	return ret;
+}
+
 long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(filp)))))
@@ -4053,6 +4106,8 @@ long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_sec_trim_file(filp, arg);
 	case F2FS_IOC_GET_COMPRESS_OPTION:
 		return f2fs_ioc_get_compress_option(filp, arg);
+	case F2FS_IOC_SET_COMPRESS_OPTION:
+		return f2fs_ioc_set_compress_option(filp, arg);
 	default:
 		return -ENOTTY;
 	}
@@ -4224,6 +4279,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
 	case F2FS_IOC_SEC_TRIM_FILE:
 	case F2FS_IOC_GET_COMPRESS_OPTION:
+	case F2FS_IOC_SET_COMPRESS_OPTION:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.28.0.1011.ga647a8990f-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Tue, Oct 13, 2020 at 11:24:29AM +0900, Daeho Jeong wrote:
> +static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> +{
> +	struct inode *inode = file_inode(filp);
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	struct f2fs_comp_option option;
> +	int ret;
> +	int writecount;
> +
> +	if (!f2fs_sb_has_compression(sbi))
> +		return -EOPNOTSUPP;
> +
> +	if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
> +		return -EINVAL;
> +
> +	if (f2fs_readonly(sbi->sb))
> +		return -EROFS;

f2fs_readonly() is redundant with mnt_want_write_file().

Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
be called on a file owned by another user, as long as the caller has read
access.

Note: if you change this to require a writable file descriptor, then
f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
be needed.

> +
> +	if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
> +				sizeof(option)))
> +		return -EFAULT;
> +
> +	if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
> +			option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
> +			option.algorithm >= COMPRESS_MAX)
> +		return -EINVAL;

What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?

> +
> +	ret = mnt_want_write_file(filp);
> +	if (ret)
> +		return ret;
> +
> +	inode_lock(inode);
> +
> +	writecount = atomic_read(&inode->i_writecount);
> +	if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
> +			(!(filp->f_mode & FMODE_WRITE) && writecount)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

I don't think the check for i_writecount == 1 accomplishes anything because it
just means there are no *other* writable file descriptors.  It doesn't mean that
some other thread isn't concurrently trying to write to this same file
descriptor.  So the lock needs to be enough.  Is it?

- Eric

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Tue, Oct 13, 2020 at 11:24:29AM +0900, Daeho Jeong wrote:
> +static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> +{
> +	struct inode *inode = file_inode(filp);
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	struct f2fs_comp_option option;
> +	int ret;
> +	int writecount;
> +
> +	if (!f2fs_sb_has_compression(sbi))
> +		return -EOPNOTSUPP;
> +
> +	if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
> +		return -EINVAL;
> +
> +	if (f2fs_readonly(sbi->sb))
> +		return -EROFS;

f2fs_readonly() is redundant with mnt_want_write_file().

Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
be called on a file owned by another user, as long as the caller has read
access.

Note: if you change this to require a writable file descriptor, then
f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
be needed.

> +
> +	if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
> +				sizeof(option)))
> +		return -EFAULT;
> +
> +	if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
> +			option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
> +			option.algorithm >= COMPRESS_MAX)
> +		return -EINVAL;

What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?

> +
> +	ret = mnt_want_write_file(filp);
> +	if (ret)
> +		return ret;
> +
> +	inode_lock(inode);
> +
> +	writecount = atomic_read(&inode->i_writecount);
> +	if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
> +			(!(filp->f_mode & FMODE_WRITE) && writecount)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

I don't think the check for i_writecount == 1 accomplishes anything because it
just means there are no *other* writable file descriptors.  It doesn't mean that
some other thread isn't concurrently trying to write to this same file
descriptor.  So the lock needs to be enough.  Is it?

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

> f2fs_readonly() is redundant with mnt_want_write_file().
>
> Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> be called on a file owned by another user, as long as the caller has read
> access.
>
> Note: if you change this to require a writable file descriptor, then
> f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> be needed.

I agree that f2fs_readonly() is redundant.
But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
way to check whether the caller has a proper write permission or not.
I think just using mnt_want_write_file() is enough for this ioctl. Am
I missing something?

> What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
> CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?

Good point!

> I don't think the check for i_writecount == 1 accomplishes anything because it
> just means there are no *other* writable file descriptors.  It doesn't mean that
> some other thread isn't concurrently trying to write to this same file
> descriptor.  So the lock needs to be enough.  Is it?

This is to detect any possibility of other threads mmap-ing and
writing the file.
Using only inode lock is not enough to prevent them from making dirty pages.



2020년 10월 13일 (화) 오후 3:11, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Tue, Oct 13, 2020 at 11:24:29AM +0900, Daeho Jeong wrote:
> > +static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> > +{
> > +     struct inode *inode = file_inode(filp);
> > +     struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > +     struct f2fs_comp_option option;
> > +     int ret;
> > +     int writecount;
> > +
> > +     if (!f2fs_sb_has_compression(sbi))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
> > +             return -EINVAL;
> > +
> > +     if (f2fs_readonly(sbi->sb))
> > +             return -EROFS;
>

>
> > +
> > +     if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
> > +                             sizeof(option)))
> > +             return -EFAULT;
> > +
> > +     if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
> > +                     option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
> > +                     option.algorithm >= COMPRESS_MAX)
> > +             return -EINVAL;
>
> What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
> CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?
>
> > +
> > +     ret = mnt_want_write_file(filp);
> > +     if (ret)
> > +             return ret;
> > +
> > +     inode_lock(inode);
> > +
> > +     writecount = atomic_read(&inode->i_writecount);
> > +     if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
> > +                     (!(filp->f_mode & FMODE_WRITE) && writecount)) {
> > +             ret = -EBUSY;
> > +             goto out;
> > +     }
>
> I don't think the check for i_writecount == 1 accomplishes anything because it
> just means there are no *other* writable file descriptors.  It doesn't mean that
> some other thread isn't concurrently trying to write to this same file
> descriptor.  So the lock needs to be enough.  Is it?
>
> - Eric

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

> f2fs_readonly() is redundant with mnt_want_write_file().
>
> Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> be called on a file owned by another user, as long as the caller has read
> access.
>
> Note: if you change this to require a writable file descriptor, then
> f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> be needed.

I agree that f2fs_readonly() is redundant.
But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
way to check whether the caller has a proper write permission or not.
I think just using mnt_want_write_file() is enough for this ioctl. Am
I missing something?

> What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
> CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?

Good point!

> I don't think the check for i_writecount == 1 accomplishes anything because it
> just means there are no *other* writable file descriptors.  It doesn't mean that
> some other thread isn't concurrently trying to write to this same file
> descriptor.  So the lock needs to be enough.  Is it?

This is to detect any possibility of other threads mmap-ing and
writing the file.
Using only inode lock is not enough to prevent them from making dirty pages.



2020년 10월 13일 (화) 오후 3:11, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Tue, Oct 13, 2020 at 11:24:29AM +0900, Daeho Jeong wrote:
> > +static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> > +{
> > +     struct inode *inode = file_inode(filp);
> > +     struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > +     struct f2fs_comp_option option;
> > +     int ret;
> > +     int writecount;
> > +
> > +     if (!f2fs_sb_has_compression(sbi))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (!f2fs_compressed_file(inode) || IS_IMMUTABLE(inode))
> > +             return -EINVAL;
> > +
> > +     if (f2fs_readonly(sbi->sb))
> > +             return -EROFS;
>

>
> > +
> > +     if (copy_from_user(&option, (struct f2fs_comp_option __user *)arg,
> > +                             sizeof(option)))
> > +             return -EFAULT;
> > +
> > +     if (option.log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
> > +                     option.log_cluster_size > MAX_COMPRESS_LOG_SIZE ||
> > +                     option.algorithm >= COMPRESS_MAX)
> > +             return -EINVAL;
>
> What if f2fs_cops[options.algorithm] == NULL, e.g. COMPRESS_LZ4 without
> CONFIG_F2FS_FS_LZ4?  Shouldn't the caller get an error then?
>
> > +
> > +     ret = mnt_want_write_file(filp);
> > +     if (ret)
> > +             return ret;
> > +
> > +     inode_lock(inode);
> > +
> > +     writecount = atomic_read(&inode->i_writecount);
> > +     if ((filp->f_mode & FMODE_WRITE && writecount != 1) ||
> > +                     (!(filp->f_mode & FMODE_WRITE) && writecount)) {
> > +             ret = -EBUSY;
> > +             goto out;
> > +     }
>
> I don't think the check for i_writecount == 1 accomplishes anything because it
> just means there are no *other* writable file descriptors.  It doesn't mean that
> some other thread isn't concurrently trying to write to this same file
> descriptor.  So the lock needs to be enough.  Is it?
>
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Wed, Oct 14, 2020 at 11:27:30AM +0900, Daeho Jeong wrote:
> > f2fs_readonly() is redundant with mnt_want_write_file().
> >
> > Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> > be called on a file owned by another user, as long as the caller has read
> > access.
> >
> > Note: if you change this to require a writable file descriptor, then
> > f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> > be needed.
> 
> I agree that f2fs_readonly() is redundant.
> But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
> way to check whether the caller has a proper write permission or not.
> I think just using mnt_want_write_file() is enough for this ioctl. Am
> I missing something?

mnt_want_write_file() checks for write permission to the mount, not to the file.

I think this ioctl wants what f2fs_sec_trim_file() does:

	if (!(filp->f_mode & FMODE_WRITE))
		return -EBADF;

	file_start_write(filp);
	inode_lock(inode);
	...
	inode_unlock(inode);
	file_end_write(filp);


After all you shouldn't be able to change the compression options of a file
given only read access to it, right?

> > I don't think the check for i_writecount == 1 accomplishes anything because it
> > just means there are no *other* writable file descriptors.  It doesn't mean that
> > some other thread isn't concurrently trying to write to this same file
> > descriptor.  So the lock needs to be enough.  Is it?
> 
> This is to detect any possibility of other threads mmap-ing and
> writing the file.
> Using only inode lock is not enough to prevent them from making dirty pages.

Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
mmap'ing or writing to the file.  It just guarantees that there aren't any other
writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
be using the same file descriptor (or the same file description) concurrently.

- Eric

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Wed, Oct 14, 2020 at 11:27:30AM +0900, Daeho Jeong wrote:
> > f2fs_readonly() is redundant with mnt_want_write_file().
> >
> > Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> > be called on a file owned by another user, as long as the caller has read
> > access.
> >
> > Note: if you change this to require a writable file descriptor, then
> > f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> > be needed.
> 
> I agree that f2fs_readonly() is redundant.
> But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
> way to check whether the caller has a proper write permission or not.
> I think just using mnt_want_write_file() is enough for this ioctl. Am
> I missing something?

mnt_want_write_file() checks for write permission to the mount, not to the file.

I think this ioctl wants what f2fs_sec_trim_file() does:

	if (!(filp->f_mode & FMODE_WRITE))
		return -EBADF;

	file_start_write(filp);
	inode_lock(inode);
	...
	inode_unlock(inode);
	file_end_write(filp);


After all you shouldn't be able to change the compression options of a file
given only read access to it, right?

> > I don't think the check for i_writecount == 1 accomplishes anything because it
> > just means there are no *other* writable file descriptors.  It doesn't mean that
> > some other thread isn't concurrently trying to write to this same file
> > descriptor.  So the lock needs to be enough.  Is it?
> 
> This is to detect any possibility of other threads mmap-ing and
> writing the file.
> Using only inode lock is not enough to prevent them from making dirty pages.

Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
mmap'ing or writing to the file.  It just guarantees that there aren't any other
writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
be using the same file descriptor (or the same file description) concurrently.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
>         if (!(filp->f_mode & FMODE_WRITE))
>                 return -EBADF;
>
>         file_start_write(filp);
>         inode_lock(inode);
>         ...
>         inode_unlock(inode);
>         file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?

Yep, this looks more accurate.

> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't any other
> writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.

Yep, I agree this is not a proper way. I think we don't need this
check here, because
compress routine doesn't compress any file data when it detects the
file is mmaped
using f2fs_is_mmap_file().

Thanks~


2020년 10월 15일 (목) 오후 1:04, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Wed, Oct 14, 2020 at 11:27:30AM +0900, Daeho Jeong wrote:
> > > f2fs_readonly() is redundant with mnt_want_write_file().
> > >
> > > Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> > > be called on a file owned by another user, as long as the caller has read
> > > access.
> > >
> > > Note: if you change this to require a writable file descriptor, then
> > > f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> > > be needed.
> >
> > I agree that f2fs_readonly() is redundant.
> > But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
> > way to check whether the caller has a proper write permission or not.
> > I think just using mnt_want_write_file() is enough for this ioctl. Am
> > I missing something?
>
> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
>         if (!(filp->f_mode & FMODE_WRITE))
>                 return -EBADF;
>
>         file_start_write(filp);
>         inode_lock(inode);
>         ...
>         inode_unlock(inode);
>         file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?
>
> > > I don't think the check for i_writecount == 1 accomplishes anything because it
> > > just means there are no *other* writable file descriptors.  It doesn't mean that
> > > some other thread isn't concurrently trying to write to this same file
> > > descriptor.  So the lock needs to be enough.  Is it?
> >
> > This is to detect any possibility of other threads mmap-ing and
> > writing the file.
> > Using only inode lock is not enough to prevent them from making dirty pages.
>
> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't any other
> writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.
>
> - Eric

Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl

[Daeho Jeong]

> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
>         if (!(filp->f_mode & FMODE_WRITE))
>                 return -EBADF;
>
>         file_start_write(filp);
>         inode_lock(inode);
>         ...
>         inode_unlock(inode);
>         file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?

Yep, this looks more accurate.

> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't any other
> writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.

Yep, I agree this is not a proper way. I think we don't need this
check here, because
compress routine doesn't compress any file data when it detects the
file is mmaped
using f2fs_is_mmap_file().

Thanks~


2020년 10월 15일 (목) 오후 1:04, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Wed, Oct 14, 2020 at 11:27:30AM +0900, Daeho Jeong wrote:
> > > f2fs_readonly() is redundant with mnt_want_write_file().
> > >
> > > Also, shouldn't this require a writable file descriptor?  As-is, this ioctl can
> > > be called on a file owned by another user, as long as the caller has read
> > > access.
> > >
> > > Note: if you change this to require a writable file descriptor, then
> > > f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> > > be needed.
> >
> > I agree that f2fs_readonly() is redundant.
> > But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
> > way to check whether the caller has a proper write permission or not.
> > I think just using mnt_want_write_file() is enough for this ioctl. Am
> > I missing something?
>
> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
>         if (!(filp->f_mode & FMODE_WRITE))
>                 return -EBADF;
>
>         file_start_write(filp);
>         inode_lock(inode);
>         ...
>         inode_unlock(inode);
>         file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?
>
> > > I don't think the check for i_writecount == 1 accomplishes anything because it
> > > just means there are no *other* writable file descriptors.  It doesn't mean that
> > > some other thread isn't concurrently trying to write to this same file
> > > descriptor.  So the lock needs to be enough.  Is it?
> >
> > This is to detect any possibility of other threads mmap-ing and
> > writing the file.
> > Using only inode lock is not enough to prevent them from making dirty pages.
>
> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't any other
> writable file descriptors.  (Actually, file descriptions.)  Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.
>
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Randy Dunlap]

On 10/12/20 7:24 PM, Daeho Jeong wrote:
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 53fe2853579c..a33c90cf979b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -433,6 +433,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
>  					_IOR(F2FS_IOCTL_MAGIC, 19, __u64)
>  #define F2FS_IOC_SEC_TRIM_FILE		_IOW(F2FS_IOCTL_MAGIC, 20,	\
>  						struct f2fs_sectrim_range)
> +#define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
> +						struct f2fs_comp_option)

Hi,

F2FS_IOCTL_MAGIC should be listed in
Documentation/userspace-api/ioctl/ioctl-number.rst.

Please add it there.

thanks.
-- 
~Randy

Re: [f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Randy Dunlap]

On 10/12/20 7:24 PM, Daeho Jeong wrote:
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 53fe2853579c..a33c90cf979b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -433,6 +433,8 @@ static inline bool __has_cursum_space(struct f2fs_journal *journal,
>  					_IOR(F2FS_IOCTL_MAGIC, 19, __u64)
>  #define F2FS_IOC_SEC_TRIM_FILE		_IOW(F2FS_IOCTL_MAGIC, 20,	\
>  						struct f2fs_sectrim_range)
> +#define F2FS_IOC_GET_COMPRESS_OPTION	_IOR(F2FS_IOCTL_MAGIC, 21,	\
> +						struct f2fs_comp_option)

Hi,

F2FS_IOCTL_MAGIC should be listed in
Documentation/userspace-api/ioctl/ioctl-number.rst.

Please add it there.

thanks.
-- 
~Randy



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Tue, Oct 13, 2020 at 11:24:28AM +0900, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
> option of a file.
> 

For new ioctls please mention the documentation, tests, and use cases.

- Eric

Re: [f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Eric Biggers]

On Tue, Oct 13, 2020 at 11:24:28AM +0900, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
> option of a file.
> 

For new ioctls please mention the documentation, tests, and use cases.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Daeho Jeong]

Randy,
I'll talk with F2FS maintainers about this.

Eric,
Sure, I'll add it in the commit message.

2020년 10월 13일 (화) 오후 3:13, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Tue, Oct 13, 2020 at 11:24:28AM +0900, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
> > option of a file.
> >
>
> For new ioctls please mention the documentation, tests, and use cases.
>
> - Eric

Re: [f2fs-dev] [PATCH 1/2] f2fs: add F2FS_IOC_GET_COMPRESS_OPTION ioctl

[Daeho Jeong]

Randy,
I'll talk with F2FS maintainers about this.

Eric,
Sure, I'll add it in the commit message.

2020년 10월 13일 (화) 오후 3:13, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Tue, Oct 13, 2020 at 11:24:28AM +0900, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > Added a new F2FS_IOC_GET_COMPRESS_OPTION ioctl to get file compression
> > option of a file.
> >
>
> For new ioctls please mention the documentation, tests, and use cases.
>
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel