Re: [PATCH v4 3/9] ovl: Create ovl_casefold() to support casefolded strncmp()

["André Almeida" <andrealmeid@igalia.com>]

Hi Amir,

Em 14/08/2025 09:53, Amir Goldstein escreveu:
> On Thu, Aug 14, 2025 at 12:37 AM André Almeida <andrealmeid@igalia.com> wrote:
>>
>> To add overlayfs support casefold filesystems, create a new function
>> ovl_casefold(), to be able to do case-insensitive strncmp().
>>
>> ovl_casefold() allocates a new buffer and stores the casefolded version
>> of the string on it. If the allocation or the casefold operation fails,
>> fallback to use the original string.
>>
>> The case-insentive name is then used in the rb-tree search/insertion
>> operation. If the name is found in the rb-tree, the name can be
>> discarded and the buffer is freed. If the name isn't found, it's then
>> stored at struct ovl_cache_entry to be used later.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---

[...]

>> +       }
>>
>>          INIT_LIST_HEAD(list);
>>   }
>> @@ -260,12 +311,28 @@ static bool ovl_fill_merge(struct dir_context *ctx, const char *name,
>>   {
>>          struct ovl_readdir_data *rdd =
>>                  container_of(ctx, struct ovl_readdir_data, ctx);
>> +       struct ovl_fs *ofs = OVL_FS(rdd->dentry->d_sb);
>> +       const char *aux = NULL;
> 
> It looks strange to me that you need aux
> and it looks strange to pair <aux, cf_len>
> neither here or there...
> 

The reason behind this `aux` var is because I need a `const char` 
pointer to point to the `name` argument, and `cf_name` can't be const 
because it goes through ovl_casefold(). I tried a couple approaches here 
to get rid of the compiler warning regarding const, and the only way I 
managed to was using a third variable like that.

>> +       char *cf_name = NULL;
>> +       int cf_len = 0;
>> +
>> +       if (ofs->casefold)
>> +               cf_len = ovl_casefold(rdd->map, name, namelen, &cf_name);
>> +
>> +       if (cf_len <= 0) {
>> +               aux = name;
> 
> why not:
> cf_name = name;
> 
>> +               cf_len = namelen;
>> +       } else {
>> +               aux = cf_name;
>> +       }
> 
> and no aux and no else needed at all?
> 
> If you don't like a var named cf_name to point at a non-casefolded
> name buffer, then use other var names which are consistent such as
> <c_name, c_len> (c for "canonical" or "compare" name).
> 
>>
>>          rdd->count++;
>>          if (!rdd->is_lowest)
>> -               return ovl_cache_entry_add_rb(rdd, name, namelen, ino, d_type);
>> +               return ovl_cache_entry_add_rb(rdd, name, namelen, aux, cf_len,
>> +                                             ino, d_type);
>>          else
>> -               return ovl_fill_lowest(rdd, name, namelen, offset, ino, d_type);
>> +               return ovl_fill_lowest(rdd, name, namelen, aux, cf_len,
>> +                                      offset, ino, d_type);
>>   }
>>
> 
> What do you think about moving all the consume/free buffer logic out to caller:
> 

That looks way cleaner to me, thanks! I will apply this approach for v5.

> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index b65cdfce31ce..e77530c63207 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -174,7 +174,8 @@ static struct ovl_cache_entry
> *ovl_cache_entry_new(struct ovl_readdir_data *rdd,
>          return p;
>   }
> 
> -static bool ovl_cache_entry_add_rb(struct ovl_readdir_data *rdd,
> +/* Return 0 for found, >0 for added, <0 for error */
> +static int ovl_cache_entry_add_rb(struct ovl_readdir_data *rdd,
>                                    const char *name, int len, u64 ino,
>                                    unsigned int d_type)
>   {
> @@ -183,22 +184,23 @@ static bool ovl_cache_entry_add_rb(struct
> ovl_readdir_data *rdd,
>          struct ovl_cache_entry *p;
> 
>          if (ovl_cache_entry_find_link(name, len, &newp, &parent))
> -               return true;
> +               return 0;
> 
>          p = ovl_cache_entry_new(rdd, name, len, ino, d_type);
>          if (p == NULL) {
>                  rdd->err = -ENOMEM;
> -               return false;
> +               return -ENOMEM;
>          }
> 
>          list_add_tail(&p->l_node, rdd->list);
>          rb_link_node(&p->node, parent, newp);
>          rb_insert_color(&p->node, rdd->root);
> 
> -       return true;
> +       return 1;
>   }
> 
> -static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
> +/* Return 0 for found, >0 for added, <0 for error */
> +static int ovl_fill_lowest(struct ovl_readdir_data *rdd,
>                             const char *name, int namelen,
>                             loff_t offset, u64 ino, unsigned int d_type)
>   {
> @@ -207,6 +209,7 @@ static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
>          p = ovl_cache_entry_find(rdd->root, name, namelen);
>          if (p) {
>                  list_move_tail(&p->l_node, &rdd->middle);
> +               return 0;
>          } else {
>                  p = ovl_cache_entry_new(rdd, name, namelen, ino, d_type);
>                  if (p == NULL)
> @@ -215,7 +218,7 @@ static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
>                          list_add_tail(&p->l_node, &rdd->middle);
>          }
> 
> -       return rdd->err == 0;
> +       return rdd->err ?: 1;
>   }
> 
> @@ -260,12 +263,31 @@ static bool ovl_fill_merge(struct dir_context
> *ctx, const char *name,
>   {
>          struct ovl_readdir_data *rdd =
>                  container_of(ctx, struct ovl_readdir_data, ctx);
> +       struct ovl_fs *ofs = OVL_FS(rdd->dentry->d_sb);
> +       char *c_name = NULL;
> +       int c_len = 0;
> +       int ret;
> +
> +       if (ofs->casefold)
> +               c_len = ovl_casefold(rdd->map, name, namelen, &c_name);
> +
> +       if (c_len <= 0) {
> +               c_name = name;
> +               c_len = namelen;
> +       }
> 
>          rdd->count++;
> -       if (!rdd->is_lowest)
> -               return ovl_cache_entry_add_rb(rdd, name, namelen, ino, d_type);
> -       else
> -               return ovl_fill_lowest(rdd, name, namelen, offset, ino, d_type);
> +       if (!rdd->is_lowest) {
> +               ret = ovl_cache_entry_add_rb(rdd, name, namelen, c_name, c_len,
> +                                            ino, d_type);
> +       } else {
> +               ret = ovl_fill_lowest(rdd, name, namelen, c_name, c_len, offset,
> +                                     ino, d_type);
> +       }
> +       // ret > 1 means c_name is consumed
> +       if (ret <= 0 && c_len > 0)
> +               kfree(c_name);
> +       return ret >= 0;
>   }
> 
> Thanks,
> Amir.