On 10/08/2010 11:01 PM, Jeff Layton wrote:
> Convert this lock to a regular spinlock
>
> A rwlock_t offers little value here. It's more expensive than a regular
> spinlock unless you have a fairly large section of code that runs under
> the read lock and can benefit from the concurrency.
>
> Additionally, we need to ensure that the refcounting for files isn't
> racy and to do that we need to lock areas that can increment it for
> write. That means that the areas that can actually use a read_lock are
> very few and relatively infrequently used.
>
> While we're at it, change the name to something easier to type, and fix
> a bug in find_writable_file. cifsFileInfo_put can sleep and shouldn't be
> called while holding the lock.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/cifsfs.c | 2 +-
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/cifssmb.c | 4 +-
> fs/cifs/file.c | 54 ++++++++++++++++++++++++++--------------------------
> fs/cifs/misc.c | 8 +++---
> fs/cifs/readdir.c | 6 ++--
> 6 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 3facf25..c25e928 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -942,8 +942,8 @@ init_cifs(void)
> GlobalTotalActiveXid = 0;
> GlobalMaxActiveXid = 0;
> memset(Local_System_Name, 0, 15);
> - rwlock_init(&GlobalSMBSeslock);
> rwlock_init(&cifs_tcp_ses_lock);
> + spin_lock_init(&cifs_file_list_lock);
> spin_lock_init(&GlobalMid_Lock);
>
> if (cifs_max_pending < 2) {
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 6dcd911..c29ef5f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -720,7 +720,7 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock;
> * If cifs_tcp_ses_lock and the lock below are both needed to be held, then
> * the cifs_tcp_ses_lock must be grabbed first and released last.
> */
> -GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
> +GLOBAL_EXTERN spinlock_t cifs_file_list_lock;
>
> /* Outstanding dir notify requests */
> GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 54bd83a..d0aed27 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -91,13 +91,13 @@ static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
> struct list_head *tmp1;
>
> /* list all files open on tree connection and mark them invalid */
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> list_for_each_safe(tmp, tmp1, &pTcon->openFileList) {
> open_file = list_entry(tmp, struct cifsFileInfo, tlist);
> open_file->invalidHandle = true;
> open_file->oplock_break_cancelled = true;
> }
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> /* BB Add call to invalidate_inodes(sb) for all superblocks mounted
> to this tcon */
> }
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 86a1597..b6b88fe 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -186,10 +186,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
> atomic_set(&pCifsFile->count, 1);
> INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
>
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
> list_add(&pCifsFile->flist, &pCifsInode->openFileList);
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
>
> if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
> pCifsInode->clientCanCacheAll = true;
> @@ -539,13 +539,13 @@ int cifs_close(struct inode *inode, struct file *file)
> pTcon = tlink_tcon(pSMBFile->tlink);
> if (pSMBFile) {
> struct cifsLockInfo *li, *tmp;
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> pSMBFile->closePend = true;
> if (pTcon) {
> /* no sense reconnecting to close a file that is
> already closed */
> if (!pTcon->need_reconnect) {
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> timeout = 2;
> while ((atomic_read(&pSMBFile->count) != 1)
> && (timeout <= 2048)) {
> @@ -565,9 +565,9 @@ int cifs_close(struct inode *inode, struct file *file)
> rc = CIFSSMBClose(xid, pTcon,
> pSMBFile->netfid);
> } else
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> } else
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
>
> /* Delete any outstanding lock records.
> We'll lose them when the file is closed anyway. */
> @@ -578,16 +578,16 @@ int cifs_close(struct inode *inode, struct file *file)
> }
> mutex_unlock(&pSMBFile->lock_mutex);
>
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> list_del(&pSMBFile->flist);
> list_del(&pSMBFile->tlist);
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> cifsFileInfo_put(file->private_data);
> file->private_data = NULL;
> } else
> rc = -EBADF;
>
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> if (list_empty(&(CIFS_I(inode)->openFileList))) {
> cFYI(1, "closing last open instance for inode %p", inode);
> /* if the file is not open we do not know if we can cache info
> @@ -595,7 +595,7 @@ int cifs_close(struct inode *inode, struct file *file)
> CIFS_I(inode)->clientCanCacheRead = false;
> CIFS_I(inode)->clientCanCacheAll = false;
> }
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> if ((rc == 0) && CIFS_I(inode)->write_behind_rc)
> rc = CIFS_I(inode)->write_behind_rc;
> FreeXid(xid);
> @@ -617,18 +617,18 @@ int cifs_closedir(struct inode *inode, struct file *file)
> struct cifsTconInfo *pTcon = tlink_tcon(pCFileStruct->tlink);
>
> cFYI(1, "Freeing private data in close dir");
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> if (!pCFileStruct->srch_inf.endOfSearch &&
> !pCFileStruct->invalidHandle) {
> pCFileStruct->invalidHandle = true;
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> rc = CIFSFindClose(xid, pTcon, pCFileStruct->netfid);
> cFYI(1, "Closing uncompleted readdir with rc %d",
> rc);
> /* not much we can do if it fails anyway, ignore rc */
> rc = 0;
> } else
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> ptmp = pCFileStruct->srch_inf.ntwrk_buf_start;
> if (ptmp) {
> cFYI(1, "closedir free smb buf in srch struct");
> @@ -1114,7 +1114,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
> if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
> fsuid_only = false;
>
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> /* we could simply get the first_list_entry since write-only entries
> are always at the end of the list but since the first entry might
> have a close pending, we go through the whole list */
> @@ -1128,7 +1128,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
> /* found a good file */
> /* lock it so it will not be closed on us */
> cifsFileInfo_get(open_file);
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return open_file;
> } /* else might as well continue, and look for
> another, or simply have the caller reopen it
> @@ -1136,7 +1136,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
> } else /* write only file */
> break; /* write only files are last so must be done */
> }
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return NULL;
> }
> #endif
> @@ -1163,7 +1163,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
> if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
> fsuid_only = false;
>
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> refind_writable:
> list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
> if (open_file->closePend)
> @@ -1177,11 +1177,11 @@ refind_writable:
>
> if (!open_file->invalidHandle) {
> /* found a good writable file */
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return open_file;
> }
>
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> /* Had to unlock since following call can block */
> rc = cifs_reopen_file(open_file, false);
> if (!rc) {
> @@ -1189,7 +1189,7 @@ refind_writable:
> return open_file;
> else { /* start over in case this was deleted */
> /* since the list could be modified */
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> cifsFileInfo_put(open_file);
> goto refind_writable;
> }
> @@ -1203,7 +1203,7 @@ refind_writable:
> to hold up writepages here (rather than
> in caller) with continuous retries */
> cFYI(1, "wp failed on reopen file");
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> /* can not use this handle, no write
> pending on this one after all */
> cifsFileInfo_put(open_file);
> @@ -1224,7 +1224,7 @@ refind_writable:
> any_available = true;
> goto refind_writable;
> }
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return NULL;
> }
>
> @@ -2132,16 +2132,16 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
> {
> struct cifsFileInfo *open_file;
>
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
> if (open_file->closePend)
> continue;
> if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return 1;
> }
> }
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> return 0;
> }
>
> @@ -2305,8 +2305,8 @@ void cifs_oplock_break(struct work_struct *work)
> * finished grabbing reference for us. Make sure it's done by
> * waiting for GlobalSMSSeslock.
> */
> - write_lock(&GlobalSMBSeslock);
> - write_unlock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> + spin_unlock(&cifs_file_list_lock);
>
> cifs_oplock_break_put(cfile);
> }
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 9bac3e7..de6073c 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -560,7 +560,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> continue;
>
> cifs_stats_inc(&tcon->num_oplock_brks);
> - read_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> list_for_each(tmp2, &tcon->openFileList) {
> netfile = list_entry(tmp2, struct cifsFileInfo,
> tlist);
> @@ -572,7 +572,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> * closed anyway.
> */
> if (netfile->closePend) {
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> read_unlock(&cifs_tcp_ses_lock);
> return true;
> }
> @@ -594,11 +594,11 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> cifs_oplock_break_get(netfile);
> netfile->oplock_break_cancelled = false;
>
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> read_unlock(&cifs_tcp_ses_lock);
> return true;
> }
> - read_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> read_unlock(&cifs_tcp_ses_lock);
> cFYI(1, "No matching file for oplock break");
> return true;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 078c625..dbc1c64 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -529,14 +529,14 @@ static int find_cifs_entry(const int xid, struct cifsTconInfo *pTcon,
> (index_to_find < first_entry_in_buffer)) {
> /* close and restart search */
> cFYI(1, "search backing up - close and restart search");
> - write_lock(&GlobalSMBSeslock);
> + spin_lock(&cifs_file_list_lock);
> if (!cifsFile->srch_inf.endOfSearch &&
> !cifsFile->invalidHandle) {
> cifsFile->invalidHandle = true;
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> CIFSFindClose(xid, pTcon, cifsFile->netfid);
> } else
> - write_unlock(&GlobalSMBSeslock);
> + spin_unlock(&cifs_file_list_lock);
> if (cifsFile->srch_inf.ntwrk_buf_start) {
> cFYI(1, "freeing SMB ff cache buf on search rewind");
> if (cifsFile->srch_inf.smallBuf)
Looks fine to me.
Reviewed-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
