RE: fs: Add FITRIM ioctl

[Boaz Harrosh <bharrosh@panasas.com>]

Lukas Czerner <lczerner@redhat.com> wrote:
> Gitweb:     http://git.kernel.org/linus/367a51a339020ba4d9edb0ce0f21d65bd50b00c9
> Commit:     367a51a339020ba4d9edb0ce0f21d65bd50b00c9
> Parent:     77ca6cdf0ab8a42f481ec997911bc89e79138723
> Author:     Lukas Czerner <lczerner@redhat.com>
> AuthorDate: Wed Oct 27 21:30:11 2010 -0400
> Committer:  Theodore Ts'o <tytso@mit.edu>
> CommitDate: Wed Oct 27 21:30:11 2010 -0400
> 

I've just noticed this patch. Was it posted to linux-fsdevel? Sorry
for missing it.

So I have a comment, and question.
---
>     fs: Add FITRIM ioctl
>     
>     Adds an filesystem independent ioctl to allow implementation of file
>     system batched discard support. I takes fstrim_range structure as an
>     argument. fstrim_range is definec in the include/fs.h and its
>     definition is as follows.
>     
>     struct fstrim_range {
>     	start;
>     	len;
>     	minlen;
>     }
>     
>     start	- first Byte to trim
>     len	- number of Bytes to trim from start
>     minlen	- minimum extent length to trim, free extents shorter than this
>     	  number of Bytes will be ignored. This will be rounded up to fs
>     	  block size.
>     

Was the range done so it could be called in parts, and progress displayed?

>     It is also possible to specify NULL as an argument. In this case the
>     arguments will set itself as follows:
>     
>     start = 0;
>     len = ULLONG_MAX;
>     minlen = 0;
>     
>     So it will trim the whole file system at one run.
>     
>     After the FITRIM is done, the number of actually discarded Bytes is stored
>     in fstrim_range.len to give the user better insight on how much storage
>     space has been really released for wear-leveling.
>     
>     Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>     Reviewed-by: Dmitry Monakhov <dmonakhov@openvz.org>
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ioctl.c         |   39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |    8 ++++++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f855ea4..e92fdbb 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -530,6 +530,41 @@ static int ioctl_fsthaw(struct file *filp)
>  	return thaw_super(sb);
>  }
>  
> +static int ioctl_fstrim(struct file *filp, void __user *argp)
> +{
> +	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> +	struct fstrim_range range;
> +	int ret = 0;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	/* If filesystem doesn't support trim feature, return. */
> +	if (sb->s_op->trim_fs == NULL)
> +		return -EOPNOTSUPP;
> +
> +	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
> +	if (sb->s_bdev == NULL)
> +		return -EINVAL;
> +

What is the point of that? sb->s_bdev is not used anywhere in this code.
If an FS published an sb->s_op->trim_fs, it should know what it wants
No? Note that the FS in question does not even need to check. It already
knows if it's block based or not.

Thanks
Boaz

> +	if (argp == NULL) {
> +		range.start = 0;
> +		range.len = ULLONG_MAX;
> +		range.minlen = 0;
> +	} else if (copy_from_user(&range, argp, sizeof(range)))
> +		return -EFAULT;
> +
> +	ret = sb->s_op->trim_fs(sb, &range);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((argp != NULL) &&
> +	    (copy_to_user(argp, &range, sizeof(range))))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>  /*
>   * When you add any new common ioctls to the switches above and below
>   * please update compat_sys_ioctl() too.
> @@ -580,6 +615,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
>  		error = ioctl_fsthaw(filp);
>  		break;
>  
> +	case FITRIM:
> +		error = ioctl_fstrim(filp, argp);
> +		break;
> +
>  	case FS_IOC_FIEMAP:
>  		return ioctl_fiemap(filp, arg);
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 63d069b..7008268 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -32,6 +32,12 @@
>  #define SEEK_END	2	/* seek relative to end of file */
>  #define SEEK_MAX	SEEK_END
>  
> +struct fstrim_range {
> +	uint64_t start;
> +	uint64_t len;
> +	uint64_t minlen;
> +};
> +
>  /* And dynamically-tunable limits and defaults: */
>  struct files_stat_struct {
>  	int nr_files;		/* read only */
> @@ -316,6 +322,7 @@ struct inodes_stat_t {
>  #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
>  #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
>  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
> +#define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
>  
>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
> @@ -1581,6 +1588,7 @@ struct super_operations {
>  	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
>  #endif
>  	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
> +	int (*trim_fs) (struct super_block *, struct fstrim_range *);
>  };
>  
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>