RFC: generic file operation for fstrim ioctl()

RFC: generic file operation for fstrim ioctl()

[Enrico Weigelt, metux IT consult]

Hello friends,


here's an RFC for a generic implementation of the FITRIM ioctl:

* introduce a new file_operation call vector for fstrim
* move the generic pieces (eg. cap check, copy from/to userland, etc)
  into a generic implementation that then calls into the fileop
* passing to the old ioctl fileop when fstrim op is NULL
* convert a few fs'es to the new schema

Rationale: it seems that all file systems implement generic stuff like
permission checks and buffer copy from/to userspace, all on their own.
We already have a common place for generic ioctl() handling (do_vfs_ioctl).
I feel its time for factoring this out the common fstrim pieces, too.

The first patch of this series introduces a new file_operation and calls
it on FITRIM (from do_vfs_ioctl) if it's set - otherwise just passes
the ioctl to file_ioctl(), as it had been before. So, this only becomes
active on a fs thats converted to the new file_operation. Subsequent patches
do the conversion for a few file systems.

Note this is just an RFC, don't apply it - there might still be bugs in there.


have fun,

--mtx

[RFC PATCH 1/6] fs: generic file operation for fstrim

[Enrico Weigelt, metux IT consult]

In order to factor out common parts of fstrim ioctl's, introduce a new
file_operation field for the file system specific parts.

The generic ioctl code (do_vfs_ioctl) calls an helper for doing the common
actions (eg. permission checks or buffer copying). If the file system
doesn't supply an fstrim function, the call will be passed down directly
to the file system, as it used to be.

This change also enables an easier way for possible future relaxing of
permissions, eg. allowing unprivileged users to trim their own files
sounds like an apelling idea.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 fs/ioctl.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  1 +
 2 files changed, 49 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1e2204fa9963..3a638f1eb0f5 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -961,6 +961,51 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
 	return err;
 }
 
+/*
+ * ioctl_fitrim() generic handling for FITRIM ioctl() via dedicated file
+ * operation. It does't common things like copying the arg from/to userland,
+ * permission check, etc.
+ *
+ * If the handler in file_operation is NULL, just pass the ioctl down to the
+ * generic ioctl() handler, as it used to be.
+ */
+static int ioctl_fitrim(struct file *filp, unsigned int fd, unsigned int cmd,
+			void __user *argp)
+{
+	struct fstrim_range;
+	int ret;
+
+
+		if (S_ISREG(inode->i_mode))
+			return file_ioctl(filp, cmd, argp);
+		break;
+
+	/* if the fs doesn't implement the fitrim operation, just pass it to
+	   the fs's ioctl() operation, so remaining implementations are kept
+	   intact. this can be removed when all fs'es are converted */
+	if (!filp->f_op->fitrim) {
+		if (S_ISREG(inode->i_mode))
+			return file_ioctl(filp, cmd, argp);
+
+		return -ENOIOCTLCMD;
+	}
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(&range, (struct fstrim_range __user)*argp,
+			   sizeof(range)))
+		return -EFAULT;
+
+	ret = filp->f_op->fitrim(filp, &range);
+
+	if (copy_to_user((struct fstrim_range __user)*argp, &range,
+			 sizeof(range)))
+		return -EFAULT;
+
+	return ret;
+}
+
 /*
  * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
  * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
@@ -1043,6 +1088,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
 	case FS_IOC_FSSETXATTR:
 		return ioctl_fssetxattr(filp, argp);
 
+	case FITRIM:
+		return ioctl_fitrim(filp, cmd, argp);
+
 	default:
 		if (S_ISREG(inode->i_mode))
 			return file_ioctl(filp, cmd, argp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c3c88fdb9b2a..9e2f0cd5c787 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2059,6 +2059,7 @@ struct file_operations {
 				   struct file *file_out, loff_t pos_out,
 				   loff_t len, unsigned int remap_flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
+	long (*fstrim)(struct file *, struct fstrim_range *range);
 } __randomize_layout;
 
 struct inode_operations {
-- 
2.20.1

[RFC PATCH 2/6] fs: ext4: move fstrim to file_operation

[Enrico Weigelt, metux IT consult]

---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/file.c  |  1 +
 fs/ext4/ioctl.c | 60 ++++++++++++++++++++-----------------------------
 3 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 37002663d521..2770f8f5da95 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1272,6 +1272,7 @@ struct ext4_inode_info {
 #define ext4_find_next_bit		find_next_bit_le
 
 extern void ext4_set_bits(void *bm, int cur, int len);
+extern long ext4_fitrim(struct file *filp, struct fstrim_range *range);
 
 /*
  * Maximal mount counts between two filesystem checks
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 816dedcbd541..07bf5a0d10f2 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -927,6 +927,7 @@ const struct file_operations ext4_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ext4_fallocate,
+	.fitrim		= ext4_fitrim,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 31627f7dc5cd..138e6128cb0c 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -800,6 +800,30 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	return error;
 }
 
+long ext4_fitrim(struct file *filp, struct fstrim_range *range)
+{
+	struct request_queue *q = bdev_get_queue(sb->s_bdev);
+	int ret = 0;
+
+	if (!blk_queue_discard(q))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We haven't replayed the journal, so we cannot use our
+	 * block-bitmap-guided storage zapping commands.
+	 */
+	if (test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb))
+		return -EROFS;
+
+	range->minlen = max((unsigned int)range->minlen,
+			   q->limits.discard_granularity);
+	ret = ext4_trim_fs(sb, range);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -1044,41 +1068,6 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return err;
 	}
 
