| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BYAPR04MB4965FC5CBAF7DB2BA1DA4FB886889@BYAPR04MB4965.namprd04.prod.outlook.com>
Date: Mon, 15 Feb 2021 19:33:13 +0000
From: Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To: Hyeongseok Kim <hyeongseok@...il.com>,
"namjae.jeon@...sung.com" <namjae.jeon@...sung.com>,
"sj1557.seo@...sung.com" <sj1557.seo@...sung.com>
CC: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] exfat: add support FITRIM ioctl
On 2/14/21 20:28, Hyeongseok Kim wrote:
> add FITRIM ioctl to support trimming mounted filesystem
>
> Signed-off-by: Hyeongseok Kim <hyeongseok@...il.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;
> }
Powered by blists - more mailing lists