diff mbox series

[2/3] exfat: don't print error log in normal case

Message ID PUZPR04MB6316E45B7AB55F18F472503481A59@PUZPR04MB6316.apcprd04.prod.outlook.com (mailing list archive)
State New, archived
Headers show
Series exfat: fix and refine exfat_alloc_cluster() | expand

Commit Message

Yuezhang.Mo@sony.com Feb. 21, 2023, 7:34 a.m. UTC 
When allocating a new cluster, exFAT first allocates from the
next cluster of the last cluster of the file. If the last cluster
of the file is the last cluster of the volume, allocate from the
first cluster. This is a normal case, but the following error log
will be printed. It makes users confused, so this commit removes
the error log.

[1960905.181545] exFAT-fs (sdb1): hint_cluster is invalid (262130)

Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
Reviewed-by: Andy Wu <Andy.Wu@sony.com>
---
 fs/exfat/fatent.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Namjae Jeon Feb. 24, 2023, 2:04 p.m. UTC | #1 
2023-02-21 16:34 GMT+09:00, Yuezhang.Mo@sony.com <Yuezhang.Mo@sony.com>:
> When allocating a new cluster, exFAT first allocates from the
> next cluster of the last cluster of the file. If the last cluster
> of the file is the last cluster of the volume, allocate from the
> first cluster. This is a normal case, but the following error log
> will be printed. It makes users confused, so this commit removes
> the error log.
>
> [1960905.181545] exFAT-fs (sdb1): hint_cluster is invalid (262130)
>
> Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
> Reviewed-by: Andy Wu <Andy.Wu@sony.com>
> ---
>  fs/exfat/fatent.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
> index 65a8c9fb072c..b4ca533acaa9 100644
> --- a/fs/exfat/fatent.c
> +++ b/fs/exfat/fatent.c
> @@ -342,14 +342,18 @@ int exfat_alloc_cluster(struct inode *inode, unsigned
> int num_alloc,
>  		}
>  	}
>
> -	/* check cluster validation */
> -	if (!is_valid_cluster(sbi, hint_clu)) {
> -		exfat_err(sb, "hint_cluster is invalid (%u)",
> -			hint_clu);
> +	if (hint_clu == sbi->num_clusters) {
>  		hint_clu = EXFAT_FIRST_CLUSTER;
>  		p_chain->flags = ALLOC_FAT_CHAIN;
>  	}
>
> +	/* check cluster validation */
> +	if (!is_valid_cluster(sbi, hint_clu)) {
> +		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
> +		ret = -EIO;
There is no problem with allocation when invalid hint clu.
It is right to handle it as before instead returning -EIO.
In other words, after change exfat_err() to exfat_warn(),
It would be better to update print to avoid misunderstanding.
i.e. it will rewind to the first cluster rather than simply hint clu
invalid.

Thanks.
> +		goto unlock;
> +	}
> +
>  	p_chain->dir = EXFAT_EOF_CLUSTER;
>
>  	while ((new_clu = exfat_find_free_bitmap(sb, hint_clu)) !=
> --
> 2.25.1
>
>
Andy.Wu@sony.com Feb. 27, 2023, 2:20 a.m. UTC | #2 
Hi Namjae:

> > +	if (hint_clu == sbi->num_clusters) {
> >  		hint_clu = EXFAT_FIRST_CLUSTER;
> >  		p_chain->flags = ALLOC_FAT_CHAIN;
> >  	}
This is normal case, so let exfat rewind to the first cluster.

> > +	/* check cluster validation */
> > +	if (!is_valid_cluster(sbi, hint_clu)) {
> > +		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
> > +		ret = -EIO;
> There is no problem with allocation when invalid hint clu.
> It is right to handle it as before instead returning -EIO.
We think all other case are real error case, so, error print and return EIO.
May I confirm is there any normal case in here?

Best Regards
Andy Wu
Namjae Jeon Feb. 27, 2023, 2:42 a.m. UTC | #3 
2023-02-27 11:20 GMT+09:00, Andy.Wu@sony.com <Andy.Wu@sony.com>:
> Hi Namjae:
Hi Andy,
>
>> > +	if (hint_clu == sbi->num_clusters) {
>> >  		hint_clu = EXFAT_FIRST_CLUSTER;
>> >  		p_chain->flags = ALLOC_FAT_CHAIN;
>> >  	}
> This is normal case, so let exfat rewind to the first cluster.
>
>> > +	/* check cluster validation */
>> > +	if (!is_valid_cluster(sbi, hint_clu)) {
>> > +		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
>> > +		ret = -EIO;
>> There is no problem with allocation when invalid hint clu.
>> It is right to handle it as before instead returning -EIO.
> We think all other case are real error case, so, error print and return
> EIO.
Why ?

> May I confirm is there any normal case in here?
Could you please explain more ? I can't understand what you are saying.
>
> Best Regards
> Andy Wu
>
>
Yuezhang.Mo@sony.com Feb. 27, 2023, 4:07 a.m. UTC | #4 
Hi Namjae,

> >
> >> > +	if (hint_clu == sbi->num_clusters) {
> >> >  		hint_clu = EXFAT_FIRST_CLUSTER;
> >> >  		p_chain->flags = ALLOC_FAT_CHAIN;
> >> >  	}
> > This is normal case, so let exfat rewind to the first cluster.
> >
> >> > +	/* check cluster validation */
> >> > +	if (!is_valid_cluster(sbi, hint_clu)) {
> >> > +		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
> >> > +		ret = -EIO;
> >> There is no problem with allocation when invalid hint clu.
> >> It is right to handle it as before instead returning -EIO.
> > We think all other case are real error case, so, error print and
> > return EIO.
> Why ?
> 
> > May I confirm is there any normal case in here?
> Could you please explain more ? I can't understand what you are saying.
> >

`hint_clu` has the following cases. 

1. Create a new cluster chain: `hint_clu == EXFAT_EOF_CLUSTER`
2. Append a new cluster to a cluster chain: `hint_clu = last_clu + 1`
  2.1 ` hint_clu == sbi-> num_clusters`
  2.2 `EXFAT_FIRST_CLUTER <= hint_clu < sbi-> num_clusters`

This commit splits case 2 to 2.1 and 2.2, and handles case 2.1 before calling is_valid_cluster().
So is_valid_cluster() is always true, even removing the check of is_valid_cluster() is fine.

But considering that this check can find bugs in future code updates, we keep this check and return -EIO.
If not returned EIO and continue, bug may not be revealed.
Namjae Jeon Feb. 27, 2023, 7:32 a.m. UTC | #5 
2023-02-27 13:07 GMT+09:00, Yuezhang.Mo@sony.com <Yuezhang.Mo@sony.com>:
> Hi Namjae,
>
>> >
>> >> > +	if (hint_clu == sbi->num_clusters) {
>> >> >  		hint_clu = EXFAT_FIRST_CLUSTER;
>> >> >  		p_chain->flags = ALLOC_FAT_CHAIN;
>> >> >  	}
>> > This is normal case, so let exfat rewind to the first cluster.
>> >
>> >> > +	/* check cluster validation */
>> >> > +	if (!is_valid_cluster(sbi, hint_clu)) {
>> >> > +		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
>> >> > +		ret = -EIO;
>> >> There is no problem with allocation when invalid hint clu.
>> >> It is right to handle it as before instead returning -EIO.
>> > We think all other case are real error case, so, error print and
>> > return EIO.
>> Why ?
>>
>> > May I confirm is there any normal case in here?
>> Could you please explain more ? I can't understand what you are saying.
>> >
>
> `hint_clu` has the following cases.
>
> 1. Create a new cluster chain: `hint_clu == EXFAT_EOF_CLUSTER`
> 2. Append a new cluster to a cluster chain: `hint_clu = last_clu + 1`
>   2.1 ` hint_clu == sbi-> num_clusters`
>   2.2 `EXFAT_FIRST_CLUTER <= hint_clu < sbi-> num_clusters`
>
> This commit splits case 2 to 2.1 and 2.2, and handles case 2.1 before
> calling is_valid_cluster().
> So is_valid_cluster() is always true, even removing the check of
> is_valid_cluster() is fine.
>
> But considering that this check can find bugs in future code updates, we
> keep this check and return -EIO.
> If not returned EIO and continue, bug may not be revealed.
As I said on previous mail, We print warning message for this before
rewinding.  There is absolutely no reason to return an error...
>
diff mbox series

Patch

diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
index 65a8c9fb072c..b4ca533acaa9 100644
--- a/fs/exfat/fatent.c
+++ b/fs/exfat/fatent.c
@@ -342,14 +342,18 @@  int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc,
 		}
 	}
 
-	/* check cluster validation */
-	if (!is_valid_cluster(sbi, hint_clu)) {
-		exfat_err(sb, "hint_cluster is invalid (%u)",
-			hint_clu);
+	if (hint_clu == sbi->num_clusters) {
 		hint_clu = EXFAT_FIRST_CLUSTER;
 		p_chain->flags = ALLOC_FAT_CHAIN;
 	}
 
+	/* check cluster validation */
+	if (!is_valid_cluster(sbi, hint_clu)) {
+		exfat_err(sb, "hint_cluster is invalid (%u)", hint_clu);
+		ret = -EIO;
+		goto unlock;
+	}
+
 	p_chain->dir = EXFAT_EOF_CLUSTER;
 
 	while ((new_clu = exfat_find_free_bitmap(sb, hint_clu)) !=