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;
IronPort-SDR: T6JrDboLtGNpqj64WQytDsL8SBSN8Q9FDz+YZk9mhOqfMYfevhgxxH/vCgEFf3rtBmrJfdAKym
 LKCs3NA3/rGIjkpY+IabJEhsGrb5PO4SFLlPloCktLj4m2Puts/2FwOQsamaYW9JNP5nuGD71A
 hJBj+0lNYdL1vQyXfyLAPvixAYERyZlp4ETLsgB/pP04V8hg9RizrzQULO7n6EBlO8CAEqbJ+j
 ggZMKlXZRSH46+bIFKrlRueoAQ2zJJZkraPEy+x5veXHuxTKp9eaxDaeimBglfY5gwTbBuFAul
 C5g=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Hyeongseok Kim <hyeongseok@gmail.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>
Subject: Re: [PATCH 2/2] exfat: add support FITRIM ioctl
Thread-Topic: [PATCH 2/2] exfat: add support FITRIM ioctl
Thread-Index: AQHXA1MGBqI38WrzyEuQdLGPmIADTQ==
Date:   Mon, 15 Feb 2021 19:33:13 +0000
Message-ID: <BYAPR04MB4965FC5CBAF7DB2BA1DA4FB886889@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20210215042411.119392-1-hyeongseok@gmail.com>
 <20210215042411.119392-2-hyeongseok@gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c2d5061-517d-482e-e8dd-08d8d1e88917
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2021 19:33:13.4499
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MZsrNeflYFxYQbl5xM8XD5Lr5wkNsWb4N7EyevdFPCkEfVveaVXlLpe5n3gzvmZJdHqS5r5VkAFc0oDgzeN/zFuH1s6m4tj8B3x8NTyeW4s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5896
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/14/21 20:28, Hyeongseok Kim wrote:=0A=
> add FITRIM ioctl to support trimming mounted filesystem=0A=
>=0A=
> Signed-off-by: Hyeongseok Kim <hyeongseok@gmail.com>=0A=
> ---=0A=
>  fs/exfat/balloc.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++=
=0A=
>  fs/exfat/exfat_fs.h |  1 +=0A=
>  fs/exfat/file.c     | 33 +++++++++++++++++=0A=
>  3 files changed, 123 insertions(+)=0A=
>=0A=
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c=0A=
> index 761c79c3a4ba..edd0f6912e8e 100644=0A=
> --- a/fs/exfat/balloc.c=0A=
> +++ b/fs/exfat/balloc.c=0A=
> @@ -273,3 +273,92 @@ int exfat_count_used_clusters(struct super_block *sb=
, unsigned int *ret_count)=0A=
>  	*ret_count =3D count;=0A=
>  	return 0;=0A=
>  }=0A=
> +=0A=
> +int exfat_trim_fs(struct inode *inode, struct fstrim_range *range)=0A=
> +{=0A=
> +	struct super_block *sb =3D inode->i_sb;=0A=
Reverse tree style for function variable declaration would be nice which yo=
u=0A=
partially have it here.=0A=
> +	struct exfat_sb_info *sbi =3D EXFAT_SB(sb);=0A=
> +	u64 clu_start, clu_end, trim_minlen, trimmed_total =3D 0;=0A=
> +	unsigned int trim_begin, trim_end, count;=0A=
> +	unsigned int next_free_clu;=0A=
> +	int err =3D 0;=0A=
> +=0A=
> +	clu_start =3D max_t(u64, range->start >> sbi->cluster_size_bits,=0A=
> +				EXFAT_FIRST_CLUSTER);=0A=
> +	clu_end =3D clu_start + (range->len >> sbi->cluster_size_bits) - 1;=0A=
> +	trim_minlen =3D range->minlen >> sbi->cluster_size_bits;=0A=
> +=0A=
> +	if (clu_start >=3D sbi->num_clusters || range->len < sbi->cluster_size)=
=0A=
> +		return -EINVAL;=0A=
> +=0A=
> +	if (clu_end >=3D sbi->num_clusters)=0A=
> +		clu_end =3D sbi->num_clusters - 1;=0A=
> +=0A=
> +	mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);=0A=
> +=0A=
> +	trim_begin =3D trim_end =3D exfat_find_free_bitmap(sb, clu_start);=0A=
> +	if (trim_begin =3D=3D EXFAT_EOF_CLUSTER)=0A=
> +		goto unlock;=0A=
> +=0A=
> +	next_free_clu =3D exfat_find_free_bitmap(sb, trim_end + 1);=0A=
> +	if (next_free_clu =3D=3D EXFAT_EOF_CLUSTER)=0A=
> +		goto unlock;=0A=
> +=0A=
> +	do {=0A=
> +		if (next_free_clu =3D=3D trim_end + 1)=0A=
> +			/* extend trim range for continuous free cluster */=0A=
> +			trim_end++;=0A=
> +		else {=0A=
> +			/* trim current range if it's larger than trim_minlen */=0A=
> +			count =3D trim_end - trim_begin + 1;=0A=
> +			if (count >=3D trim_minlen) {=0A=
> +				err =3D sb_issue_discard(sb,=0A=
> +					exfat_cluster_to_sector(sbi, trim_begin),=0A=
> +					count * sbi->sect_per_clus, GFP_NOFS, 0);=0A=
You are specifying the last argument as 0 to sb_issue_disacrd() i.e.=0A=
flags =3D=3D 0 this will propagate to :-=0A=
=0A=
sb_issue_discard()=0A=
    blkdev_issue_discard()=0A=
        __blkdev__issue_discard()=0A=
=0A=
Now blkdev_issue_disacrd() returns -ENOTSUPP in 3 cases :-=0A=
=0A=
1. If flags arg is set to BLKDEV_DISCARD_SECURE and queue doesn't support=
=0A=
   secure erase. In this case you have not set BLKDEV_DISCARD_SECURE that.=
=0A=
   So it should not return -EOPNOTSUPP.=0A=
2. If queue doesn't support discard. In this case caller of this function=
=0A=
   already set that. So it should not return -EOPNOTSUPP.=0A=
3. If q->limits.discard_granularity is not but LLD which I think caller of=
=0A=
   this function already used that to calculate the range->minlen.=0A=
=0A=
If above is true then err will not have value of -EOPNOTSUPP ?=0A=
=0A=
> +				if (err && err !=3D -EOPNOTSUPP)=0A=
> +					goto unlock;=0A=
> +				if (!err)=0A=
> +					trimmed_total +=3D count;=0A=
> +			}=0A=
> +=0A=
> +			/* set next start point of the free hole */=0A=
> +			trim_begin =3D trim_end =3D next_free_clu;=0A=
> +		}=0A=
> +=0A=
> +		if (next_free_clu >=3D clu_end)=0A=
> +			break;=0A=
> +=0A=
> +		if (fatal_signal_pending(current)) {=0A=
> +			err =3D -ERESTARTSYS;=0A=
> +			goto unlock;=0A=
> +		}=0A=
> +=0A=
> +		if (need_resched()) {=0A=
> +			mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);=0A=
sb_issue_discard() ->blkdev_issue_discard() will call cond_resced().=0A=
1. The check for need_resched() will ever be true since=0A=
blkdev_issue_discard()=0A=
   is already calling cond_sched() ?=0A=
2. If so do you still need to drop the mutex before calling=0A=
   sb_issue_discard() ?=0A=
> +			cond_resched();=0A=
> +			mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);=0A=
> +		}=0A=
> +=0A=
> +		next_free_clu =3D exfat_find_free_bitmap(sb, next_free_clu + 1);=0A=
> +=0A=
> +	} while (next_free_clu !=3D EXFAT_EOF_CLUSTER &&=0A=
> +			next_free_clu > trim_end);=0A=
> +=0A=
> +	/* try to trim remainder */=0A=
> +	count =3D trim_end - trim_begin + 1;=0A=
> +	if (count >=3D trim_minlen) {=0A=
> +		err =3D sb_issue_discard(sb, exfat_cluster_to_sector(sbi, trim_begin),=
=0A=
> +			count * sbi->sect_per_clus, GFP_NOFS, 0);=0A=
> +		if (err && err !=3D -EOPNOTSUPP)=0A=
> +			goto unlock;=0A=
> +=0A=
> +		if (!err)=0A=
> +			trimmed_total +=3D count;=0A=
> +	}=0A=
> +=0A=
> +unlock:=0A=
> +	mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);=0A=
> +	range->len =3D trimmed_total << sbi->cluster_size_bits;=0A=
> +=0A=
> +	return err;=0A=
> +}=0A=
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h=0A=
> index a183021ae31d..e050aea0b639 100644=0A=
> --- a/fs/exfat/exfat_fs.h=0A=
> +++ b/fs/exfat/exfat_fs.h=0A=
> @@ -411,6 +411,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned in=
t clu);=0A=
>  void exfat_clear_bitmap(struct inode *inode, unsigned int clu, bool sync=
);=0A=
>  unsigned int exfat_find_free_bitmap(struct super_block *sb, unsigned int=
 clu);=0A=
