Re: [PATCH 0/4] cifs: alternate approach to fixing oplock queue races
Jeff Layton <[email protected]>
| Newsgroups | gmane.linux.file-systems.cifs |
|---|---|
| Message-ID | <[email protected]> |
On Tue, 8 Sep 2009 13:28:58 -0400 Jeff Layton <[email protected]> wrote: > On Tue, 8 Sep 2009 12:12:22 -0500 > Steve French <[email protected]> wrote: > > > On Tue, Sep 8, 2009 at 9:12 AM, Jeff Layton<[email protected]> wrote: > > > This patchset is an alternate approach to fixing the oplock queue > > > problems. Rather than tracking oplocks with separate structures, this > > > patchset adds some fields to cifsFileInfo. > > > > > > When an oplock break comes in, is_valid_oplock_break takes an extra > > > reference to the cifsFileInfo and queues the oplock break job to the > > > events workqueue. > > > > This looks excellent - the 4th patch has to be tried carefully but > > easier to read than alternatives. Any idea when the schedule_work > > would get dispatched (is the current task put to sleep immediately?) > > > > When it actually runs is sort of dependent on what other jobs are > running in queue, how many CPU's are in the box etc. In practice, I > don't think this set will materially change when the oplock break > actually happens, unless there is a lot of stuff sitting in the events > queue beforehand. > > It may also help performance if a lot of oplock breaks come in at once > since they won't be serialized. You could have one running for each CPU > you have. > > Another thing we could consider is using the new slow_work stuff that > dhowells added recently. An oplock break task could take a while to > run if there is a lot of data to be flushed, so that may be a better > scheme (though it does add a build-time kconfig dependency to CIFS). > > LWN has a good writeup on the slow_work infrastructure: > > http://lwn.net/Articles/329464/ > Here's a quick conversion to slow_work that I did this morning. This patch applies on top of the set I sent yesterday. Tested with connectathon and seems to work correctly (sniffing traffic showed the oplock break response go out onto the wire). I like this better than queueing to the events queue -- seems less likely to block more critical tasks. Thoughts? -- Jeff Layton <[email protected]> _______________________________________________ linux-cifs-client mailing list [email protected] https://lists.samba.org/mailman/listinfo/linux-cifs-client
0001-cifs-convert-oplock_break-job-to-slow_work.patch
(text/x-patch, 6.4 KB)
From 46ae620b364db83b3db79508364303d4b01780b3 Mon Sep 17 00:00:00 2001 From: Jeff Layton <[email protected]> Date: Wed, 9 Sep 2009 11:44:16 -0400 Subject: [PATCH 5/4] cifs: convert oplock_break job to slow_work Seems like a good candidate for this. Signed-off-by: Jeff Layton <[email protected]> --- fs/cifs/Kconfig | 1 + fs/cifs/cifsfs.c | 6 ++++++ fs/cifs/cifsglob.h | 4 +++- fs/cifs/cifsproto.h | 1 - fs/cifs/dir.c | 3 ++- fs/cifs/file.c | 27 ++++++++++++++++++++++++--- fs/cifs/misc.c | 13 +++++++++---- 7 files changed, 45 insertions(+), 10 deletions(-) diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig index 6994a0f..80f3525 100644 --- a/fs/cifs/Kconfig +++ b/fs/cifs/Kconfig @@ -2,6 +2,7 @@ config CIFS tristate "CIFS support (advanced network filesystem, SMBFS successor)" depends on INET select NLS + select SLOW_WORK help This is the client VFS module for the Common Internet File System (CIFS) protocol which is the successor to the Server Message Block diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index c4872ad..89142b3 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1038,8 +1038,14 @@ init_cifs(void) if (rc) goto out_unregister_key_type; #endif + rc = slow_work_register_user(); + if (rc) + goto out_unregister_resolver_key; + return 0; + out_unregister_resolver_key: + unregister_key_type(&key_type_dns_resolver); #ifdef CONFIG_CIFS_DFS_UPCALL out_unregister_key_type: #endif diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 2be0471..c3280fa 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -18,6 +18,7 @@ */ #include <linux/in.h> #include <linux/in6.h> +#include <linux/slow-work.h> #include "cifs_fs_sb.h" #include "cifsacl.h" /* @@ -355,7 +356,7 @@ struct cifsFileInfo { atomic_t count; /* reference count */ struct mutex fh_mutex; /* prevents reopen race after dead ses*/ struct cifs_search_info srch_inf; - struct work_struct oplock_break; /* workqueue job for oplock breaks */ + struct slow_work oplock_break; /* slow_work job for oplock breaks */ }; /* Take a reference on the file private data */ @@ -722,3 +723,4 @@ GLOBAL_EXTERN unsigned int cifs_min_rcv; /* min size of big ntwrk buf pool */ GLOBAL_EXTERN unsigned int cifs_min_small; /* min size of small buf pool */ GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/ +extern const struct slow_work_ops cifs_oplock_break_ops; diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index b063802..f2a6241 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -88,7 +88,6 @@ extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses, extern __u16 GetNextMid(struct TCP_Server_Info *server); extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); extern u64 cifs_UnixTimeToNT(struct timespec); -extern void cifs_oplock_break(struct work_struct *work); extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset); diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 5d70d7a..ae09e5c 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -31,6 +31,7 @@ #include "cifs_debug.h" #include "cifs_fs_sb.h" + static void renew_parental_timestamps(struct dentry *direntry) { @@ -155,7 +156,7 @@ cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle, mutex_init(&pCifsFile->lock_mutex); INIT_LIST_HEAD(&pCifsFile->llist); atomic_set(&pCifsFile->count, 1); - INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break); + slow_work_init(&pCifsFile->oplock_break, &cifs_oplock_break_ops); /* set the following in open now pCifsFile->pfile = file; */ diff --git a/fs/cifs/file.c b/fs/cifs/file.c index aae5870..04aab66 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -57,7 +57,7 @@ static inline struct cifsFileInfo *cifs_init_private( /* Initialize reference count to one. The private data is freed on the release of the last reference */ atomic_set(&private_data->count, 1); - INIT_WORK(&private_data->oplock_break, cifs_oplock_break); + slow_work_init(&private_data->oplock_break, &cifs_oplock_break_ops); return private_data; } @@ -2311,8 +2311,8 @@ out: return rc; } -void -cifs_oplock_break(struct work_struct *work) +static void +cifs_oplock_break(struct slow_work *work) { struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo, oplock_break); @@ -2350,10 +2350,31 @@ cifs_oplock_break(struct work_struct *work) LOCKING_ANDX_OPLOCK_RELEASE, false); cFYI(1, ("Oplock release rc = %d", rc)); } +} +static int +cifs_oplock_break_get(struct slow_work *work) +{ + struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo, + oplock_break); + cifsFileInfo_get(cfile); + return 0; +} + +static void +cifs_oplock_break_put(struct slow_work *work) +{ + struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo, + oplock_break); cifsFileInfo_put(cfile); } +const struct slow_work_ops cifs_oplock_break_ops = { + .get_ref = cifs_oplock_break_get, + .put_ref = cifs_oplock_break_put, + .execute = cifs_oplock_break, +}; + const struct address_space_operations cifs_addr_ops = { .readpage = cifs_readpage, .readpages = cifs_readpages, diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index 8e098e6..0241b25 100644 --- a/fs/cifs/misc.c +++ b/fs/cifs/misc.c @@ -499,6 +499,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) struct cifsTconInfo *tcon; struct cifsInodeInfo *pCifsInode; struct cifsFileInfo *netfile; + int rc; cFYI(1, ("Checking for oplock break or dnotify response")); if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) && @@ -578,16 +579,20 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) return true; } - cifsFileInfo_get(netfile); - read_unlock(&GlobalSMBSeslock); - read_unlock(&cifs_tcp_ses_lock); cFYI(1, ("file id match, oplock break")); pCifsInode = CIFS_I(netfile->pInode); pCifsInode->clientCanCacheAll = false; if (pSMB->OplockLevel == 0) pCifsInode->clientCanCacheRead = false; - if (schedule_work(&netfile->oplock_break)) + rc = slow_work_enqueue(&netfile->oplock_break); + if (rc) { + cERROR(1, ("failed to enqueue oplock " + "break: %d\n", rc)); + } else { netfile->oplock_break_cancelled = false; + } + read_unlock(&GlobalSMBSeslock); + read_unlock(&cifs_tcp_ses_lock); return true; } read_unlock(&GlobalSMBSeslock); -- 1.6.0.6