-	case FITRIM:
-	{
-		struct request_queue *q = bdev_get_queue(sb->s_bdev);
-		struct fstrim_range range;
-		int ret = 0;
-
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-
-		if (!blk_queue_discard(q))
-			return -EOPNOTSUPP;
-
-		/*
-		 * We haven't replayed the journal, so we cannot use our
-		 * block-bitmap-guided storage zapping commands.
-		 */
-		if (test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb))
-			return -EROFS;
-
-		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
-		    sizeof(range)))
-			return -EFAULT;
-
-		range.minlen = max((unsigned int)range.minlen,
-				   q->limits.discard_granularity);
-		ret = ext4_trim_fs(sb, &range);
-		if (ret < 0)
-			return ret;
-
-		if (copy_to_user((struct fstrim_range __user *)arg, &range,
-		    sizeof(range)))
-			return -EFAULT;
-
-		return 0;
-	}
 	case EXT4_IOC_PRECACHE_EXTENTS:
 		return ext4_ext_precache(inode);
 
@@ -1272,7 +1261,6 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 	case EXT4_IOC_MOVE_EXT:
 	case EXT4_IOC_RESIZE_FS:
-	case FITRIM:
 	case EXT4_IOC_PRECACHE_EXTENTS:
 	case FS_IOC_SET_ENCRYPTION_POLICY:
 	case FS_IOC_GET_ENCRYPTION_PWSALT:
-- 
2.20.1

Re: [RFC PATCH 2/6] fs: ext4: move fstrim to file_operation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3602 bytes --]

Hi "Enrico,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kdave/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc7]
[cannot apply to ext4/dev next-20210622]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/2f26b6809e1541cf945da594ef5251e51ba73d3b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
        git checkout 2f26b6809e1541cf945da594ef5251e51ba73d3b
        # save the attached .config to linux build tree
        make W=1 ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/ext4/file.c:930:3: error: 'const struct file_operations' has no member named 'fitrim'; did you mean 'fstrim'?
     930 |  .fitrim  = ext4_fitrim,
         |   ^~~~~~
         |   fstrim
>> fs/ext4/file.c:930:13: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     930 |  .fitrim  = ext4_fitrim,
         |             ^~~~~~~~~~~
   fs/ext4/file.c:930:13: note: (near initialization for 'ext4_file_operations')
>> fs/ext4/file.c:930:13: error: initialization of 'void (*)(struct seq_file *, struct file *)' from incompatible pointer type 'long int (*)(struct file *, struct fstrim_range *)' [-Werror=incompatible-pointer-types]
   fs/ext4/file.c:930:13: note: (near initialization for 'ext4_file_operations.show_fdinfo')
   cc1: some warnings being treated as errors
