We can only cache a limited amount of entries.
We need to force entries to be dropped from the cache at regular
intervals to make room for potential new entries/different
directories.
Access patterns change over time and what is the "hot" directory will
also change over time so we need to drop entries to make sure that
when some directory becomes hot there will be decent chance that it
will be able to become cached.
If a directory becomes "cold" we no longer want it to take up entries
in our cache.
regards
ronnie sahlberg
On Fri, 7 Jul 2023 at 15:37, Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> On Thu, Jul 6, 2023 at 8:51 AM Steve French <smfrench@xxxxxxxxx> wrote:
> >
> > updated to fix two checkpatch warnings (see attached) and tentatively
> > merged into for-next pending review/testing
> >
> >
> > On Wed, Jul 5, 2023 at 9:32 PM Ronnie Sahlberg <lsahlber@xxxxxxxxxx> wrote:
> > >
> > > and drop cached directories after 30 seconds
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > > ---
> > > fs/smb/client/cached_dir.c | 67 ++++++++++++++++++++++++++++++++++++++
> > > fs/smb/client/cached_dir.h | 1 +
> > > 2 files changed, 68 insertions(+)
> > >
> > > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> > > index bfc964b36c72..f567eea0a456 100644
> > > --- a/fs/smb/client/cached_dir.c
> > > +++ b/fs/smb/client/cached_dir.c
> > > @@ -568,6 +568,53 @@ static void free_cached_dir(struct cached_fid *cfid)
> > > kfree(cfid);
> > > }
> > >
> > > +static int
> > > +cifs_cfids_laundromat_thread(void *p)
> > > +{
> > > + struct cached_fids *cfids = p;
> > > + struct cached_fid *cfid, *q;
> > > + struct list_head entry;
> > > +
> > > + while (!kthread_should_stop()) {
> > > + ssleep(1);
> > > + INIT_LIST_HEAD(&entry);
> > > + if (kthread_should_stop())
> > > + return 0;
> > > + spin_lock(&cfids->cfid_list_lock);
> > > + list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> > > + if (jiffies > cfid->time + HZ * 30) {
> > > + list_del(&cfid->entry);
> > > + list_add(&cfid->entry, &entry);
> > > + cfids->num_entries--;
> > > + }
> > > + }
> > > + spin_unlock(&cfids->cfid_list_lock);
> > > +
> > > + list_for_each_entry_safe(cfid, q, &entry, entry) {
> > > + cfid->on_list = false;
> > > + list_del(&cfid->entry);
> > > + /*
> > > + * Cancel, and wait for the work to finish in
> > > + * case we are racing with it.
> > > + */
> > > + cancel_work_sync(&cfid->lease_break);
> > > + if (cfid->has_lease) {
> > > + /*
> > > + * We lease has not yet been cancelled from
> > > + * the server so we need to drop the reference.
> > > + */
> > > + spin_lock(&cfids->cfid_list_lock);
> > > + cfid->has_lease = false;
> > > + spin_unlock(&cfids->cfid_list_lock);
> > > + kref_put(&cfid->refcount, smb2_close_cached_fid);
> > > + }
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > struct cached_fids *init_cached_dirs(void)
> > > {
> > > struct cached_fids *cfids;
> > > @@ -577,6 +624,20 @@ struct cached_fids *init_cached_dirs(void)
> > > return NULL;
> > > spin_lock_init(&cfids->cfid_list_lock);
> > > INIT_LIST_HEAD(&cfids->entries);
> > > +
> > > + /*
> > > + * since we're in a cifs function already, we know that
> > > + * this will succeed. No need for try_module_get().
> > > + */
> > > + __module_get(THIS_MODULE);
> > > + cfids->laundromat = kthread_run(cifs_cfids_laundromat_thread,
> > > + cfids, "cifsd-cfid-laundromat");
> > > + if (IS_ERR(cfids->laundromat)) {
> > > + cifs_dbg(VFS, "Failed to start cfids laundromat thread.\n");
> > > + kfree(cfids);
> > > + module_put(THIS_MODULE);
> > > + return NULL;
> > > + }
> > > return cfids;
> > > }
> > >
> > > @@ -589,6 +650,12 @@ void free_cached_dirs(struct cached_fids *cfids)
> > > struct cached_fid *cfid, *q;
> > > LIST_HEAD(entry);
> > >
> > > + if (cfids->laundromat) {
> > > + kthread_stop(cfids->laundromat);
> > > + cfids->laundromat = NULL;
> > > + module_put(THIS_MODULE);
> > > + }
> > > +
> > > spin_lock(&cfids->cfid_list_lock);
> > > list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> > > cfid->on_list = false;
> > > diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> > > index 2f4e764c9ca9..facc9b154d00 100644
> > > --- a/fs/smb/client/cached_dir.h
> > > +++ b/fs/smb/client/cached_dir.h
> > > @@ -57,6 +57,7 @@ struct cached_fids {
> > > spinlock_t cfid_list_lock;
> > > int num_entries;
> > > struct list_head entries;
> > > + struct task_struct *laundromat;
> > > };
> > >
> > > extern struct cached_fids *init_cached_dirs(void);
> > > --
> > > 2.35.3
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
> Hi Ronnie,
>
> Why do we need this?
> We only cache dirents if we have the parent directory lease. As long
> as we have the lease, why can't we continue to cache these?
>
> --
> Regards,
> Shyam
