Re: [SMB CLIENT][PATCHES] directory lease debugging and configuration

[Bharath SM <bharathsm.hsk@gmail.com>]

Thanks for your comments. Apologies for getting back late on this.

On Tue, Sep 30, 2025 at 6:11 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Hi Steve, Bharath,
>
> Sending my review based on the commit messages and the patches applied
> manually on my local tree.
>
> Please try sending the patches with git-send-email next time, as it's
> much easier to apply and review.  Thanks!
Ack.

>
> On 09/29, Steve French via samba-technical wrote:
> >4 patches from Bharath to improve directory lease handling (see
> >attached).  Lightly updated and rebased on current mainline, and
> >merged into cifs-2.6.git for-next.  Feedback/review/comments welcome
> >
> >commit a50843f864205ea4576638cb32321313d9c06e54
> >Author: Bharath SM <bharathsm@microsoft.com>
> >Date:   Tue Sep 2 14:18:21 2025 +0530
> >
> >    smb: client: cap smb directory cache memory via module parameter
> >
> >    The CIFS directory entry cache could grow without a global
> >    bound across mounts. Add a module-wide cap to limit memory
> >    used by cached dirents and avoid unbounded growth.
> >
> >    Introduce a new module parameter, dir_cache_max_memory_kb
> >    (KB units; 0 = unlimited). When unset and directory caching
>
> "0 = unlimited" should be "0 = ~10% of RAM"

If the user wants behavior like today, then can explicitly write '0'
to make it unlimited.
Otherwise by default 10%of RAM.

>
> >    is enabled (dir_cache_timeout != 0), default the cap to ~10%
> >    of system RAM during module init. The parameter is exposed
> >    under: /sys/module/cifs/parameters/dir_cache_max_memory_kb.
> >
> >    Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >    Signed-off-by: Steve French <stfrench@microsoft.com>
>
> I think this should be a sysfs module parameter, as it assumes users
> knows how much memory they'll need beforehand:

Ack. We can keep it in procfs.

> - one can't say how many entries are in the shares, or how many shares
>    will be mounted
> - if they do, they need to calculate (nentries * (sizeof(struct
>    cached_dirents) + namelen (of each entry) + 1)), + round up(1024),
>    then finally divide by 1024, meaning they'll fallback to using the
>    default value
>
> On the default value, I think 10% of RAM is too much for cifs cached
> entries.

Today, we don't have a limit on memory for cached entries. I took 10%
percent RAM
as default value to start with and we can tune it later based on need.
Please let me know if you have suggestions about limits.


>
> The max memory value should be module-wide, yes, but equally divided for
> each tcon, because I might have an initial not-so-important share that
> ends up filling the whole cache, then a more important/accessed one that
> will not have the chance to cache entries.

Ack. We can have more granular control later. But since we have dir
cache timeout of 30 seconds
unless it's reused again it is not a problem today. IMO memory will be
utilised by the mount which is
performing heavy metadata operations and that would actually need the
most caching.