--
   fs/ext4/ioctl.c: In function 'ext4_fitrim':
>> fs/ext4/ioctl.c:805:43: error: 'sb' undeclared (first use in this function); did you mean 's8'?
     805 |  struct request_queue *q = bdev_get_queue(sb->s_bdev);
         |                                           ^~
         |                                           s8
   fs/ext4/ioctl.c:805:43: note: each undeclared identifier is reported only once for each function it appears in


vim +930 fs/ext4/file.c

   911	
   912	const struct file_operations ext4_file_operations = {
   913		.llseek		= ext4_llseek,
   914		.read_iter	= ext4_file_read_iter,
   915		.write_iter	= ext4_file_write_iter,
   916		.iopoll		= iomap_dio_iopoll,
   917		.unlocked_ioctl = ext4_ioctl,
   918	#ifdef CONFIG_COMPAT
   919		.compat_ioctl	= ext4_compat_ioctl,
   920	#endif
   921		.mmap		= ext4_file_mmap,
   922		.mmap_supported_flags = MAP_SYNC,
   923		.open		= ext4_file_open,
   924		.release	= ext4_release_file,
   925		.fsync		= ext4_sync_file,
   926		.get_unmapped_area = thp_get_unmapped_area,
   927		.splice_read	= generic_file_splice_read,
   928		.splice_write	= iter_file_splice_write,
   929		.fallocate	= ext4_fallocate,
 > 930		.fitrim		= ext4_fitrim,
   931	};
   932	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 8645 bytes --]

[RFC PATCH 3/6] fs: hpfs: move fstrim to file_operation

[Enrico Weigelt, metux IT consult]

Use the newly introduced file_operation callback for FITRIM ioctl.
This removes some common code, eg. permission check, buffer copying,
which is now done by generic vfs code.
---
 fs/hpfs/dir.c     |  1 +
 fs/hpfs/file.c    |  1 +
 fs/hpfs/hpfs_fn.h |  1 +
 fs/hpfs/super.c   | 36 +++++++++++++++---------------------
 4 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/hpfs/dir.c b/fs/hpfs/dir.c
index f32f15669996..92084f4ce6a7 100644
--- a/fs/hpfs/dir.c
+++ b/fs/hpfs/dir.c
@@ -326,4 +326,5 @@ const struct file_operations hpfs_dir_ops =
 	.fsync		= hpfs_file_fsync,
 	.unlocked_ioctl	= hpfs_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
+	.fitrim		= hpfs_fitrim,
 };
diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
index 077c25128eb7..66112702cd4c 100644
--- a/fs/hpfs/file.c
+++ b/fs/hpfs/file.c
@@ -216,6 +216,7 @@ const struct file_operations hpfs_file_ops =
 	.splice_read	= generic_file_splice_read,
 	.unlocked_ioctl	= hpfs_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
+	.fitrim		= hpfs_fitrim,
 };
 
 const struct inode_operations hpfs_file_iops =
diff --git a/fs/hpfs/hpfs_fn.h b/fs/hpfs/hpfs_fn.h
index 167ec6884642..05c558084033 100644
--- a/fs/hpfs/hpfs_fn.h
+++ b/fs/hpfs/hpfs_fn.h
@@ -329,6 +329,7 @@ void hpfs_error(struct super_block *, const char *, ...);
 int hpfs_stop_cycles(struct super_block *, int, int *, int *, char *);
 unsigned hpfs_get_free_dnodes(struct super_block *);
 long hpfs_ioctl(struct file *file, unsigned cmd, unsigned long arg);
