[PATCH 1/2] exfat: add initial ioctl function

[PATCH 1/2] exfat: add initial ioctl function

[Hyeongseok Kim]

Initialize empty ioctl function

Signed-off-by: Hyeongseok Kim <hyeongseok@gmail.com>
---
 fs/exfat/dir.c      |  5 +++++
 fs/exfat/exfat_fs.h |  3 +++
 fs/exfat/file.c     | 21 +++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 916797077aad..e1d5536de948 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/compat.h>
 #include <linux/bio.h>
 #include <linux/buffer_head.h>
 
@@ -306,6 +307,10 @@ const struct file_operations exfat_dir_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.iterate	= exfat_iterate,
+	.unlocked_ioctl = exfat_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = exfat_compat_ioctl,
+#endif
 	.fsync		= exfat_file_fsync,
 };
 
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 764bc645241e..a183021ae31d 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -420,6 +420,9 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr);
 int exfat_getattr(const struct path *path, struct kstat *stat,
 		unsigned int request_mask, unsigned int query_flags);
 int exfat_file_fsync(struct file *file, loff_t start, loff_t end, int datasync);
+long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+long exfat_compat_ioctl(struct file *filp, unsigned int cmd,
+				unsigned long arg);
 
 /* namei.c */
 extern const struct dentry_operations exfat_dentry_ops;
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index a92478eabfa4..679828e7be07 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/compat.h>
 #include <linux/cred.h>
 #include <linux/buffer_head.h>
 #include <linux/blkdev.h>
@@ -348,6 +349,22 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr)
 	return error;
 }
 
+long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	default:
+		return -ENOTTY;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+long exfat_compat_ioctl(struct file *filp, unsigned int cmd,
+				unsigned long arg)
+{
+	return exfat_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
 int exfat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 {
 	struct inode *inode = filp->f_mapping->host;
@@ -368,6 +385,10 @@ const struct file_operations exfat_file_operations = {
 	.llseek		= generic_file_llseek,
 	.read_iter	= generic_file_read_iter,
 	.write_iter	= generic_file_write_iter,
+	.unlocked_ioctl = exfat_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = exfat_compat_ioctl,
+#endif
 	.mmap		= generic_file_mmap,
 	.fsync		= exfat_file_fsync,
 	.splice_read	= generic_file_splice_read,
-- 
2.27.0.83.g0313f36

[PATCH 2/2] exfat: add support FITRIM ioctl

[Hyeongseok Kim]

add FITRIM ioctl to support trimming mounted filesystem

Signed-off-by: Hyeongseok Kim <hyeongseok@gmail.com>
---
 fs/exfat/balloc.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++
 fs/exfat/exfat_fs.h |  1 +
 fs/exfat/file.c     | 33 +++++++++++++++++
 3 files changed, 123 insertions(+)

diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c
index 761c79c3a4ba..edd0f6912e8e 100644
--- a/fs/exfat/balloc.c
+++ b/fs/exfat/balloc.c
@@ -273,3 +273,92 @@ int exfat_count_used_clusters(struct super_block *sb, unsigned int *ret_count)
 	*ret_count = count;
 	return 0;
 }
+
+int exfat_trim_fs(struct inode *inode, struct fstrim_range *range)
+{
+	struct super_block *sb = inode->i_sb;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	u64 clu_start, clu_end, trim_minlen, trimmed_total = 0;
+	unsigned int trim_begin, trim_end, count;
+	unsigned int next_free_clu;
+	int err = 0;
+
+	clu_start = max_t(u64, range->start >> sbi->cluster_size_bits,
+				EXFAT_FIRST_CLUSTER);
+	clu_end = clu_start + (range->len >> sbi->cluster_size_bits) - 1;
+	trim_minlen = range->minlen >> sbi->cluster_size_bits;
+
+	if (clu_start >= sbi->num_clusters || range->len < sbi->cluster_size)
+		return -EINVAL;
+
+	if (clu_end >= sbi->num_clusters)
+		clu_end = sbi->num_clusters - 1;
+
+	mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
+
+	trim_begin = trim_end = exfat_find_free_bitmap(sb, clu_start);
+	if (trim_begin == EXFAT_EOF_CLUSTER)
+		goto unlock;
+
+	next_free_clu = exfat_find_free_bitmap(sb, trim_end + 1);
+	if (next_free_clu == EXFAT_EOF_CLUSTER)
+		goto unlock;
+
+	do {
+		if (next_free_clu == trim_end + 1)
+			/* extend trim range for continuous free cluster */
+			trim_end++;
+		else {
+			/* trim current range if it's larger than trim_minlen */
+			count = trim_end - trim_begin + 1;
+			if (count >= trim_minlen) {
+				err = sb_issue_discard(sb,
+					exfat_cluster_to_sector(sbi, trim_begin),
+					count * sbi->sect_per_clus, GFP_NOFS, 0);
+				if (err && err != -EOPNOTSUPP)
+					goto unlock;
+				if (!err)
+					trimmed_total += count;
+			}
+
+			/* set next start point of the free hole */
+			trim_begin = trim_end = next_free_clu;
+		}
+
+		if (next_free_clu >= clu_end)
+			break;
+
+		if (fatal_signal_pending(current)) {
+			err = -ERESTARTSYS;
+			goto unlock;
+		}
+
+		if (need_resched()) {
+			mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
+			cond_resched();
+			mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
+		}
+
+		next_free_clu = exfat_find_free_bitmap(sb, next_free_clu + 1);
+
+	} while (next_free_clu != EXFAT_EOF_CLUSTER &&
+			next_free_clu > trim_end);
+
+	/* try to trim remainder */
+	count = trim_end - trim_begin + 1;
+	if (count >= trim_minlen) {
+		err = sb_issue_discard(sb, exfat_cluster_to_sector(sbi, trim_begin),
+			count * sbi->sect_per_clus, GFP_NOFS, 0);
+		if (err && err != -EOPNOTSUPP)
+			goto unlock;
+
+		if (!err)
+			trimmed_total += count;
+	}
+
+unlock:
+	mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
+	range->len = trimmed_total << sbi->cluster_size_bits;
+
+	return err;
+}
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index a183021ae31d..e050aea0b639 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -411,6 +411,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int clu);
 void exfat_clear_bitmap(struct inode *inode, unsigned int clu, bool sync);
 unsigned int exfat_find_free_bitmap(struct super_block *sb, unsigned int clu);
 int exfat_count_used_clusters(struct super_block *sb, unsigned int *ret_count);
+int exfat_trim_fs(struct inode *inode, struct fstrim_range *range);
 
 /* file.c */
 extern const struct file_operations exfat_file_operations;
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 679828e7be07..61a64a4d4e6a 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -351,7 +351,40 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr)
 
 long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = file_inode(filp);
