On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
> пн, 3 июн. 2019 г. в 01:02, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