+long hpfs_fitrim(struct file *file, struct fstrim_range *range);
 
 /*
  * local time (HPFS) to GMT (Unix)
diff --git a/fs/hpfs/super.c b/fs/hpfs/super.c
index a7dbfc892022..8c45cb749ba4 100644
--- a/fs/hpfs/super.c
+++ b/fs/hpfs/super.c
@@ -200,30 +200,24 @@ static int hpfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
+long hpfs_fitrim(struct file *file, struct fstrim_range *range)
+{
+	secno n_trimmed;
+
+	int r = hpfs_trim_fs(file_inode(file)->i_sb, range->start >> 9,
+			     (range->start + range->len) >> 9,
+			     (range->minlen + 511) >> 9, &n_trimmed);
+
+	if (r)
+		return r;
+
+	range->len = (u64)n_trimmed << 9;
+	return 0;
+}
 
 long hpfs_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 {
-	switch (cmd) {
-		case FITRIM: {
-			struct fstrim_range range;
-			secno n_trimmed;
-			int r;
-			if (!capable(CAP_SYS_ADMIN))
-				return -EPERM;
-			if (copy_from_user(&range, (struct fstrim_range __user *)arg, sizeof(range)))
-				return -EFAULT;
-			r = hpfs_trim_fs(file_inode(file)->i_sb, range.start >> 9, (range.start + range.len) >> 9, (range.minlen + 511) >> 9, &n_trimmed);
-			if (r)
-				return r;
-			range.len = (u64)n_trimmed << 9;
-			if (copy_to_user((struct fstrim_range __user *)arg, &range, sizeof(range)))
-				return -EFAULT;
-			return 0;
-		}
-		default: {
-			return -ENOIOCTLCMD;
-		}
-	}
+	return -ENOIOCTLCMD;
 }
 
 
-- 
2.20.1

[RFC PATCH 4/6] fs: btrfs: move fstrim to file_operation

[Enrico Weigelt, metux IT consult]

Use the newly introduced file_operation callback for FITRIM ioctl.
This removes some common code, eg. permission check, buffer copying,
which is now done by generic vfs code.
---
 fs/btrfs/ctree.h |  1 +
 fs/btrfs/inode.c |  1 +
 fs/btrfs/ioctl.c | 26 +++++++-------------------
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 9fb76829a281..0361d95fe690 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3206,6 +3206,7 @@ void btrfs_update_inode_bytes(struct btrfs_inode *inode,
 
 /* ioctl.c */
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
+long btrfs_ioctl_fitrim(struct file *file, struct fstrim_range *range);
 long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int btrfs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
 int btrfs_fileattr_set(struct user_namespace *mnt_userns,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 46f392943f4d..5f0d1032c890 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10640,6 +10640,7 @@ static const struct file_operations btrfs_dir_file_operations = {
 #endif
 	.release        = btrfs_release_file,
 	.fsync		= btrfs_sync_file,
+	.fitrim		= btrfs_ioctl_fitrim,
 };
 
 /*
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5dc2fd843ae3..38b1de381836 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -372,19 +372,16 @@ static int btrfs_ioctl_getversion(struct file *file, int __user *arg)
 	return put_user(inode->i_generation, arg);
 }
 
-static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
-					void __user *arg)
+long btrfs_ioctl_fitrim(struct file *file, struct fstrim_range *range)
 {
+	struct inode *inode = file_inode(file);
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_device *device;
 	struct request_queue *q;
-	struct fstrim_range range;
 	u64 minlen = ULLONG_MAX;
 	u64 num_devices = 0;
 	int ret;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	/*
 	 * btrfs_trim_block_group() depends on space cache, which is not
 	 * available in zoned filesystem. So, disallow fitrim on a zoned
@@ -419,26 +416,19 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
 
 	if (!num_devices)
 		return -EOPNOTSUPP;
-	if (copy_from_user(&range, arg, sizeof(range)))
-		return -EFAULT;
 
 	/*
 	 * NOTE: Don't truncate the range using super->total_bytes.  Bytenr of
 	 * block group is in the logical address space, which can be any
 	 * sectorsize aligned bytenr in  the range [0, U64_MAX].
 	 */
-	if (range.len < fs_info->sb->s_blocksize)
+	if (range->len < fs_info->sb->s_blocksize)
 		return -EINVAL;
 
-	range.minlen = max(range.minlen, minlen);
-	ret = btrfs_trim_fs(fs_info, &range);
-	if (ret < 0)
-		return ret;
-
-	if (copy_to_user(arg, &range, sizeof(range)))
-		return -EFAULT;
+	range->minlen = max(range->minlen, minlen);
+	ret = btrfs_trim_fs(fs_info, range);
 
-	return 0;
+	return ret;
 }
 
 int __pure btrfs_is_empty_uuid(u8 *uuid)
