Skip to content

Commit bf2069d

Browse files
committed
Merge tag '6.6-rc5-smb3-client-fixes' of git://git.samba.org/sfrench/cifs-2.6
Pull smb client fixes from Steve French: - fix caching race with open_cached_dir and laundromat cleanup of cached dirs (addresses a problem spotted with xfstest run with directory leases enabled) - reduce excessive resource usage of laundromat threads * tag '6.6-rc5-smb3-client-fixes' of git://git.samba.org/sfrench/cifs-2.6: smb: client: prevent new fids from being removed by laundromat smb: client: make laundromat a delayed worker
2 parents dc9b2e6 + 81ba109 commit bf2069d

File tree

2 files changed

+69
-74
lines changed

2 files changed

+69
-74
lines changed

fs/smb/client/cached_dir.c

Lines changed: 68 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
static struct cached_fid *init_cached_dir(const char *path);
1616
static void free_cached_dir(struct cached_fid *cfid);
1717
static void smb2_close_cached_fid(struct kref *ref);
18+
static void cfids_laundromat_worker(struct work_struct *work);
1819

1920
static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
2021
const char *path,
@@ -169,15 +170,18 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
169170
return -ENOENT;
170171
}
171172
/*
172-
* At this point we either have a lease already and we can just
173-
* return it. If not we are guaranteed to be the only thread accessing
174-
* this cfid.
173+
* Return cached fid if it has a lease. Otherwise, it is either a new
174+
* entry or laundromat worker removed it from @cfids->entries. Caller
175+
* will put last reference if the latter.
175176
*/
177+
spin_lock(&cfids->cfid_list_lock);
176178
if (cfid->has_lease) {
179+
spin_unlock(&cfids->cfid_list_lock);
177180
*ret_cfid = cfid;
178181
kfree(utf16_path);
179182
return 0;
180183
}
184+
spin_unlock(&cfids->cfid_list_lock);
181185

182186
/*
183187
* Skip any prefix paths in @path as lookup_positive_unlocked() ends up
@@ -294,9 +298,11 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
294298
goto oshr_free;
295299
}
296300
}
301+
spin_lock(&cfids->cfid_list_lock);
297302
cfid->dentry = dentry;
298303
cfid->time = jiffies;
299304
cfid->has_lease = true;
305+
spin_unlock(&cfids->cfid_list_lock);
300306

301307
oshr_free:
302308
kfree(utf16_path);
@@ -305,32 +311,36 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
305311
free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
306312
free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
307313
spin_lock(&cfids->cfid_list_lock);
308-
if (rc && !cfid->has_lease) {
309-
if (cfid->on_list) {
310-
list_del(&cfid->entry);
311-
cfid->on_list = false;
312-
cfids->num_entries--;
314+
if (!cfid->has_lease) {
315+
if (rc) {
316+
if (cfid->on_list) {
317+
list_del(&cfid->entry);
318+
cfid->on_list = false;
319+
cfids->num_entries--;
320+
}
321+
rc = -ENOENT;
322+
} else {
323+
/*
324+
* We are guaranteed to have two references at this
325+
* point. One for the caller and one for a potential
326+
* lease. Release the Lease-ref so that the directory
327+
* will be closed when the caller closes the cached
328+
* handle.
329+
*/
330+
spin_unlock(&cfids->cfid_list_lock);
331+
kref_put(&cfid->refcount, smb2_close_cached_fid);
332+
goto out;
313333
}
314-
rc = -ENOENT;
315334
}
316335
spin_unlock(&cfids->cfid_list_lock);
317-
if (!rc && !cfid->has_lease) {
318-
/*
319-
* We are guaranteed to have two references at this point.
320-
* One for the caller and one for a potential lease.
321-
* Release the Lease-ref so that the directory will be closed
322-
* when the caller closes the cached handle.
323-
*/
324-
kref_put(&cfid->refcount, smb2_close_cached_fid);
325-
}
326336
if (rc) {
327337
if (cfid->is_open)
328338
SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
329339
cfid->fid.volatile_fid);
330340
free_cached_dir(cfid);
331341
cfid = NULL;
332342
}
333-
343+
out:
334344
if (rc == 0) {
335345
*ret_cfid = cfid;
336346
atomic_inc(&tcon->num_remote_opens);
@@ -572,53 +582,51 @@ static void free_cached_dir(struct cached_fid *cfid)
572582
kfree(cfid);
573583
}
574584

