| Message ID | 1250618822-6131-2-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
Starting to look through this set ACK on this one. Doesn't look like this would affect behavior, but slight cosmetic improvement On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton <jlayton@redhat.com> wrote: > Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That > lock is also used for completely unrelated purposes (mostly for managing > the global mid queue). Give the list its own dedicated spinlock > (cifs_oplock_lock) and rename the list to cifs_oplock_list to > eliminate the camel-case. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/cifsfs.c | 13 +++++++------ > fs/cifs/cifsglob.h | 6 +++++- > fs/cifs/transport.c | 17 ++++++++--------- > 3 files changed, 20 insertions(+), 16 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index b750aa5..3610e99 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg) > if (try_to_freeze()) > continue; > > - spin_lock(&GlobalMid_Lock); > - if (list_empty(&GlobalOplock_Q)) { > - spin_unlock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > + if (list_empty(&cifs_oplock_list)) { > + spin_unlock(&cifs_oplock_lock); > set_current_state(TASK_INTERRUPTIBLE); > schedule_timeout(39*HZ); > } else { > - oplock_item = list_entry(GlobalOplock_Q.next, > + oplock_item = list_entry(cifs_oplock_list.next, > struct oplock_q_entry, > qhead); > cFYI(1, ("found oplock item to write out")); > pTcon = oplock_item->tcon; > inode = oplock_item->pinode; > netfid = oplock_item->netfid; > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > DeleteOplockQEntry(oplock_item); > /* can not grab inode sem here since it would > deadlock when oplock received on delete > @@ -1055,7 +1055,7 @@ init_cifs(void) > int rc = 0; > cifs_proc_init(); > INIT_LIST_HEAD(&cifs_tcp_ses_list); > - INIT_LIST_HEAD(&GlobalOplock_Q); > + INIT_LIST_HEAD(&cifs_oplock_list); > #ifdef CONFIG_CIFS_EXPERIMENTAL > INIT_LIST_HEAD(&GlobalDnotifyReqList); > INIT_LIST_HEAD(&GlobalDnotifyRsp_Q); > @@ -1084,6 +1084,7 @@ init_cifs(void) > rwlock_init(&GlobalSMBSeslock); > rwlock_init(&cifs_tcp_ses_lock); > spin_lock_init(&GlobalMid_Lock); > + spin_lock_init(&cifs_oplock_lock); > > if (cifs_max_pending < 2) { > cifs_max_pending = 2; > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 6084d63..f100399 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock; > */ > GLOBAL_EXTERN rwlock_t GlobalSMBSeslock; > > -GLOBAL_EXTERN struct list_head GlobalOplock_Q; > +/* Global list of oplocks */ > +GLOBAL_EXTERN struct list_head cifs_oplock_list; > + > +/* Protects the cifs_oplock_list */ > +GLOBAL_EXTERN spinlock_t cifs_oplock_lock; > > /* Outstanding dir notify requests */ > GLOBAL_EXTERN struct list_head GlobalDnotifyReqList; > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 0ad3e2d..1da4ab2 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, > struct cifsTconInfo *tcon) > temp->pinode = pinode; > temp->tcon = tcon; > temp->netfid = fid; > - spin_lock(&GlobalMid_Lock); > - list_add_tail(&temp->qhead, &GlobalOplock_Q); > - spin_unlock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > + list_add_tail(&temp->qhead, &cifs_oplock_list); > + spin_unlock(&cifs_oplock_lock); > } > return temp; > - > } > > void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry) > { > - spin_lock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > /* should we check if list empty first? */ > list_del(&oplockEntry->qhead); > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > kmem_cache_free(cifs_oplock_cachep, oplockEntry); > } > > @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo > *tcon) > if (tcon == NULL) > return; > > - spin_lock(&GlobalMid_Lock); > - list_for_each_entry(temp, &GlobalOplock_Q, qhead) { > + spin_lock(&cifs_oplock_lock); > + list_for_each_entry(temp, &cifs_oplock_list, qhead) { > if ((temp->tcon) && (temp->tcon == tcon)) { > list_del(&temp->qhead); > kmem_cache_free(cifs_oplock_cachep, temp); > } > } > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > } > > static int > -- > 1.6.0.6 > >
On Thu, 20 Aug 2009 19:32:17 -0500 Steve French <smfrench@gmail.com> wrote: > Starting to look through this set > > ACK on this one. Doesn't look like this would affect behavior, but slight > cosmetic improvement > Well, it could affect behavior. It changes the lock from the GlobalMid_lock to a completely different one. It's always hard to tell exactly what the effect will be of breaking up a coarse-grained lock like that. The locks are taken in the same place however. If there truly is no relationship between the cifs_oplock_list and the other things protected by the mid lock, then yes there should be no behavior change. > > On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That > > lock is also used for completely unrelated purposes (mostly for managing > > the global mid queue). Give the list its own dedicated spinlock > > (cifs_oplock_lock) and rename the list to cifs_oplock_list to > > eliminate the camel-case. > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/cifsfs.c | 13 +++++++------ > > fs/cifs/cifsglob.h | 6 +++++- > > fs/cifs/transport.c | 17 ++++++++--------- > > 3 files changed, 20 insertions(+), 16 deletions(-) > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index b750aa5..3610e99 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg) > > if (try_to_freeze()) > > continue; > > > > - spin_lock(&GlobalMid_Lock); > > - if (list_empty(&GlobalOplock_Q)) { > > - spin_unlock(&GlobalMid_Lock); > > + spin_lock(&cifs_oplock_lock); > > + if (list_empty(&cifs_oplock_list)) { > > + spin_unlock(&cifs_oplock_lock); > > set_current_state(TASK_INTERRUPTIBLE); > > schedule_timeout(39*HZ); > > } else { > > - oplock_item = list_entry(GlobalOplock_Q.next, > > + oplock_item = list_entry(cifs_oplock_list.next, > > struct oplock_q_entry, > > qhead); > > cFYI(1, ("found oplock item to write out")); > > pTcon = oplock_item->tcon; > > inode = oplock_item->pinode; > > netfid = oplock_item->netfid; > > - spin_unlock(&GlobalMid_Lock); > > + spin_unlock(&cifs_oplock_lock); > > DeleteOplockQEntry(oplock_item); > > /* can not grab inode sem here since it would > > deadlock when oplock received on delete > > @@ -1055,7 +1055,7 @@ init_cifs(void) > > int rc = 0; > > cifs_proc_init(); > > INIT_LIST_HEAD(&cifs_tcp_ses_list); > > - INIT_LIST_HEAD(&GlobalOplock_Q); > > + INIT_LIST_HEAD(&cifs_oplock_list); > > #ifdef CONFIG_CIFS_EXPERIMENTAL > > INIT_LIST_HEAD(&GlobalDnotifyReqList); > > INIT_LIST_HEAD(&GlobalDnotifyRsp_Q); > > @@ -1084,6 +1084,7 @@ init_cifs(void) > > rwlock_init(&GlobalSMBSeslock); > > rwlock_init(&cifs_tcp_ses_lock); > > spin_lock_init(&GlobalMid_Lock); > > + spin_lock_init(&cifs_oplock_lock); > > > > if (cifs_max_pending < 2) { > > cifs_max_pending = 2; > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index 6084d63..f100399 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock; > > */ > > GLOBAL_EXTERN rwlock_t GlobalSMBSeslock; > > > > -GLOBAL_EXTERN struct list_head GlobalOplock_Q; > > +/* Global list of oplocks */ > > +GLOBAL_EXTERN struct list_head cifs_oplock_list; > > + > > +/* Protects the cifs_oplock_list */ > > +GLOBAL_EXTERN spinlock_t cifs_oplock_lock; > > > > /* Outstanding dir notify requests */ > > GLOBAL_EXTERN struct list_head GlobalDnotifyReqList; > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > index 0ad3e2d..1da4ab2 100644 > > --- a/fs/cifs/transport.c > > +++ b/fs/cifs/transport.c > > @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, > > struct cifsTconInfo *tcon) > > temp->pinode = pinode; > > temp->tcon = tcon; > > temp->netfid = fid; > > - spin_lock(&GlobalMid_Lock); > > - list_add_tail(&temp->qhead, &GlobalOplock_Q); > > - spin_unlock(&GlobalMid_Lock); > > + spin_lock(&cifs_oplock_lock); > > + list_add_tail(&temp->qhead, &cifs_oplock_list); > > + spin_unlock(&cifs_oplock_lock); > > } > > return temp; > > - > > } > > > > void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry) > > { > > - spin_lock(&GlobalMid_Lock); > > + spin_lock(&cifs_oplock_lock); > > /* should we check if list empty first? */ > > list_del(&oplockEntry->qhead); > > - spin_unlock(&GlobalMid_Lock); > > + spin_unlock(&cifs_oplock_lock); > > kmem_cache_free(cifs_oplock_cachep, oplockEntry); > > } > > > > @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo > > *tcon) > > if (tcon == NULL) > > return; > > > > - spin_lock(&GlobalMid_Lock); > > - list_for_each_entry(temp, &GlobalOplock_Q, qhead) { > > + spin_lock(&cifs_oplock_lock); > > + list_for_each_entry(temp, &cifs_oplock_list, qhead) { > > if ((temp->tcon) && (temp->tcon == tcon)) { > > list_del(&temp->qhead); > > kmem_cache_free(cifs_oplock_cachep, temp); > > } > > } > > - spin_unlock(&GlobalMid_Lock); > > + spin_unlock(&cifs_oplock_lock); > > } > > > > static int > > -- > > 1.6.0.6 > > > > > >
On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton@redhat.com> wrote: > Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That > lock is also used for completely unrelated purposes (mostly for managing > the global mid queue). Give the list its own dedicated spinlock > (cifs_oplock_lock) and rename the list to cifs_oplock_list to > eliminate the camel-case. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/cifsfs.c | 13 +++++++------ > fs/cifs/cifsglob.h | 6 +++++- > fs/cifs/transport.c | 17 ++++++++--------- > 3 files changed, 20 insertions(+), 16 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index b750aa5..3610e99 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg) > if (try_to_freeze()) > continue; > > - spin_lock(&GlobalMid_Lock); > - if (list_empty(&GlobalOplock_Q)) { > - spin_unlock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > + if (list_empty(&cifs_oplock_list)) { > + spin_unlock(&cifs_oplock_lock); > set_current_state(TASK_INTERRUPTIBLE); > schedule_timeout(39*HZ); > } else { > - oplock_item = list_entry(GlobalOplock_Q.next, > + oplock_item = list_entry(cifs_oplock_list.next, > struct oplock_q_entry, qhead); > cFYI(1, ("found oplock item to write out")); > pTcon = oplock_item->tcon; > inode = oplock_item->pinode; > netfid = oplock_item->netfid; > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > DeleteOplockQEntry(oplock_item); > /* can not grab inode sem here since it would > deadlock when oplock received on delete > @@ -1055,7 +1055,7 @@ init_cifs(void) > int rc = 0; > cifs_proc_init(); > INIT_LIST_HEAD(&cifs_tcp_ses_list); > - INIT_LIST_HEAD(&GlobalOplock_Q); > + INIT_LIST_HEAD(&cifs_oplock_list); > #ifdef CONFIG_CIFS_EXPERIMENTAL > INIT_LIST_HEAD(&GlobalDnotifyReqList); > INIT_LIST_HEAD(&GlobalDnotifyRsp_Q); > @@ -1084,6 +1084,7 @@ init_cifs(void) > rwlock_init(&GlobalSMBSeslock); > rwlock_init(&cifs_tcp_ses_lock); > spin_lock_init(&GlobalMid_Lock); > + spin_lock_init(&cifs_oplock_lock); > > if (cifs_max_pending < 2) { > cifs_max_pending = 2; > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 6084d63..f100399 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock; > */ > GLOBAL_EXTERN rwlock_t GlobalSMBSeslock; > > -GLOBAL_EXTERN struct list_head GlobalOplock_Q; > +/* Global list of oplocks */ > +GLOBAL_EXTERN struct list_head cifs_oplock_list; > + > +/* Protects the cifs_oplock_list */ > +GLOBAL_EXTERN spinlock_t cifs_oplock_lock; > > /* Outstanding dir notify requests */ > GLOBAL_EXTERN struct list_head GlobalDnotifyReqList; > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 0ad3e2d..1da4ab2 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon) > temp->pinode = pinode; > temp->tcon = tcon; > temp->netfid = fid; > - spin_lock(&GlobalMid_Lock); > - list_add_tail(&temp->qhead, &GlobalOplock_Q); > - spin_unlock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > + list_add_tail(&temp->qhead, &cifs_oplock_list); > + spin_unlock(&cifs_oplock_lock); > } > return temp; > - > } > > void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry) > { > - spin_lock(&GlobalMid_Lock); > + spin_lock(&cifs_oplock_lock); > /* should we check if list empty first? */ > list_del(&oplockEntry->qhead); > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > kmem_cache_free(cifs_oplock_cachep, oplockEntry); > } > > @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon) > if (tcon == NULL) > return; > > - spin_lock(&GlobalMid_Lock); > - list_for_each_entry(temp, &GlobalOplock_Q, qhead) { > + spin_lock(&cifs_oplock_lock); > + list_for_each_entry(temp, &cifs_oplock_list, qhead) { > if ((temp->tcon) && (temp->tcon == tcon)) { > list_del(&temp->qhead); > kmem_cache_free(cifs_oplock_cachep, temp); > } > } > - spin_unlock(&GlobalMid_Lock); > + spin_unlock(&cifs_oplock_lock); > } > > static int > -- > 1.6.0.6 > > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client > Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index b750aa5..3610e99 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg) if (try_to_freeze()) continue; - spin_lock(&GlobalMid_Lock); - if (list_empty(&GlobalOplock_Q)) { - spin_unlock(&GlobalMid_Lock); + spin_lock(&cifs_oplock_lock); + if (list_empty(&cifs_oplock_list)) { + spin_unlock(&cifs_oplock_lock); set_current_state(TASK_INTERRUPTIBLE); schedule_timeout(39*HZ); } else { - oplock_item = list_entry(GlobalOplock_Q.next, + oplock_item = list_entry(cifs_oplock_list.next, struct oplock_q_entry, qhead); cFYI(1, ("found oplock item to write out")); pTcon = oplock_item->tcon; inode = oplock_item->pinode; netfid = oplock_item->netfid; - spin_unlock(&GlobalMid_Lock); + spin_unlock(&cifs_oplock_lock); DeleteOplockQEntry(oplock_item); /* can not grab inode sem here since it would deadlock when oplock received on delete @@ -1055,7 +1055,7 @@ init_cifs(void) int rc = 0; cifs_proc_init(); INIT_LIST_HEAD(&cifs_tcp_ses_list); - INIT_LIST_HEAD(&GlobalOplock_Q); + INIT_LIST_HEAD(&cifs_oplock_list); #ifdef CONFIG_CIFS_EXPERIMENTAL INIT_LIST_HEAD(&GlobalDnotifyReqList); INIT_LIST_HEAD(&GlobalDnotifyRsp_Q); @@ -1084,6 +1084,7 @@ init_cifs(void) rwlock_init(&GlobalSMBSeslock); rwlock_init(&cifs_tcp_ses_lock); spin_lock_init(&GlobalMid_Lock); + spin_lock_init(&cifs_oplock_lock); if (cifs_max_pending < 2) { cifs_max_pending = 2; diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 6084d63..f100399 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock; */ GLOBAL_EXTERN rwlock_t GlobalSMBSeslock; -GLOBAL_EXTERN struct list_head GlobalOplock_Q; +/* Global list of oplocks */ +GLOBAL_EXTERN struct list_head cifs_oplock_list; + +/* Protects the cifs_oplock_list */ +GLOBAL_EXTERN spinlock_t cifs_oplock_lock; /* Outstanding dir notify requests */ GLOBAL_EXTERN struct list_head GlobalDnotifyReqList; diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 0ad3e2d..1da4ab2 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon) temp->pinode = pinode; temp->tcon = tcon; temp->netfid = fid; - spin_lock(&GlobalMid_Lock); - list_add_tail(&temp->qhead, &GlobalOplock_Q); - spin_unlock(&GlobalMid_Lock); + spin_lock(&cifs_oplock_lock); + list_add_tail(&temp->qhead, &cifs_oplock_list); + spin_unlock(&cifs_oplock_lock); } return temp; - } void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry) { - spin_lock(&GlobalMid_Lock); + spin_lock(&cifs_oplock_lock); /* should we check if list empty first? */ list_del(&oplockEntry->qhead); - spin_unlock(&GlobalMid_Lock); + spin_unlock(&cifs_oplock_lock); kmem_cache_free(cifs_oplock_cachep, oplockEntry); } @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon) if (tcon == NULL) return; - spin_lock(&GlobalMid_Lock); - list_for_each_entry(temp, &GlobalOplock_Q, qhead) { + spin_lock(&cifs_oplock_lock); + list_for_each_entry(temp, &cifs_oplock_list, qhead) { if ((temp->tcon) && (temp->tcon == tcon)) { list_del(&temp->qhead); kmem_cache_free(cifs_oplock_cachep, temp); } } - spin_unlock(&GlobalMid_Lock); + spin_unlock(&cifs_oplock_lock); } static int
Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That lock is also used for completely unrelated purposes (mostly for managing the global mid queue). Give the list its own dedicated spinlock (cifs_oplock_lock) and rename the list to cifs_oplock_list to eliminate the camel-case. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/cifsfs.c | 13 +++++++------ fs/cifs/cifsglob.h | 6 +++++- fs/cifs/transport.c | 17 ++++++++--------- 3 files changed, 20 insertions(+), 16 deletions(-)