On Fri, Jul 7, 2023 at 11:20 AM ronnie sahlberg
<ronniesahlberg@xxxxxxxxx> wrote:
>
> 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.
>
Makes sense.
However, the value of MAX_CACHED_FIDS to 16 seems very restrictive.
And as Steve suggested, 30s expiry seems very aggressive.
I think we can increase both.
Also, I was wondering how do we handle cleanup of cfids when VFS
decides to free the inode (maybe due to memory crunch?).
Regards,
Shyam
> 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
--
Regards,
Shyam
