| Message ID | 20190603075922.27266-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: add global spinlock for the openFileList | expand |
пн, 3 июн. 2019 г. в 01:02, Ronnie Sahlberg <lsahlber@redhat.com>: > > We can not depend on the tcon->open_file_lock here since in multiuser mode > we may have the same file/inode open via multiple different tcons. > > The current code is race prone and will crash if one user deletes a file > at the same time a different user opens/create the file. > > RHBZ: 1580165 > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cifsfs.c | 1 + > fs/cifs/cifsglob.h | 5 +++++ > fs/cifs/file.c | 12 ++++++++++-- > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index f5fcd6360056..20cc4eaa7a49 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -1459,6 +1459,7 @@ init_cifs(void) > GlobalTotalActiveXid = 0; > GlobalMaxActiveXid = 0; > spin_lock_init(&cifs_tcp_ses_lock); > + spin_lock_init(&cifs_list_lock); > spin_lock_init(&GlobalMid_Lock); > > cifs_lock_secret = get_random_u32(); > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 334ff5f9c3f3..807b7cd7d48d 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head cifs_tcp_ses_list; > * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file > */ > GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock; > +/* > + * This lock protects the cifsInodeInfo->openFileList as well as > + * cifsFileInfo->flist|tlist. > + */ > +GLOBAL_EXTERN spinlock_t cifs_list_lock; > > #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */ > /* Outstanding dir notify requests */ > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 06e27ac6d82c..8e96a5ae83bf 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, > atomic_inc(&tcon->num_local_opens); > > /* if readable file instance put first in list*/ > + spin_lock(&cifs_list_lock); > if (file->f_mode & FMODE_READ) > list_add(&cfile->flist, &cinode->openFileList); > else > list_add_tail(&cfile->flist, &cinode->openFileList); > + spin_unlock(&cifs_list_lock); You are protecting cinode->openFileList here - this should be a lock per inode structure. > spin_unlock(&tcon->open_file_lock); > > if (fid->purge_cache) > @@ -413,8 +415,10 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) > cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open); > > /* remove it from the lists */ > + spin_lock(&cifs_list_lock); > list_del(&cifs_file->flist); The same here - inode lock. > list_del(&cifs_file->tlist); It is a list per tcon - existing tcon lock is protecting here. > + spin_unlock(&cifs_list_lock); > atomic_dec(&tcon->num_local_opens); > > if (list_empty(&cifsi->openFileList)) { > @@ -1459,8 +1463,10 @@ void > cifs_move_llist(struct list_head *source, struct list_head *dest) > { > struct list_head *li, *tmp; > + spin_lock(&cifs_list_lock); > list_for_each_safe(li, tmp, source) > list_move(li, dest); > + spin_unlock(&cifs_list_lock); > } > > void > @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist) > struct cifsLockInfo *li, *tmp; > list_for_each_entry_safe(li, tmp, llist, llist) { > cifs_del_lock_waiters(li); > + spin_lock(&cifs_list_lock); > list_del(&li->llist); > + spin_unlock(&cifs_list_lock); > kfree(li); > } > } Above two functions operate on lists of locks of inode's files - all protected by cifsi->lock_sem. > @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only, > return 0; > } > > - spin_lock(&tcon->open_file_lock); > + spin_lock(&cifs_list_lock); > list_move_tail(&inv_file->flist, &cifs_inode->openFileList); > - spin_unlock(&tcon->open_file_lock); > + spin_unlock(&cifs_list_lock); inode lock. > cifsFileInfo_put(inv_file); > ++refind; > inv_file = NULL; > -- > 2.13.6 > What is the reasoning under using a global spin lock? Global locking is always a source of performance problems and should be avoided as much as possible. -- Best regards, Pavel Shilovsky
On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > пн, 3 июн. 2019 г. в 01:02, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > We can not depend on the tcon->open_file_lock here since in multiuser mode > > we may have the same file/inode open via multiple different tcons. > > > > The current code is race prone and will crash if one user deletes a file > > at the same time a different user opens/create the file. > > > > RHBZ: 1580165 > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cifsfs.c | 1 + > > fs/cifs/cifsglob.h | 5 +++++ > > fs/cifs/file.c | 12 ++++++++++-- > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index f5fcd6360056..20cc4eaa7a49 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -1459,6 +1459,7 @@ init_cifs(void) > > GlobalTotalActiveXid = 0; > > GlobalMaxActiveXid = 0; > > spin_lock_init(&cifs_tcp_ses_lock); > > + spin_lock_init(&cifs_list_lock); > > spin_lock_init(&GlobalMid_Lock); > > > > cifs_lock_secret = get_random_u32(); > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index 334ff5f9c3f3..807b7cd7d48d 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head cifs_tcp_ses_list; > > * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file > > */ > > GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock; > > +/* > > + * This lock protects the cifsInodeInfo->openFileList as well as > > + * cifsFileInfo->flist|tlist. > > + */ > > +GLOBAL_EXTERN spinlock_t cifs_list_lock; > > > > #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */ > > /* Outstanding dir notify requests */ > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > index 06e27ac6d82c..8e96a5ae83bf 100644 > > --- a/fs/cifs/file.c > > +++ b/fs/cifs/file.c > > @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, > > atomic_inc(&tcon->num_local_opens); > > > > /* if readable file instance put first in list*/ > > + spin_lock(&cifs_list_lock); > > if (file->f_mode & FMODE_READ) > > list_add(&cfile->flist, &cinode->openFileList); > > else > > list_add_tail(&cfile->flist, &cinode->openFileList); > > + spin_unlock(&cifs_list_lock); > > You are protecting cinode->openFileList here - this should be a lock > per inode structure. > > > spin_unlock(&tcon->open_file_lock); > > > > if (fid->purge_cache) > > @@ -413,8 +415,10 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) > > cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open); > > > > /* remove it from the lists */ > > + spin_lock(&cifs_list_lock); > > list_del(&cifs_file->flist); > > The same here - inode lock. > > > > list_del(&cifs_file->tlist); > > It is a list per tcon - existing tcon lock is protecting here. > > > + spin_unlock(&cifs_list_lock); > > atomic_dec(&tcon->num_local_opens); > > > > if (list_empty(&cifsi->openFileList)) { > > @@ -1459,8 +1463,10 @@ void > > cifs_move_llist(struct list_head *source, struct list_head *dest) > > { > > struct list_head *li, *tmp; > > + spin_lock(&cifs_list_lock); > > list_for_each_safe(li, tmp, source) > > list_move(li, dest); > > + spin_unlock(&cifs_list_lock); > > } > > > > void > > @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist) > > struct cifsLockInfo *li, *tmp; > > list_for_each_entry_safe(li, tmp, llist, llist) { > > cifs_del_lock_waiters(li); > > + spin_lock(&cifs_list_lock); > > list_del(&li->llist); > > + spin_unlock(&cifs_list_lock); > > kfree(li); > > } > > } > > Above two functions operate on lists of locks of inode's files - all > protected by cifsi->lock_sem. > > > @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only, > > return 0; > > } > > > > - spin_lock(&tcon->open_file_lock); > > + spin_lock(&cifs_list_lock); > > list_move_tail(&inv_file->flist, &cifs_inode->openFileList); > > - spin_unlock(&tcon->open_file_lock); > > + spin_unlock(&cifs_list_lock); > > inode lock. > > > cifsFileInfo_put(inv_file); > > ++refind; > > inv_file = NULL; > > -- > > 2.13.6 > > > > What is the reasoning under using a global spin lock? Global locking > is always a source of performance problems and should be avoided as > much as possible. In multiuser each user has their own tcon so if user A and user B does a list_add/list_del at the same time they are not protected against eachother sicne A and B use different tcon->open_file_list spinlocks :-( I will do the other changes you suggest later today and re-send > > -- > Best regards, > Pavel Shilovsky
пн, 3 июн. 2019 г. в 16:21, ronnie sahlberg <ronniesahlberg@gmail.com>: > > On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > > пн, 3 июн. 2019 г. в 01:02, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > > > We can not depend on the tcon->open_file_lock here since in multiuser mode > > > we may have the same file/inode open via multiple different tcons. > > > > > > The current code is race prone and will crash if one user deletes a file > > > at the same time a different user opens/create the file. > > > > > > RHBZ: 1580165 > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/cifsfs.c | 1 + > > > fs/cifs/cifsglob.h | 5 +++++ > > > fs/cifs/file.c | 12 ++++++++++-- > > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > > index f5fcd6360056..20cc4eaa7a49 100644 > > > --- a/fs/cifs/cifsfs.c > > > +++ b/fs/cifs/cifsfs.c > > > @@ -1459,6 +1459,7 @@ init_cifs(void) > > > GlobalTotalActiveXid = 0; > > > GlobalMaxActiveXid = 0; > > > spin_lock_init(&cifs_tcp_ses_lock); > > > + spin_lock_init(&cifs_list_lock); > > > spin_lock_init(&GlobalMid_Lock); > > > > > > cifs_lock_secret = get_random_u32(); > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > > index 334ff5f9c3f3..807b7cd7d48d 100644 > > > --- a/fs/cifs/cifsglob.h > > > +++ b/fs/cifs/cifsglob.h > > > @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head cifs_tcp_ses_list; > > > * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file > > > */ > > > GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock; > > > +/* > > > + * This lock protects the cifsInodeInfo->openFileList as well as > > > + * cifsFileInfo->flist|tlist. > > > + */ > > > +GLOBAL_EXTERN spinlock_t cifs_list_lock; > > > > > > #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */ > > > /* Outstanding dir notify requests */ > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > > index 06e27ac6d82c..8e96a5ae83bf 100644 > > > --- a/fs/cifs/file.c > > > +++ b/fs/cifs/file.c > > > @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, > > > atomic_inc(&tcon->num_local_opens); > > > > > > /* if readable file instance put first in list*/ > > > + spin_lock(&cifs_list_lock); > > > if (file->f_mode & FMODE_READ) > > > list_add(&cfile->flist, &cinode->openFileList); > > > else > > > list_add_tail(&cfile->flist, &cinode->openFileList); > > > + spin_unlock(&cifs_list_lock); > > > > You are protecting cinode->openFileList here - this should be a lock > > per inode structure. > > > > > spin_unlock(&tcon->open_file_lock); > > > > > > if (fid->purge_cache) > > > @@ -413,8 +415,10 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) > > > cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open); > > > > > > /* remove it from the lists */ > > > + spin_lock(&cifs_list_lock); > > > list_del(&cifs_file->flist); > > > > The same here - inode lock. > > > > > > > list_del(&cifs_file->tlist); > > > > It is a list per tcon - existing tcon lock is protecting here. > > > > > + spin_unlock(&cifs_list_lock); > > > atomic_dec(&tcon->num_local_opens); > > > > > > if (list_empty(&cifsi->openFileList)) { > > > @@ -1459,8 +1463,10 @@ void > > > cifs_move_llist(struct list_head *source, struct list_head *dest) > > > { > > > struct list_head *li, *tmp; > > > + spin_lock(&cifs_list_lock); > > > list_for_each_safe(li, tmp, source) > > > list_move(li, dest); > > > + spin_unlock(&cifs_list_lock); > > > } > > > > > > void > > > @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist) > > > struct cifsLockInfo *li, *tmp; > > > list_for_each_entry_safe(li, tmp, llist, llist) { > > > cifs_del_lock_waiters(li); > > > + spin_lock(&cifs_list_lock); > > > list_del(&li->llist); > > > + spin_unlock(&cifs_list_lock); > > > kfree(li); > > > } > > > } > > > > Above two functions operate on lists of locks of inode's files - all > > protected by cifsi->lock_sem. > > > > > @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only, > > > return 0; > > > } > > > > > > - spin_lock(&tcon->open_file_lock); > > > + spin_lock(&cifs_list_lock); > > > list_move_tail(&inv_file->flist, &cifs_inode->openFileList); > > > - spin_unlock(&tcon->open_file_lock); > > > + spin_unlock(&cifs_list_lock); > > > > inode lock. > > > > > cifsFileInfo_put(inv_file); > > > ++refind; > > > inv_file = NULL; > > > -- > > > 2.13.6 > > > > > > > What is the reasoning under using a global spin lock? Global locking > > is always a source of performance problems and should be avoided as > > much as possible. > > In multiuser each user has their own tcon so if user A and user B > does a list_add/list_del at the same time > they are not protected against eachother sicne A and B use different > tcon->open_file_list spinlocks :-( In this case both users still have the common thing to lock - inode, so, locking should be a part of the inode structure to not slow access to other files on the file system. We also need to agree on the locking order here - tcon first and inode second looks fine unless anybody has objections. > > I will do the other changes you suggest later today and re-send Thanks for spotting the problem! It seems that multiuser mounts don't have that much of usage but *theoretically* this is a right way to setup SMB authorization on Linux. -- Best regards, Pavel Shilovsky
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index f5fcd6360056..20cc4eaa7a49 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1459,6 +1459,7 @@ init_cifs(void) GlobalTotalActiveXid = 0; GlobalMaxActiveXid = 0; spin_lock_init(&cifs_tcp_ses_lock); + spin_lock_init(&cifs_list_lock); spin_lock_init(&GlobalMid_Lock); cifs_lock_secret = get_random_u32(); diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 334ff5f9c3f3..807b7cd7d48d 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head cifs_tcp_ses_list; * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file */ GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock; +/* + * This lock protects the cifsInodeInfo->openFileList as well as + * cifsFileInfo->flist|tlist. + */ +GLOBAL_EXTERN spinlock_t cifs_list_lock; #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */ /* Outstanding dir notify requests */ diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 06e27ac6d82c..8e96a5ae83bf 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, atomic_inc(&tcon->num_local_opens); /* if readable file instance put first in list*/ + spin_lock(&cifs_list_lock); if (file->f_mode & FMODE_READ) list_add(&cfile->flist, &cinode->openFileList); else list_add_tail(&cfile->flist, &cinode->openFileList); + spin_unlock(&cifs_list_lock); spin_unlock(&tcon->open_file_lock); if (fid->purge_cache) @@ -413,8 +415,10 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open); /* remove it from the lists */ + spin_lock(&cifs_list_lock); list_del(&cifs_file->flist); list_del(&cifs_file->tlist); + spin_unlock(&cifs_list_lock); atomic_dec(&tcon->num_local_opens); if (list_empty(&cifsi->openFileList)) { @@ -1459,8 +1463,10 @@ void cifs_move_llist(struct list_head *source, struct list_head *dest) { struct list_head *li, *tmp; + spin_lock(&cifs_list_lock); list_for_each_safe(li, tmp, source) list_move(li, dest); + spin_unlock(&cifs_list_lock); } void @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist) struct cifsLockInfo *li, *tmp; list_for_each_entry_safe(li, tmp, llist, llist) { cifs_del_lock_waiters(li); + spin_lock(&cifs_list_lock); list_del(&li->llist); + spin_unlock(&cifs_list_lock); kfree(li); } } @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only, return 0; } - spin_lock(&tcon->open_file_lock); + spin_lock(&cifs_list_lock); list_move_tail(&inv_file->flist, &cifs_inode->openFileList); - spin_unlock(&tcon->open_file_lock); + spin_unlock(&cifs_list_lock); cifsFileInfo_put(inv_file); ++refind; inv_file = NULL;
We can not depend on the tcon->open_file_lock here since in multiuser mode we may have the same file/inode open via multiple different tcons. The current code is race prone and will crash if one user deletes a file at the same time a different user opens/create the file. RHBZ: 1580165 Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cifsfs.c | 1 + fs/cifs/cifsglob.h | 5 +++++ fs/cifs/file.c | 12 ++++++++++-- 3 files changed, 16 insertions(+), 2 deletions(-)