On Mon, 18 Oct 2010 23:29:37 +0530
Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
> cifs_tcp_ses_lock is a rwlock with protects the cifs_tcp_ses_list,
> server->smb_ses_list and the ses->tcon_list. It also protects a few
> ref counters in server, ses and tcon. In most cases the critical section
> doesn't seem to be large, in a few cases where it is slightly large, there
> seem to be really no benefit from concurrent access. I briefly considered RCU
> mechanism but it appears to me that there is no real need.
>
> Replace it with a spinlock and get rid of the last rwlock in the cifs code.
>
> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
> ---
> fs/cifs/cifs_debug.c | 12 ++++----
> fs/cifs/cifsfs.c | 8 +++---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/cifssmb.c | 6 ++--
> fs/cifs/connect.c | 70 +++++++++++++++++++++++++-------------------------
> fs/cifs/misc.c | 14 +++++-----
> fs/cifs/sess.c | 4 +-
> 7 files changed, 58 insertions(+), 58 deletions(-)
>
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index eb1ba49..103ab8b 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -148,7 +148,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
> seq_printf(m, "Servers:");
>
> i = 0;
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp1, &cifs_tcp_ses_list) {
> server = list_entry(tmp1, struct TCP_Server_Info,
> tcp_ses_list);
> @@ -230,7 +230,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
> spin_unlock(&GlobalMid_Lock);
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> seq_putc(m, '\n');
>
> /* BB add code to dump additional info such as TCP session info now */
> @@ -270,7 +270,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> atomic_set(&totBufAllocCount, 0);
> atomic_set(&totSmBufAllocCount, 0);
> #endif /* CONFIG_CIFS_STATS2 */
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp1, &cifs_tcp_ses_list) {
> server = list_entry(tmp1, struct TCP_Server_Info,
> tcp_ses_list);
> @@ -303,7 +303,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> }
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> }
>
> return count;
> @@ -343,7 +343,7 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
> GlobalCurrentXid, GlobalMaxActiveXid);
>
> i = 0;
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp1, &cifs_tcp_ses_list) {
> server = list_entry(tmp1, struct TCP_Server_Info,
> tcp_ses_list);
> @@ -397,7 +397,7 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
> }
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> seq_putc(m, '\n');
> return 0;
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index f1d9c71..cb77915 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -482,16 +482,16 @@ static void cifs_umount_begin(struct super_block *sb)
>
> tcon = cifs_sb_master_tcon(cifs_sb);
>
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if ((tcon->tc_count > 1) || (tcon->tidStatus == CifsExiting)) {
> /* we have other mounts to same share or we have
> already tried to force umount this and woken up
> all waiting network requests, nothing to do */
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return;
> } else if (tcon->tc_count == 1)
> tcon->tidStatus = CifsExiting;
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> /* cancel_brl_requests(tcon); */ /* BB mark all brl mids as exiting */
> /* cancel_notify_requests(tcon); */
> @@ -940,7 +940,7 @@ init_cifs(void)
> GlobalTotalActiveXid = 0;
> GlobalMaxActiveXid = 0;
> memset(Local_System_Name, 0, 15);
> - rwlock_init(&cifs_tcp_ses_lock);
> + spin_lock_init(&cifs_tcp_ses_lock);
> spin_lock_init(&cifs_file_list_lock);
> spin_lock_init(&GlobalMid_Lock);
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 18ee0ad..28337cb 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -703,7 +703,7 @@ GLOBAL_EXTERN struct list_head cifs_tcp_ses_list;
> * the reference counters for the server, smb session, and tcon. Finally,
> * changes to the tcon->tidStatus should be done while holding this lock.
> */
> -GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock;
> +GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock;
>
> /*
> * This lock protects the cifs_file->llist and cifs_file->flist
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index bfb59a6..e98f1f3 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -593,9 +593,9 @@ CIFSSMBNegotiate(unsigned int xid, struct cifsSesInfo *ses)
> rc = -EIO;
> goto neg_err_exit;
> }
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if (server->srv_count > 1) {
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> if (memcmp(server->server_GUID,
> pSMBr->u.extended_response.
> GUID, 16) != 0) {
> @@ -605,7 +605,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifsSesInfo *ses)
> 16);
> }
> } else {
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> memcpy(server->server_GUID,
> pSMBr->u.extended_response.GUID, 16);
> }
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 019f003..7e73176 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -150,7 +150,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>
> /* before reconnecting the tcp session, mark the smb session (uid)
> and the tid bad so they are not used until reconnected */
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp, &server->smb_ses_list) {
> ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> ses->need_reconnect = true;
> @@ -160,7 +160,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> tcon->need_reconnect = true;
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> /* do not want to be sending data on a socket we are freeing */
> mutex_lock(&server->srv_mutex);
> if (server->ssocket) {
> @@ -637,9 +637,9 @@ multi_t2_fnd:
> } /* end while !EXITING */
>
> /* take it off the list, if it's not already */
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_del_init(&server->tcp_ses_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> spin_lock(&GlobalMid_Lock);
> server->tcpStatus = CifsExiting;
> @@ -677,7 +677,7 @@ multi_t2_fnd:
> * BB: we shouldn't have to do any of this. It shouldn't be
> * possible to exit from the thread with active SMB sessions
> */
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if (list_empty(&server->pending_mid_q)) {
> /* loop through server session structures attached to this and
> mark them dead */
> @@ -687,7 +687,7 @@ multi_t2_fnd:
> ses->status = CifsExiting;
> ses->server = NULL;
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> } else {
> /* although we can not zero the server struct pointer yet,
> since there are active requests which may depnd on them,
> @@ -710,7 +710,7 @@ multi_t2_fnd:
> }
> }
> spin_unlock(&GlobalMid_Lock);
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> /* 1/8th of sec is more than enough time for them to exit */
> msleep(125);
> }
> @@ -733,12 +733,12 @@ multi_t2_fnd:
> if a crazy root user tried to kill cifsd
> kernel thread explicitly this might happen) */
> /* BB: This shouldn't be necessary, see above */
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp, &server->smb_ses_list) {
> ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> ses->server = NULL;
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> kfree(server->hostname);
> task_to_wake = xchg(&server->tsk, NULL);
> @@ -1524,7 +1524,7 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
> {
> struct TCP_Server_Info *server;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
> if (!match_address(server, addr,
> (struct sockaddr *)&vol->srcaddr))
> @@ -1534,11 +1534,11 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
> continue;
>
> ++server->srv_count;
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> cFYI(1, "Existing tcp session with server found");
> return server;
> }
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return NULL;
> }
>
> @@ -1547,14 +1547,14 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
> {
> struct task_struct *task;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if (--server->srv_count > 0) {
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return;
> }
>
> list_del_init(&server->tcp_ses_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> spin_lock(&GlobalMid_Lock);
> server->tcpStatus = CifsExiting;
> @@ -1679,9 +1679,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> }
>
> /* thread spawned, put it on the list */
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_add(&tcp_ses->tcp_ses_list, &cifs_tcp_ses_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> cifs_fscache_get_client_cookie(tcp_ses);
>
> @@ -1703,7 +1703,7 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
> {
> struct cifsSesInfo *ses;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> switch (server->secType) {
> case Kerberos:
> @@ -1723,10 +1723,10 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
> continue;
> }
> ++ses->ses_count;
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return ses;
> }
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return NULL;
> }
>
> @@ -1737,14 +1737,14 @@ cifs_put_smb_ses(struct cifsSesInfo *ses)
> struct TCP_Server_Info *server = ses->server;
>
> cFYI(1, "%s: ses_count=%d\n", __func__, ses->ses_count);
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if (--ses->ses_count > 0) {
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return;
> }
>
> list_del_init(&ses->smb_ses_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> if (ses->status == CifsGood) {
> xid = GetXid();
> @@ -1841,9 +1841,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
> goto get_ses_fail;
>
> /* success, put it on the list */
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_add(&ses->smb_ses_list, &server->smb_ses_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> FreeXid(xid);
> return ses;
> @@ -1860,7 +1860,7 @@ cifs_find_tcon(struct cifsSesInfo *ses, const char *unc)
> struct list_head *tmp;
> struct cifsTconInfo *tcon;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp, &ses->tcon_list) {
> tcon = list_entry(tmp, struct cifsTconInfo, tcon_list);
> if (tcon->tidStatus == CifsExiting)
> @@ -1869,10 +1869,10 @@ cifs_find_tcon(struct cifsSesInfo *ses, const char *unc)
> continue;
>
> ++tcon->tc_count;
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return tcon;
> }
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return NULL;
> }
>
> @@ -1883,14 +1883,14 @@ cifs_put_tcon(struct cifsTconInfo *tcon)
> struct cifsSesInfo *ses = tcon->ses;
>
> cFYI(1, "%s: tc_count=%d\n", __func__, tcon->tc_count);
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if (--tcon->tc_count > 0) {
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return;
> }
>
> list_del_init(&tcon->tcon_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> xid = GetXid();
> CIFSSMBTDis(xid, tcon);
> @@ -1963,9 +1963,9 @@ cifs_get_tcon(struct cifsSesInfo *ses, struct smb_vol *volume_info)
> tcon->nocase = volume_info->nocase;
> tcon->local_lease = volume_info->local_lease;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_add(&tcon->tcon_list, &ses->tcon_list);
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> cifs_fscache_get_super_cookie(tcon);
>
> @@ -3225,9 +3225,9 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, uid_t fsuid)
> vol_info->secFlg = CIFSSEC_MUST_KRB5;
>
> /* get a reference for the same TCP session */
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> ++master_tcon->ses->server->srv_count;
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info);
> if (IS_ERR(ses)) {
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index de6073c..a7b492c 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -347,7 +347,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ ,
> if (current_fsuid() != treeCon->ses->linux_uid) {
> cFYI(1, "Multiuser mode and UID "
> "did not match tcon uid");
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(temp_item, &treeCon->ses->server->smb_ses_list) {
> ses = list_entry(temp_item, struct cifsSesInfo, smb_ses_list);
> if (ses->linux_uid == current_fsuid()) {
> @@ -361,7 +361,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ ,
> }
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> }
> }
> }
> @@ -551,7 +551,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> return false;
>
> /* look up tcon based on tid & uid */
> - read_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> list_for_each(tmp, &srv->smb_ses_list) {
> ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> list_for_each(tmp1, &ses->tcon_list) {
> @@ -573,7 +573,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> */
> if (netfile->closePend) {
> spin_unlock(&cifs_file_list_lock);
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return true;
> }
>
> @@ -595,16 +595,16 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
> netfile->oplock_break_cancelled = false;
>
> spin_unlock(&cifs_file_list_lock);
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> return true;
> }
> spin_unlock(&cifs_file_list_lock);
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> cFYI(1, "No matching file for oplock break");
> return true;
> }
> }
> - read_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> cFYI(1, "Can not process oplock break for non-existent connection");
> return true;
> }
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 2111bed..933ed26 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -80,7 +80,7 @@ static __le16 get_next_vcnum(struct cifsSesInfo *ses)
> if (max_vcs < 2)
> max_vcs = 0xFFFF;
>
> - write_lock(&cifs_tcp_ses_lock);
> + spin_lock(&cifs_tcp_ses_lock);
> if ((ses->need_reconnect) && is_first_ses_reconnect(ses))
> goto get_vc_num_exit; /* vcnum will be zero */
> for (i = ses->server->srv_count - 1; i < max_vcs; i++) {
> @@ -112,7 +112,7 @@ static __le16 get_next_vcnum(struct cifsSesInfo *ses)
> vcnum = i;
> ses->vcnum = vcnum;
> get_vc_num_exit:
> - write_unlock(&cifs_tcp_ses_lock);
> + spin_unlock(&cifs_tcp_ses_lock);
>
> return cpu_to_le16(vcnum);
> }
> --
> 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
>
Looks reasonable. These codepaths aren't terribly "hot", so this lock
is fairly low-contention anyway.
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
--
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