@@ -4796,8 +4786,6 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_get_fslabel(fs_info, argp);
 	case FS_IOC_SETFSLABEL:
 		return btrfs_ioctl_set_fslabel(file, argp);
-	case FITRIM:
-		return btrfs_ioctl_fitrim(fs_info, argp);
 	case BTRFS_IOC_SNAP_CREATE:
 		return btrfs_ioctl_snap_create(file, argp, 0);
 	case BTRFS_IOC_SNAP_CREATE_V2:
-- 
2.20.1

Re: [RFC PATCH 4/6] fs: btrfs: move fstrim to file_operation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

Hi "Enrico,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kdave/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc7]
[cannot apply to ext4/dev next-20210622]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: m68k-randconfig-s031-20210622 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/1239d457c0b015846b187f9b31460e638301a395
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
        git checkout 1239d457c0b015846b187f9b31460e638301a395
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/btrfs/inode.c:10652:3: error: 'const struct file_operations' has no member named 'fitrim'; did you mean 'fstrim'?
   10652 |  .fitrim  = btrfs_ioctl_fitrim,
         |   ^~~~~~
         |   fstrim
>> fs/btrfs/inode.c:10652:13: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
   10652 |  .fitrim  = btrfs_ioctl_fitrim,
         |             ^~~~~~~~~~~~~~~~~~
   fs/btrfs/inode.c:10652:13: note: (near initialization for 'btrfs_dir_file_operations')
>> fs/btrfs/inode.c:10652:13: error: initialization of 'int (*)(int,  struct file *, int)' from incompatible pointer type 'long int (*)(struct file *, struct fstrim_range *)' [-Werror=incompatible-pointer-types]
   fs/btrfs/inode.c:10652:13: note: (near initialization for 'btrfs_dir_file_operations.fasync')
   cc1: some warnings being treated as errors


vim +10652 fs/btrfs/inode.c

 10640	
 10641	static const struct file_operations btrfs_dir_file_operations = {
 10642		.llseek		= generic_file_llseek,
 10643		.read		= generic_read_dir,
 10644		.iterate_shared	= btrfs_real_readdir,
 10645		.open		= btrfs_opendir,
 10646		.unlocked_ioctl	= btrfs_ioctl,
 10647	#ifdef CONFIG_COMPAT
 10648		.compat_ioctl	= btrfs_compat_ioctl,
 10649	#endif
 10650		.release        = btrfs_release_file,
 10651		.fsync		= btrfs_sync_file,
 10652		.fitrim		= btrfs_ioctl_fitrim,
 10653	};
 10654	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39928 bytes --]

[RFC PATCH 5/6] fs: fat: move fstrim to file_operation

[Enrico Weigelt, metux IT consult]

Use the newly introduced file_operation callback for FITRIM ioctl.
This removes some common code, eg. permission check, buffer copying,
which is now done by generic vfs code.
---
 fs/fat/file.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/fs/fat/file.c b/fs/fat/file.c
index 13855ba49cd9..5f57f06341d0 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -122,35 +122,20 @@ static int fat_ioctl_get_volume_id(struct inode *inode, u32 __user *user_attr)
 	return put_user(sbi->vol_id, user_attr);
 }
 