+	struct super_block *sb = inode->i_sb;
+
 	switch (cmd) {
+	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;
+
+		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
+			sizeof(range)))
+			return -EFAULT;
+
+		range.minlen = max_t(unsigned int, range.minlen,
+					q->limits.discard_granularity);
+
+		ret = exfat_trim_fs(inode, &range);
+		if (ret < 0)
+			return ret;
+
+		if (copy_to_user((struct fstrim_range __user *)arg, &range,
+			sizeof(range)))
+			return -EFAULT;
+
+		return 0;
+	}
+
 	default:
 		return -ENOTTY;
 	}
-- 
2.27.0.83.g0313f36

Re: [PATCH 2/2] exfat: add support FITRIM ioctl

[Chaitanya Kulkarni]

On 2/14/21 20:28, Hyeongseok Kim wrote:
> add FITRIM ioctl to support trimming mounted filesystem
>
> Signed-off-by: Hyeongseok Kim <hyeongseok@gmail.com>
> ---
>  fs/exfat/balloc.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/exfat/exfat_fs.h |  1 +
>  fs/exfat/file.c     | 33 +++++++++++++++++
>  3 files changed, 123 insertions(+)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c
> index 761c79c3a4ba..edd0f6912e8e 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -273,3 +273,92 @@ int exfat_count_used_clusters(struct super_block *sb, unsigned int *ret_count)
>  	*ret_count = count;
>  	return 0;
>  }
> +
> +int exfat_trim_fs(struct inode *inode, struct fstrim_range *range)
> +{
> +	struct super_block *sb = inode->i_sb;
Reverse tree style for function variable declaration would be nice which you
partially have it here.
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +	u64 clu_start, clu_end, trim_minlen, trimmed_total = 0;
> +	unsigned int trim_begin, trim_end, count;
> +	unsigned int next_free_clu;
> +	int err = 0;
> +
> +	clu_start = max_t(u64, range->start >> sbi->cluster_size_bits,
> +				EXFAT_FIRST_CLUSTER);
> +	clu_end = clu_start + (range->len >> sbi->cluster_size_bits) - 1;
> +	trim_minlen = range->minlen >> sbi->cluster_size_bits;
> +
> +	if (clu_start >= sbi->num_clusters || range->len < sbi->cluster_size)
> +		return -EINVAL;
> +
> +	if (clu_end >= sbi->num_clusters)
> +		clu_end = sbi->num_clusters - 1;
> +
> +	mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
> +
> +	trim_begin = trim_end = exfat_find_free_bitmap(sb, clu_start);
> +	if (trim_begin == EXFAT_EOF_CLUSTER)
> +		goto unlock;
> +
> +	next_free_clu = exfat_find_free_bitmap(sb, trim_end + 1);
> +	if (next_free_clu == EXFAT_EOF_CLUSTER)
> +		goto unlock;
> +
> +	do {
> +		if (next_free_clu == trim_end + 1)
> +			/* extend trim range for continuous free cluster */
> +			trim_end++;
> +		else {
> +			/* trim current range if it's larger than trim_minlen */
> +			count = trim_end - trim_begin + 1;
> +			if (count >= trim_minlen) {
> +				err = sb_issue_discard(sb,
> +					exfat_cluster_to_sector(sbi, trim_begin),
> +					count * sbi->sect_per_clus, GFP_NOFS, 0);
You are specifying the last argument as 0 to sb_issue_disacrd() i.e.
flags == 0 this will propagate to :-

sb_issue_discard()
    blkdev_issue_discard()
        __blkdev__issue_discard()

Now blkdev_issue_disacrd() returns -ENOTSUPP in 3 cases :-

1. If flags arg is set to BLKDEV_DISCARD_SECURE and queue doesn't support
   secure erase. In this case you have not set BLKDEV_DISCARD_SECURE that.
   So it should not return -EOPNOTSUPP.
2. If queue doesn't support discard. In this case caller of this function
   already set that. So it should not return -EOPNOTSUPP.
3. If q->limits.discard_granularity is not but LLD which I think caller of
   this function already used that to calculate the range->minlen.

If above is true then err will not have value of -EOPNOTSUPP ?

> +				if (err && err != -EOPNOTSUPP)
> +					goto unlock;
> +				if (!err)
> +					trimmed_total += count;
> +			}
> +
> +			/* set next start point of the free hole */
> +			trim_begin = trim_end = next_free_clu;
> +		}
> +
> +		if (next_free_clu >= clu_end)
> +			break;
> +
> +		if (fatal_signal_pending(current)) {
> +			err = -ERESTARTSYS;
> +			goto unlock;
> +		}
> +
> +		if (need_resched()) {
> +			mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
sb_issue_discard() ->blkdev_issue_discard() will call cond_resced().
1. The check for need_resched() will ever be true since
blkdev_issue_discard()
   is already calling cond_sched() ?
2. If so do you still need to drop the mutex before calling
   sb_issue_discard() ?
> +			cond_resched();
> +			mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
> +		}
> +
> +		next_free_clu = exfat_find_free_bitmap(sb, next_free_clu + 1);
> +
> +	} while (next_free_clu != EXFAT_EOF_CLUSTER &&
> +			next_free_clu > trim_end);
> +
> +	/* try to trim remainder */
> +	count = trim_end - trim_begin + 1;
> +	if (count >= trim_minlen) {
> +		err = sb_issue_discard(sb, exfat_cluster_to_sector(sbi, trim_begin),
> +			count * sbi->sect_per_clus, GFP_NOFS, 0);
> +		if (err && err != -EOPNOTSUPP)
> +			goto unlock;
> +
> +		if (!err)
> +			trimmed_total += count;
> +	}
> +
> +unlock:
> +	mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
> +	range->len = trimmed_total << sbi->cluster_size_bits;
> +
> +	return err;
> +}
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index a183021ae31d..e050aea0b639 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -411,6 +411,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int clu);
>  void exfat_clear_bitmap(struct inode *inode, unsigned int clu, bool sync);
>  unsigned int exfat_find_free_bitmap(struct super_block *sb, unsigned int clu);
>  int exfat_count_used_clusters(struct super_block *sb, unsigned int *ret_count);
> +int exfat_trim_fs(struct inode *inode, struct fstrim_range *range);
>  
>  /* file.c */
>  extern const struct file_operations exfat_file_operations;
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 679828e7be07..61a64a4d4e6a 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -351,7 +351,40 @@ int exfat_setattr(struct dentry *dentry, struct iattr *attr)
>  
>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	struct inode *inode = file_inode(filp);
> +	struct super_block *sb = inode->i_sb;
> +
>  	switch (cmd) {
> +	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;
> +
> +		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
> +			sizeof(range)))
> +			return -EFAULT;
> +
> +		range.minlen = max_t(unsigned int, range.minlen,
> +					q->limits.discard_granularity);
> +
> +		ret = exfat_trim_fs(inode, &range);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (copy_to_user((struct fstrim_range __user *)arg, &range,
> +			sizeof(range)))
> +			return -EFAULT;
> +
> +		return 0;
> +	}
> +
Is {} really needed for switch case ?
Also, code related to FITRIM needs to be moved to a helper otherwise it
will bloat
the ioctl function, unless that is the objective here.
>  	default:
>  		return -ENOTTY;
>  	}