575-
static int
576-
cifs_cfids_laundromat_thread(void *p)
585+
static void cfids_laundromat_worker(struct work_struct *work)
577586
{
578-
struct cached_fids *cfids = p;
587+
struct cached_fids *cfids;
579588
struct cached_fid *cfid, *q;
580-
struct list_head entry;
589+
LIST_HEAD(entry);
581590

582-
while (!kthread_should_stop()) {
583-
ssleep(1);
584-
INIT_LIST_HEAD(&entry);
585-
if (kthread_should_stop())
586-
return 0;
587-
spin_lock(&cfids->cfid_list_lock);
588-
list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
589-
if (time_after(jiffies, cfid->time + HZ * dir_cache_timeout)) {
590-
list_del(&cfid->entry);
591-
list_add(&cfid->entry, &entry);
592-
cfids->num_entries--;
593-
}
594-
}
595-
spin_unlock(&cfids->cfid_list_lock);
591+
cfids = container_of(work, struct cached_fids, laundromat_work.work);
596592

597-
list_for_each_entry_safe(cfid, q, &entry, entry) {
593+
spin_lock(&cfids->cfid_list_lock);
594+
list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
595+
if (cfid->time &&
596+
time_after(jiffies, cfid->time + HZ * dir_cache_timeout)) {
598597
cfid->on_list = false;
599-
list_del(&cfid->entry);
598+
list_move(&cfid->entry, &entry);
599+
cfids->num_entries--;
600+
/* To prevent race with smb2_cached_lease_break() */
601+
kref_get(&cfid->refcount);
602+
}
603+
}
604+
spin_unlock(&cfids->cfid_list_lock);
605+
606+
list_for_each_entry_safe(cfid, q, &entry, entry) {
607+
list_del(&cfid->entry);
608+
/*
609+
* Cancel and wait for the work to finish in case we are racing
610+
* with it.
611+
*/
612+
cancel_work_sync(&cfid->lease_break);
613+
if (cfid->has_lease) {
600614
/*
601-
* Cancel, and wait for the work to finish in
602-
* case we are racing with it.
615+
* Our lease has not yet been cancelled from the server
616+
* so we need to drop the reference.
603617
*/
604-
cancel_work_sync(&cfid->lease_break);
605-
if (cfid->has_lease) {
606-
/*
607-
* We lease has not yet been cancelled from
608-
* the server so we need to drop the reference.
609-
*/
610-
spin_lock(&cfids->cfid_list_lock);
611-
cfid->has_lease = false;
612-
spin_unlock(&cfids->cfid_list_lock);
613-
kref_put(&cfid->refcount, smb2_close_cached_fid);
614-
}
618+
spin_lock(&cfids->cfid_list_lock);
619+
cfid->has_lease = false;
620+
spin_unlock(&cfids->cfid_list_lock);
621+
kref_put(&cfid->refcount, smb2_close_cached_fid);
615622
}
623+
/* Drop the extra reference opened above */
624+
kref_put(&cfid->refcount, smb2_close_cached_fid);
616625
}
617-
618-
return 0;
626+
queue_delayed_work(cifsiod_wq, &cfids->laundromat_work,
627+
dir_cache_timeout * HZ);
619628
}
620629

621-
622630
struct cached_fids *init_cached_dirs(void)
623631
{
624632
struct cached_fids *cfids;
@@ -629,19 +637,10 @@ struct cached_fids *init_cached_dirs(void)
629637
spin_lock_init(&cfids->cfid_list_lock);
630638
INIT_LIST_HEAD(&cfids->entries);
631639

632-
/*
633-
* since we're in a cifs function already, we know that
634-
* this will succeed. No need for try_module_get().
635-
*/
636-
__module_get(THIS_MODULE);
637-
cfids->laundromat = kthread_run(cifs_cfids_laundromat_thread,
638-
cfids, "cifsd-cfid-laundromat");
639-
if (IS_ERR(cfids->laundromat)) {
640-
cifs_dbg(VFS, "Failed to start cfids laundromat thread.\n");
641-
kfree(cfids);
642-
module_put(THIS_MODULE);
643-
return NULL;
644-
}
640+
INIT_DELAYED_WORK(&cfids->laundromat_work, cfids_laundromat_worker);
641+
queue_delayed_work(cifsiod_wq, &cfids->laundromat_work,
642+
dir_cache_timeout * HZ);
643+
645644
return cfids;
646645
}
647646

@@ -657,11 +656,7 @@ void free_cached_dirs(struct cached_fids *cfids)
657656
if (cfids == NULL)
658657
return;
659658

660-
if (cfids->laundromat) {
661-
kthread_stop(cfids->laundromat);
662-
cfids->laundromat = NULL;
663-
module_put(THIS_MODULE);
664-
}
659+
cancel_delayed_work_sync(&cfids->laundromat_work);
665660

666661
spin_lock(&cfids->cfid_list_lock);
667662
list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {

fs/smb/client/cached_dir.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ struct cached_fids {
5757
spinlock_t cfid_list_lock;
5858
int num_entries;
5959
struct list_head entries;
60-
struct task_struct *laundromat;
60+
struct delayed_work laundromat_work;
6161
};
6262

6363
extern struct cached_fids *init_cached_dirs(void);

0 commit comments

Comments
 (0)