-static int fat_ioctl_fitrim(struct inode *inode, unsigned long arg)
+static int fat_ioctl_fitrim(struct file *file, struct fstrim_range *range)
 {
+	struct inode *inode = file_inode(file);
 	struct super_block *sb = inode->i_sb;
-	struct fstrim_range __user *user_range;
-	struct fstrim_range range;
 	struct request_queue *q = bdev_get_queue(sb->s_bdev);
 	int err;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	if (!blk_queue_discard(q))
 		return -EOPNOTSUPP;
 
-	user_range = (struct fstrim_range __user *)arg;
-	if (copy_from_user(&range, user_range, sizeof(range)))
-		return -EFAULT;
-
-	range.minlen = max_t(unsigned int, range.minlen,
+	range->minlen = max_t(unsigned int, range->minlen,
 			     q->limits.discard_granularity);
 
-	err = fat_trim_fs(inode, &range);
-	if (err < 0)
-		return err;
-
-	if (copy_to_user(user_range, &range, sizeof(range)))
-		return -EFAULT;
-
-	return 0;
+	return fat_trim_fs(inode, range);
 }
 
 long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
@@ -165,8 +150,6 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return fat_ioctl_set_attributes(filp, user_attr);
 	case FAT_IOCTL_GET_VOLUME_ID:
 		return fat_ioctl_get_volume_id(inode, user_attr);
-	case FITRIM:
-		return fat_ioctl_fitrim(inode, arg);
 	default:
 		return -ENOTTY;	/* Inappropriate ioctl for device */
 	}
-- 
2.20.1

[RFC PATCH 6/6] fs: f2fs: move fstrim to file_operation

[Enrico Weigelt, metux IT consult]

Use the newly introduced file_operation callback for FITRIM ioctl.
This removes some common code, eg. permission check, buffer copying,
which is now done by generic vfs code.
---
 fs/f2fs/file.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ceb575f99048..4c3f5eab22aa 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2252,38 +2252,27 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 	return ret;
 }
 
-static int f2fs_ioc_fitrim(struct file *filp, unsigned long arg)
+static long f2fs_ioc_fitrim(struct file *file, struct fstrim_range *range)
 {
 	struct inode *inode = file_inode(filp);
 	struct super_block *sb = inode->i_sb;
 	struct request_queue *q = bdev_get_queue(sb->s_bdev);
-	struct fstrim_range range;
 	int ret;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	if (!f2fs_hw_support_discard(F2FS_SB(sb)))
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&range, (struct fstrim_range __user *)arg,
-				sizeof(range)))
-		return -EFAULT;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
 
-	range.minlen = max((unsigned int)range.minlen,
+	range->minlen = max((unsigned int)range->minlen,
 				q->limits.discard_granularity);
-	ret = f2fs_trim_fs(F2FS_SB(sb), &range);
+	ret = f2fs_trim_fs(F2FS_SB(sb), range);
 	mnt_drop_write_file(filp);
 	if (ret < 0)
 		return ret;
 
-	if (copy_to_user((struct fstrim_range __user *)arg, &range,
-				sizeof(range)))
-		return -EFAULT;
 	f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
 	return 0;
 }
@@ -4124,8 +4113,6 @@ static long __f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_ioc_abort_volatile_write(filp);
 	case F2FS_IOC_SHUTDOWN:
 		return f2fs_ioc_shutdown(filp, arg);
-	case FITRIM:
-		return f2fs_ioc_fitrim(filp, arg);
 	case FS_IOC_SET_ENCRYPTION_POLICY:
 		return f2fs_ioc_set_encryption_policy(filp, arg);
 	case FS_IOC_GET_ENCRYPTION_POLICY:
@@ -4405,7 +4392,6 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RELEASE_VOLATILE_WRITE:
 	case F2FS_IOC_ABORT_VOLATILE_WRITE:
 	case F2FS_IOC_SHUTDOWN:
-	case FITRIM:
 	case FS_IOC_SET_ENCRYPTION_POLICY:
 	case FS_IOC_GET_ENCRYPTION_PWSALT:
 	case FS_IOC_GET_ENCRYPTION_POLICY:
@@ -4461,4 +4447,5 @@ const struct file_operations f2fs_file_operations = {
 #endif
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.fitrim		= f2fs_ioc_fitrim,
 };
-- 
2.20.1

Re: [RFC PATCH 6/6] fs: f2fs: move fstrim to file_operation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5824 bytes --]