Re: [PATCH 2/2] exfat: add support FITRIM ioctl

[Hyeongseok Kim]

Hi Chaitanya,
Thank you for the review.

On 2/16/21 4:33 AM, Chaitanya Kulkarni wrote:
> On 2/14/21 20:28, Hyeongseok Kim wrote:
>> +
>> +int exfat_trim_fs(struct inode *inode, struct fstrim_range *range)
>> +{
>> +	struct super_block *sb = inode->i_sb;
> Reverse tree style for function variable declaration would be nice which you
> partially have it here.
So, you mean that it would be better to be somethink like this, right?

+
+int exfat_trim_fs(struct inode *inode, struct fstrim_range *range)
+{
+	unsigned int trim_begin, trim_end, count, next_free_clu;
+	u64 clu_start, clu_end, trim_minlen, trimmed_total = 0;
+	struct super_block *sb = inode->i_sb;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	int err = 0;
+

>> +		else {
>> +			/* trim current range if it's larger than trim_minlen */
>> +			count = trim_end - trim_begin + 1;
>> +			if (count >= trim_minlen) {
>> +				err = sb_issue_discard(sb,
>> +					exfat_cluster_to_sector(sbi, trim_begin),
>> +					count * sbi->sect_per_clus, GFP_NOFS, 0);
> You are specifying the last argument as 0 to sb_issue_disacrd() i.e.
> flags == 0 this will propagate to :-
>
> sb_issue_discard()
>      blkdev_issue_discard()
>          __blkdev__issue_discard()
>
> Now blkdev_issue_disacrd() returns -ENOTSUPP in 3 cases :-
>
> 1. If flags arg is set to BLKDEV_DISCARD_SECURE and queue doesn't support
>     secure erase. In this case you have not set BLKDEV_DISCARD_SECURE that.
>     So it should not return -EOPNOTSUPP.
> 2. If queue doesn't support discard. In this case caller of this function
>     already set that. So it should not return -EOPNOTSUPP.
> 3. If q->limits.discard_granularity is not but LLD which I think caller of
>     this function already used that to calculate the range->minlen.
>
> If above is true then err will not have value of -EOPNOTSUPP ?
I think case 3. could be possible, but I agree we don't need to handle 
-EOPNOTSUPP in other way,
but better to just return it. I'll fix this in v2.
>> +		if (need_resched()) {
>> +			mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
> sb_issue_discard() ->blkdev_issue_discard() will call cond_resced().
> 1. The check for need_resched() will ever be true since
> blkdev_issue_discard()
>     is already calling cond_sched() ?
> 2. If so do you still need to drop the mutex before calling
>     sb_issue_discard() ?
I considered the case if there are no more used blocks or no more free 
blocks (no fragmentation)
to the end of the disk, then we couldn't have the chance to call 
sb_issue_discard() until this loop ends,
that would possibly take long time.
But it's not a good idea because other process can have chance to use 
blocks which were already
been ready to discard, if we release the mutex here. I'll remove this in 
v2.
>> +			cond_resched();
>> +			mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
>> +		}
>> +
..
>> +
>>   	switch (cmd) {
>> +	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;
>> +
>> +		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
>> +			sizeof(range)))
>> +			return -EFAULT;
>> +
>> +		range.minlen = max_t(unsigned int, range.minlen,
>> +					q->limits.discard_granularity);
>> +
>> +		ret = exfat_trim_fs(inode, &range);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (copy_to_user((struct fstrim_range __user *)arg, &range,
>> +			sizeof(range)))
>> +			return -EFAULT;
>> +
>> +		return 0;
>> +	}
>> +
> Is {} really needed for switch case ?
> Also, code related to FITRIM needs to be moved to a helper otherwise it
> will bloat
> the ioctl function, unless that is the objective here.
>>   	default:
>>   		return -ENOTTY;
>>   	}
OK, I'll move it into the exfat_trim_fs().
>