Re: [PATCH v2 1/4] exfat: use upcase_ptable and upcase_range_info to reduce memory footprint

[David Timber <dxdt@dev.snart.me>]

On 5/7/26 21:03, Yuezhang.Mo@sony.com wrote:
>> + * To maintain interoperability, these errors are not corrected.
>> + */
>> +static const struct exfat_upcase_range_info def_utbl_ri[] __initconst = {
>> +       /* ASCII */
>> +       {
>> +               /* (index = 0, len = 26) */
>> +               .start = 0x0061,
>> +               .end   = 0x007B,
>> +               .value = 0x0041,
>> +               .inc   = 0x0001,
>> +       },
> How about changing to this style? the table looks too long.
>
> /*     start   end     value      inc */
> /* ASCII */
> [0] = {0x0061, 0x007B, 0x0041, 0x0001}, /* 26 characters */
>
> And 'len' in the comments is not very meaningful, the comments indicate
> how many characters need to be converted more appropriately.
Agreed. Will change the procuder program.

>> @@ -896,6 +898,12 @@ static int __init init_exfat_fs(void)
>>  {
>>         int err;
>>
>> +       err = exfat_populate_upcase_ptable(&exfat_def_upcase_ptable);
>> +       if (err) {
>> +               WARN_ON(err == -EINVAL);
>> +               return err;
>> +       }
>> +
>>         err = exfat_cache_init();
>>         if (err)
>>                 return err;
>>
> As I said before, we should call exfat_free_upcase_ptable() if
> exfat_cache_init() return an error.
This is frustrating. Yes, this is skill issues on my end, but it makes
me wonder why we can't just have only two return points(success and bail).

Whilst trying to hunt down the kernel memory corruption error in f2fs,
I've witness the horrors in the fill_super() as well. Why complicate
things by having too many goto labels?

https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions

I think this generally comes from the coding guidelines that mixes up
goto and return statements for clean ups. Sure, it makes sense if this
is the function that gets called millions of times in like skb a
handling callback, where avoiding a couple of no-op calls actually
matters. In this case, I fail to see if doing something like this can
hurt anything as long as we're careful with the order of resources being
freed:

    static int __init init_exfat_fs(void)
    {
    	int err;

    	err = exfat_populate_upcase_ptable(&exfat_def_upcase_ptable);
    	if (err) {
    		WARN_ON(err == -EINVAL);
    		goto bail;
    	}

    	err = exfat_cache_init();
    	if (err)
    		goto bail;

    	exfat_inode_cachep = kmem_cache_create("exfat_inode_cache",
    			sizeof(struct exfat_inode_info),
    			0, SLAB_RECLAIM_ACCOUNT,
    			exfat_inode_init_once);
    	if (!exfat_inode_cachep) {
    		err = -ENOMEM;
    		goto bail;
    	}

    	err = register_filesystem(&exfat_fs_type);
    	if (err)
    		goto bail;

    	return 0;

    bail:
    	kmem_cache_destroy(exfat_inode_cachep);
    	exfat_cache_shutdown();
    	exfat_free_upcase_ptable(&exfat_def_upcase_ptable);

    	return err;
    }

>> @@ -691,53 +330,36 @@ static int exfat_load_upcase_table(struct super_block *sb,
>>                 brelse(bh);
>>         }
>>
>> -       if (index >= 0xFFFF && utbl_checksum == chksum)
>> +       if (index >= 0xFFFF && utbl_checksum == chksum) {
>> +               /*
>> +                * is_default being set does not necessarily mean the contents are exact same as the
>> +                * upcase table loaded from the volume may be missing some entries. The checksum
>> +                * matching should be enough to cover that case.
>> +                */
>> +               if (is_default && utbl_checksum == EXFAT_DEF_UTBL_CHKSUM) {
>> +                       exfat_free_upcase_ptable(upcase_table);
>> +                       kvfree(upcase_table);
>>
> If the default upcase table is used, 874 entries will be set.
>
> We can determine whether the default upcase table is being used, rather
> than checksum, by checking `is_default` and the number of entries.
>
Hmmm.. I can read between the lines. I get what you're saying.

I have to admit, I realise now that the idea of `is_default` was rather
stupid. Even with the algorithm you suggested, I've found that there are
some corner cases that would have been missed.

The look up op will definitely add a few milliseconds to the mounting
process. Let's not do that.

The thing is, we're probably giving too much love to the codepath that
will (probably) never be executed in production. No sane formatter will
ever use non-default table. We can even assume that volumes with
non-default upcase tables are probably with malicious intent. I have no
real-world data, though. If anyone can prove that there is indeed an
exfat formatting util that uses non-default upcase table, I'd like to be
enlightened, please.

Anyway, we're stuck with the specs and so we have to do our best to find
the common ground. How about removing `is_default` and just checking
`utbl_checksum == EXFAT_DEF_UTBL_CHKSUM` for non-default fallback? That
should be more than enough. I don't think other implementations actually
bother to use the upcase table in the first place.

Anyway, my patches will have to wait whilst Jeon is trying hard to get
iomap settled down in the mainline. In the mean time, I will continue to
contribute from userland as much as possible.

Regards,
Davo