Hi "Enrico,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kdave/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc7]
[cannot apply to ext4/dev next-20210622]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: m68k-randconfig-s031-20210622 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/a244db0d686be8dbc08aba54a817438247fe1405
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Enrico-Weigelt-metux-IT-consult/fs-generic-file-operation-for-fstrim/20210622-211217
        git checkout a244db0d686be8dbc08aba54a817438247fe1405
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   fs/f2fs/file.c: In function 'f2fs_ioc_fitrim':
>> fs/f2fs/file.c:2257:35: error: 'filp' undeclared (first use in this function); did you mean 'file'?
    2257 |  struct inode *inode = file_inode(filp);
         |                                   ^~~~
         |                                   file
   fs/f2fs/file.c:2257:35: note: each undeclared identifier is reported only once for each function it appears in
   fs/f2fs/file.c: At top level:
>> fs/f2fs/file.c:4450:3: error: 'const struct file_operations' has no member named 'fitrim'; did you mean 'fstrim'?
    4450 |  .fitrim  = f2fs_ioc_fitrim,
         |   ^~~~~~
         |   fstrim
>> fs/f2fs/file.c:4450:13: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
    4450 |  .fitrim  = f2fs_ioc_fitrim,
         |             ^~~~~~~~~~~~~~~
   fs/f2fs/file.c:4450:13: note: (near initialization for 'f2fs_file_operations')
>> fs/f2fs/file.c:4450:13: error: initialization of 'ssize_t (*)(struct file *, loff_t *, struct pipe_inode_info *, size_t,  unsigned int)' {aka 'int (*)(struct file *, long long int *, struct pipe_inode_info *, unsigned int,  unsigned int)'} from incompatible pointer type 'long int (*)(struct file *, struct fstrim_range *)' [-Werror=incompatible-pointer-types]
   fs/f2fs/file.c:4450:13: note: (near initialization for 'f2fs_file_operations.splice_read')
>> fs/f2fs/file.c:4450:13: warning: initialized field overwritten [-Woverride-init]
   fs/f2fs/file.c:4450:13: note: (near initialization for 'f2fs_file_operations.splice_read')
   cc1: some warnings being treated as errors


vim +2257 fs/f2fs/file.c

1abff93d01edda Jaegeuk Kim                      2015-01-08  2254  
a244db0d686be8 Enrico Weigelt, metux IT consult 2021-06-22  2255  static long f2fs_ioc_fitrim(struct file *file, struct fstrim_range *range)
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2256  {
52656e6cf7be69 Jaegeuk Kim                      2014-09-24 @2257  	struct inode *inode = file_inode(filp);
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2258  	struct super_block *sb = inode->i_sb;
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2259  	struct request_queue *q = bdev_get_queue(sb->s_bdev);
52656e6cf7be69 Jaegeuk Kim                      2014-09-24  2260  	int ret;
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2261  
7d20c8abb2edcf Chao Yu                          2018-09-04  2262  	if (!f2fs_hw_support_discard(F2FS_SB(sb)))
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2263  		return -EOPNOTSUPP;
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2264  
7fb17fe44b70c8 Chao Yu                          2016-05-09  2265  	ret = mnt_want_write_file(filp);
7fb17fe44b70c8 Chao Yu                          2016-05-09  2266  	if (ret)
7fb17fe44b70c8 Chao Yu                          2016-05-09  2267  		return ret;
7fb17fe44b70c8 Chao Yu                          2016-05-09  2268  
a244db0d686be8 Enrico Weigelt, metux IT consult 2021-06-22  2269  	range->minlen = max((unsigned int)range->minlen,
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2270  				q->limits.discard_granularity);
a244db0d686be8 Enrico Weigelt, metux IT consult 2021-06-22  2271  	ret = f2fs_trim_fs(F2FS_SB(sb), range);
7fb17fe44b70c8 Chao Yu                          2016-05-09  2272  	mnt_drop_write_file(filp);
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2273  	if (ret < 0)
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2274  		return ret;
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2275  
d0239e1bf5204d Jaegeuk Kim                      2016-01-08  2276  	f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2277  	return 0;
4b2fecc8465505 Jaegeuk Kim                      2014-09-20  2278  }
52656e6cf7be69 Jaegeuk Kim                      2014-09-24  2279  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39928 bytes --]