>  int exfat_count_used_clusters(struct super_block *sb, unsigned int *ret_=
count);=0A=
> +int exfat_trim_fs(struct inode *inode, struct fstrim_range *range);=0A=
>  =0A=
>  /* file.c */=0A=
>  extern const struct file_operations exfat_file_operations;=0A=
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c=0A=
> index 679828e7be07..61a64a4d4e6a 100644=0A=
> --- a/fs/exfat/file.c=0A=
> +++ b/fs/exfat/file.c=0A=
> @@ -351,7 +351,40 @@ int exfat_setattr(struct dentry *dentry, struct iatt=
r *attr)=0A=
>  =0A=
>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)=
=0A=
>  {=0A=
> +	struct inode *inode =3D file_inode(filp);=0A=
> +	struct super_block *sb =3D inode->i_sb;=0A=
> +=0A=
>  	switch (cmd) {=0A=
> +	case FITRIM:=0A=
> +	{=0A=
> +		struct request_queue *q =3D bdev_get_queue(sb->s_bdev);=0A=
> +		struct fstrim_range range;=0A=
> +		int ret =3D 0;=0A=
> +=0A=
> +		if (!capable(CAP_SYS_ADMIN))=0A=
> +			return -EPERM;=0A=
> +=0A=
> +		if (!blk_queue_discard(q))=0A=
> +			return -EOPNOTSUPP;=0A=
> +=0A=
> +		if (copy_from_user(&range, (struct fstrim_range __user *)arg,=0A=
> +			sizeof(range)))=0A=
> +			return -EFAULT;=0A=
> +=0A=
> +		range.minlen =3D max_t(unsigned int, range.minlen,=0A=
> +					q->limits.discard_granularity);=0A=
> +=0A=
> +		ret =3D exfat_trim_fs(inode, &range);=0A=
> +		if (ret < 0)=0A=
> +			return ret;=0A=
> +=0A=
> +		if (copy_to_user((struct fstrim_range __user *)arg, &range,=0A=
> +			sizeof(range)))=0A=
> +			return -EFAULT;=0A=
> +=0A=
> +		return 0;=0A=
> +	}=0A=
> +=0A=
Is {} really needed for switch case ?=0A=
Also, code related to FITRIM needs to be moved to a helper otherwise it=0A=
will bloat=0A=
the ioctl function, unless that is the objective here.=0A=
>  	default:=0A=
>  		return -ENOTTY;=0A=
>  	}=0A=
=0A=