Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] exfat: add support FITRIM ioctl
To:     Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
        "namjae.jeon@samsung.com" <namjae.jeon@samsung.com>,
        "sj1557.seo@samsung.com" <sj1557.seo@samsung.com>
Cc:     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20210215042411.119392-1-hyeongseok@gmail.com>
 <20210215042411.119392-2-hyeongseok@gmail.com>
 <BYAPR04MB4965FC5CBAF7DB2BA1DA4FB886889@BYAPR04MB4965.namprd04.prod.outlook.com>
From:   Hyeongseok Kim <hyeongseok@gmail.com>
Message-ID: <08394066-3cde-4d0e-3c44-ea7ecf3a3844@gmail.com>
Date:   Tue, 16 Feb 2021 13:07:14 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <BYAPR04MB4965FC5CBAF7DB2BA1DA4FB886889@BYAPR04MB4965.namprd04.prod.outlook.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: ko-KR
Precedence: bulk

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().
>