This patch applies on 5.4-rc4 plus Pavel's patch: "CIFS: Fix retry mid list corruption on reconnects" v2 Changes * In the original version I had erroneously moved lock_sem inside cifs_find_lock_conflict but the lock_sem should be taken in the caller of that function not inside it. * I have started to run some basic locking tests with disruptions of the server and they run ok so far. There's a deadlock that is possible that can easily be seen with multiple readers open/read/close of the same file. The deadlock is due to a reader calling down_read(lock_sem) and holding it across the full IO, even if a network or server disruption occurs and the session has to be reconnected. Upon reconnect, cifs_relock_file is called where down_read(lock_sem) is called a second time. Normally this is not a problem, but if there is another process that calls down_write(lock_sem) in between the first and second reader call to down_read(lock_sem), this will cause a deadlock. The caller of down_write (often either _cifsFileInfo_put that is just removing and freeing cifsLockInfo structures from the list of locks, or cifs_new_fileinfo, which is just attaching cifs_fid_locks to cifsInodeInfo->llist), will block due to the reader's first down_read(lock_sem) that obtains the semaphore (read IO in flight). And then when the server comes back up, the reader that holds calls down_read(lock_sem) a second time, and this time is blocked too because of the blocked in down_write (rw_semaphores would starve writers if this was not the case). Interestingly enough, the callers of down_write in the simple test case was not adding a conflicting lock at all, just either opening or closing the file, and modifying the list of locks attached to cifsInodeInfo, this ends up tripping up the reader process and causing the deadlock. The root of the problem is that lock_sem both protects the cifsInodeInfo fields (such as the lllist - the list of locks), but is also being re-used to prevent a conflicting lock added while IO is in flight. Add a new semaphore that tracks just the IO in flight, and must be obtained before adding a new lock. While this does add another layer of complexity and a semaphore ordering that must be obeyed to avoid new deadlocks, it does clealy solve the underlying problem of the deadlock and double call to down_read by the same thread. Due to the removal of the possibility of a thread calling down_read(lock_sem) twice, this patch also reverts 560d388950ce ("CIFS: silence lockdep splat in cifs_relock_file()") Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx> --- fs/cifs/cifsfs.c | 1 + fs/cifs/cifsglob.h | 1 + fs/cifs/file.c | 32 +++++++++++++++++++++++++------- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index c049c7b3aa87..10f614324e4e 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1336,6 +1336,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off, inode_init_once(&cifsi->vfs_inode); init_rwsem(&cifsi->lock_sem); + init_rwsem(&cifsi->io_inflight_sem); } static int __init diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 50dfd9049370..40e8358dc1cc 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1392,6 +1392,7 @@ struct cifsInodeInfo { bool can_cache_brlcks; struct list_head llist; /* locks helb by this inode */ struct rw_semaphore lock_sem; /* protect the fields above */ + struct rw_semaphore io_inflight_sem; /* Used to avoid lock conflicts */ /* BB add in lists for dirty pages i.e. write caching info for oplock */ struct list_head openFileList; spinlock_t open_file_lock; /* protects openFileList */ diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 5ad15de2bb4f..a8b494205781 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -621,7 +621,7 @@ int cifs_open(struct inode *inode, struct file *file) struct cifs_tcon *tcon = tlink_tcon(cfile->tlink); int rc = 0; - down_read_nested(&cinode->lock_sem, SINGLE_DEPTH_NESTING); + down_read(&cinode->lock_sem); if (cinode->can_cache_brlcks) { /* can cache locks - no need to relock */ up_read(&cinode->lock_sem); @@ -1027,9 +1027,11 @@ int cifs_closedir(struct inode *inode, struct file *file) cifs_lock_add(struct cifsFileInfo *cfile, struct cifsLockInfo *lock) { struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry)); + down_write(&cinode->io_inflight_sem); down_write(&cinode->lock_sem); list_add_tail(&lock->llist, &cfile->llist->locks); up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); } /* @@ -1049,6 +1051,7 @@ int cifs_closedir(struct inode *inode, struct file *file) try_again: exist = false; + down_write(&cinode->io_inflight_sem); down_write(&cinode->lock_sem); exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length, @@ -1057,6 +1060,7 @@ int cifs_closedir(struct inode *inode, struct file *file) if (!exist && cinode->can_cache_brlcks) { list_add_tail(&lock->llist, &cfile->llist->locks); up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); return rc; } @@ -1077,6 +1081,7 @@ int cifs_closedir(struct inode *inode, struct file *file) } up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); return rc; } @@ -1125,14 +1130,17 @@ int cifs_closedir(struct inode *inode, struct file *file) return rc; try_again: + down_write(&cinode->io_inflight_sem); down_write(&cinode->lock_sem); if (!cinode->can_cache_brlcks) { up_write(&cinode->lock_sem); + down_write(&cinode->io_inflight_sem); return rc; } rc = posix_lock_file(file, flock, NULL); up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); if (rc == FILE_LOCK_DEFERRED) { rc = wait_event_interruptible(flock->fl_wait, !flock->fl_blocker); if (!rc) @@ -1331,6 +1339,7 @@ struct lock_to_push { int rc = 0; /* we are going to update can_cache_brlcks here - need a write access */ + down_write(&cinode->io_inflight_sem); down_write(&cinode->lock_sem); if (!cinode->can_cache_brlcks) { up_write(&cinode->lock_sem); @@ -1346,6 +1355,7 @@ struct lock_to_push { cinode->can_cache_brlcks = false; up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); return rc; } @@ -1522,6 +1532,7 @@ struct lock_to_push { if (!buf) return -ENOMEM; + down_write(&cinode->io_inflight_sem); down_write(&cinode->lock_sem); for (i = 0; i < 2; i++) { cur = buf; @@ -1593,6 +1604,7 @@ struct lock_to_push { } up_write(&cinode->lock_sem); + up_write(&cinode->io_inflight_sem); kfree(buf); return rc; } @@ -3148,20 +3160,23 @@ ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from) * We need to hold the sem to be sure nobody modifies lock list * with a brlock that prevents writing. */ - down_read(&cinode->lock_sem); + down_read(&cinode->io_inflight_sem); rc = generic_write_checks(iocb, from); if (rc <= 0) goto out; - if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from), + down_read(&cinode->lock_sem); + if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from), server->vals->exclusive_lock_type, 0, - NULL, CIFS_WRITE_OP)) + NULL, CIFS_WRITE_OP)) { + up_read(&cinode->lock_sem); rc = __generic_file_write_iter(iocb, from); + } else rc = -EACCES; out: - up_read(&cinode->lock_sem); + up_read(&cinode->io_inflight_sem); inode_unlock(inode); if (rc > 0) @@ -3887,12 +3902,15 @@ ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to) * We need to hold the sem to be sure nobody modifies lock list * with a brlock that prevents reading. */ + down_read(&cinode->io_inflight_sem); down_read(&cinode->lock_sem); if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to), tcon->ses->server->vals->shared_lock_type, - 0, NULL, CIFS_READ_OP)) + 0, NULL, CIFS_READ_OP)) { + up_read(&cinode->lock_sem); rc = generic_file_read_iter(iocb, to); - up_read(&cinode->lock_sem); + } + up_read(&cinode->io_inflight_sem); return rc; } -- 1.8.3.1