| Message ID | 20180627034540.30762-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: OFD locks do not conflict with eachothers | expand |
On Wed, 2018-06-27 at 13:45 +1000, Ronnie Sahlberg wrote: > RHBZ 1484130 > > Update cifs_find_fid_lock_conflict() to recognize that > ODF locks do not conflict with eachother. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cifsglob.h | 3 ++- > fs/cifs/cifsproto.h | 2 +- > fs/cifs/file.c | 34 +++++++++++++++++++++------------- > 3 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 0543187fe707..a0c92ed656c5 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1092,7 +1092,8 @@ struct cifsLockInfo { > __u64 offset; > __u64 length; > __u32 pid; > - __u32 type; > + __u16 type; > + __u16 flags; > }; > > /* > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index e23eec5372df..b8a6a228b1a7 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -214,7 +214,7 @@ extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon); > extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon); > > extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, > - __u64 length, __u8 type, > + __u64 length, __u8 type, __u16 flags, > struct cifsLockInfo **conf_lock, > int rw_check); > extern void cifs_add_pending_open(struct cifs_fid *fid, > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 8d41ca7bfcf1..3efd150c61b3 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -864,7 +864,7 @@ int cifs_closedir(struct inode *inode, struct file *file) > } > > static struct cifsLockInfo * > -cifs_lock_init(__u64 offset, __u64 length, __u8 type) > +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags) > { > struct cifsLockInfo *lock = > kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); > @@ -874,6 +874,7 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type) > lock->length = length; > lock->type = type; > lock->pid = current->tgid; > + lock->flags = flags; > INIT_LIST_HEAD(&lock->blist); > init_waitqueue_head(&lock->block_q); > return lock; > @@ -896,7 +897,8 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock) > /* @rw_check : 0 - no op, 1 - read, 2 - write */ > static bool > cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > - __u64 length, __u8 type, struct cifsFileInfo *cfile, > + __u64 length, __u8 type, __u16 flags, > + struct cifsFileInfo *cfile, > struct cifsLockInfo **conf_lock, int rw_check) > { > struct cifsLockInfo *li; > @@ -918,6 +920,9 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > ((server->ops->compare_fids(cfile, cur_cfile) && > current->tgid == li->pid) || type == li->type)) > continue; > + if (rw_check == CIFS_LOCK_OP && > + flags & FL_OFDLCK && li->flags & FL_OFDLCK) Maybe some parenthesis around those flags checks? I can never remember whether && or & takes precedence. > + continue; This looks like it'll fix the reported problem, but the logic in this function just keeps getting more complex with all of the special casing. It might be nice to start by documenting the logic with some comments in this code and then maybe rework this function to be more understandable. Also, don't classic POSIX locks have the same problem here? If you do s/F_OFD_/F_/ in Laura's test program, wouldn't it fail in the same way? > if (conf_lock) > *conf_lock = li; > return true; > @@ -927,8 +932,8 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > > bool > cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, > - __u8 type, struct cifsLockInfo **conf_lock, > - int rw_check) > + __u8 type, __u16 flags, > + struct cifsLockInfo **conf_lock, int rw_check) > { > bool rc = false; > struct cifs_fid_locks *cur; > @@ -936,7 +941,8 @@ cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, > > list_for_each_entry(cur, &cinode->llist, llist) { > rc = cifs_find_fid_lock_conflict(cur, offset, length, type, > - cfile, conf_lock, rw_check); > + flags, cfile, conf_lock, > + rw_check); > if (rc) > break; > } > @@ -964,7 +970,8 @@ cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length, > down_read(&cinode->lock_sem); > > exist = cifs_find_lock_conflict(cfile, offset, length, type, > - &conf_lock, CIFS_LOCK_OP); > + flock->fl_flags, &conf_lock, > + CIFS_LOCK_OP); > if (exist) { > flock->fl_start = conf_lock->offset; > flock->fl_end = conf_lock->offset + conf_lock->length - 1; > @@ -1011,7 +1018,8 @@ cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock, > down_write(&cinode->lock_sem); > > exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length, > - lock->type, &conf_lock, CIFS_LOCK_OP); > + lock->type, lock->flags, &conf_lock, > + CIFS_LOCK_OP); > if (!exist && cinode->can_cache_brlcks) { > list_add_tail(&lock->llist, &cfile->llist->locks); > up_write(&cinode->lock_sem); > @@ -1321,7 +1329,7 @@ cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock, > cifs_dbg(FYI, "Lease on file - not implemented yet\n"); > if (flock->fl_flags & > (~(FL_POSIX | FL_FLOCK | FL_SLEEP | > - FL_ACCESS | FL_LEASE | FL_CLOSE))) > + FL_ACCESS | FL_LEASE | FL_CLOSE | FL_OFDLCK))) > cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags); > > *type = server->vals->large_lock_type; > @@ -1584,7 +1592,8 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u32 type, > if (lock) { > struct cifsLockInfo *lock; > > - lock = cifs_lock_init(flock->fl_start, length, type); > + lock = cifs_lock_init(flock->fl_start, length, type, > + flock->fl_flags); > if (!lock) > return -ENOMEM; > > @@ -1653,7 +1662,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock) > > cifs_read_flock(flock, &type, &lock, &unlock, &wait_flag, > tcon->ses->server); > - > cifs_sb = CIFS_FILE_SB(file); > netfid = cfile->fid.netfid; > cinode = CIFS_I(file_inode(file)); > @@ -2817,8 +2825,8 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from) > goto out; > > if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from), > - server->vals->exclusive_lock_type, NULL, > - CIFS_WRITE_OP)) > + server->vals->exclusive_lock_type, 0, > + NULL, CIFS_WRITE_OP)) > rc = __generic_file_write_iter(iocb, from); > else > rc = -EACCES; > @@ -3388,7 +3396,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to) > 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, > - NULL, CIFS_READ_OP)) > + 0, NULL, CIFS_READ_OP)) > rc = generic_file_read_iter(iocb, to); > up_read(&cinode->lock_sem); > return rc;
вт, 26 июн. 2018 г. в 22:03, Jeff Layton <jlayton@redhat.com>: > > On Wed, 2018-06-27 at 13:45 +1000, Ronnie Sahlberg wrote: > > RHBZ 1484130 > > > > Update cifs_find_fid_lock_conflict() to recognize that > > ODF locks do not conflict with eachother. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cifsglob.h | 3 ++- > > fs/cifs/cifsproto.h | 2 +- > > fs/cifs/file.c | 34 +++++++++++++++++++++------------- > > 3 files changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index 0543187fe707..a0c92ed656c5 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1092,7 +1092,8 @@ struct cifsLockInfo { > > __u64 offset; > > __u64 length; > > __u32 pid; > > - __u32 type; > > + __u16 type; > > + __u16 flags; > > }; > > > > /* > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > > index e23eec5372df..b8a6a228b1a7 100644 > > --- a/fs/cifs/cifsproto.h > > +++ b/fs/cifs/cifsproto.h > > @@ -214,7 +214,7 @@ extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon); > > extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon); > > > > extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, > > - __u64 length, __u8 type, > > + __u64 length, __u8 type, __u16 flags, > > struct cifsLockInfo **conf_lock, > > int rw_check); > > extern void cifs_add_pending_open(struct cifs_fid *fid, > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > index 8d41ca7bfcf1..3efd150c61b3 100644 > > --- a/fs/cifs/file.c > > +++ b/fs/cifs/file.c > > @@ -864,7 +864,7 @@ int cifs_closedir(struct inode *inode, struct file *file) > > } > > > > static struct cifsLockInfo * > > -cifs_lock_init(__u64 offset, __u64 length, __u8 type) > > +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags) > > { > > struct cifsLockInfo *lock = > > kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); > > @@ -874,6 +874,7 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type) > > lock->length = length; > > lock->type = type; > > lock->pid = current->tgid; > > + lock->flags = flags; > > INIT_LIST_HEAD(&lock->blist); > > init_waitqueue_head(&lock->block_q); > > return lock; > > @@ -896,7 +897,8 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock) > > /* @rw_check : 0 - no op, 1 - read, 2 - write */ > > static bool > > cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > > - __u64 length, __u8 type, struct cifsFileInfo *cfile, > > + __u64 length, __u8 type, __u16 flags, > > + struct cifsFileInfo *cfile, > > struct cifsLockInfo **conf_lock, int rw_check) > > { > > struct cifsLockInfo *li; > > @@ -918,6 +920,9 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, > > ((server->ops->compare_fids(cfile, cur_cfile) && > > current->tgid == li->pid) || type == li->type)) > > continue; > > + if (rw_check == CIFS_LOCK_OP && > > + flags & FL_OFDLCK && li->flags & FL_OFDLCK) > > Maybe some parenthesis around those flags checks? I can never remember > whether && or & takes precedence. > > > + continue; > > This looks like it'll fix the reported problem, but the logic in this > function just keeps getting more complex with all of the special > casing. It might be nice to start by documenting the logic with some > comments in this code and then maybe rework this function to be more > understandable. > > Also, don't classic POSIX locks have the same problem here? If you do > s/F_OFD_/F_/ in Laura's test program, wouldn't it fail in the same way? As far as I remember cifs_find_fid_lock_conflict() does the same logic the SMB server would do if the client decided to skip internal checks and proceeded to the remote server directly. So, for classic POSIX locks, the proposed test program should fail too. I don't think there is much we can do in general case to fix it, but we can fix the case where locks are set/get within the same mount. Anyway, this check: > > + if (rw_check == CIFS_LOCK_OP && > > + flags & FL_OFDLCK && li->flags & FL_OFDLCK) > > + continue; doesn't look right: it basically allows any FD with a WRITE lock to overwrite the existing READ lock held by a different FD. It seems that we need to check if file ids are equal here. -- Best regards, Pavel Shilovsky -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 0543187fe707..a0c92ed656c5 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1092,7 +1092,8 @@ struct cifsLockInfo { __u64 offset; __u64 length; __u32 pid; - __u32 type; + __u16 type; + __u16 flags; }; /* diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index e23eec5372df..b8a6a228b1a7 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -214,7 +214,7 @@ extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon); extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon); extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, - __u64 length, __u8 type, + __u64 length, __u8 type, __u16 flags, struct cifsLockInfo **conf_lock, int rw_check); extern void cifs_add_pending_open(struct cifs_fid *fid, diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 8d41ca7bfcf1..3efd150c61b3 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -864,7 +864,7 @@ int cifs_closedir(struct inode *inode, struct file *file) } static struct cifsLockInfo * -cifs_lock_init(__u64 offset, __u64 length, __u8 type) +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 flags) { struct cifsLockInfo *lock = kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL); @@ -874,6 +874,7 @@ cifs_lock_init(__u64 offset, __u64 length, __u8 type) lock->length = length; lock->type = type; lock->pid = current->tgid; + lock->flags = flags; INIT_LIST_HEAD(&lock->blist); init_waitqueue_head(&lock->block_q); return lock; @@ -896,7 +897,8 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock) /* @rw_check : 0 - no op, 1 - read, 2 - write */ static bool cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, - __u64 length, __u8 type, struct cifsFileInfo *cfile, + __u64 length, __u8 type, __u16 flags, + struct cifsFileInfo *cfile, struct cifsLockInfo **conf_lock, int rw_check) { struct cifsLockInfo *li; @@ -918,6 +920,9 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, ((server->ops->compare_fids(cfile, cur_cfile) && current->tgid == li->pid) || type == li->type)) continue; + if (rw_check == CIFS_LOCK_OP && + flags & FL_OFDLCK && li->flags & FL_OFDLCK) + continue; if (conf_lock) *conf_lock = li; return true; @@ -927,8 +932,8 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset, bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, - __u8 type, struct cifsLockInfo **conf_lock, - int rw_check) + __u8 type, __u16 flags, + struct cifsLockInfo **conf_lock, int rw_check) { bool rc = false; struct cifs_fid_locks *cur; @@ -936,7 +941,8 @@ cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length, list_for_each_entry(cur, &cinode->llist, llist) { rc = cifs_find_fid_lock_conflict(cur, offset, length, type, - cfile, conf_lock, rw_check); + flags, cfile, conf_lock, + rw_check); if (rc) break; } @@ -964,7 +970,8 @@ cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length, down_read(&cinode->lock_sem); exist = cifs_find_lock_conflict(cfile, offset, length, type, - &conf_lock, CIFS_LOCK_OP); + flock->fl_flags, &conf_lock, + CIFS_LOCK_OP); if (exist) { flock->fl_start = conf_lock->offset; flock->fl_end = conf_lock->offset + conf_lock->length - 1; @@ -1011,7 +1018,8 @@ cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock, down_write(&cinode->lock_sem); exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length, - lock->type, &conf_lock, CIFS_LOCK_OP); + lock->type, lock->flags, &conf_lock, + CIFS_LOCK_OP); if (!exist && cinode->can_cache_brlcks) { list_add_tail(&lock->llist, &cfile->llist->locks); up_write(&cinode->lock_sem); @@ -1321,7 +1329,7 @@ cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock, cifs_dbg(FYI, "Lease on file - not implemented yet\n"); if (flock->fl_flags & (~(FL_POSIX | FL_FLOCK | FL_SLEEP | - FL_ACCESS | FL_LEASE | FL_CLOSE))) + FL_ACCESS | FL_LEASE | FL_CLOSE | FL_OFDLCK))) cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags); *type = server->vals->large_lock_type; @@ -1584,7 +1592,8 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u32 type, if (lock) { struct cifsLockInfo *lock; - lock = cifs_lock_init(flock->fl_start, length, type); + lock = cifs_lock_init(flock->fl_start, length, type, + flock->fl_flags); if (!lock) return -ENOMEM; @@ -1653,7 +1662,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock) cifs_read_flock(flock, &type, &lock, &unlock, &wait_flag, tcon->ses->server); - cifs_sb = CIFS_FILE_SB(file); netfid = cfile->fid.netfid; cinode = CIFS_I(file_inode(file)); @@ -2817,8 +2825,8 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from) goto out; if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from), - server->vals->exclusive_lock_type, NULL, - CIFS_WRITE_OP)) + server->vals->exclusive_lock_type, 0, + NULL, CIFS_WRITE_OP)) rc = __generic_file_write_iter(iocb, from); else rc = -EACCES; @@ -3388,7 +3396,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to) 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, - NULL, CIFS_READ_OP)) + 0, NULL, CIFS_READ_OP)) rc = generic_file_read_iter(iocb, to); up_read(&cinode->lock_sem); return rc;
RHBZ 1484130 Update cifs_find_fid_lock_conflict() to recognize that ODF locks do not conflict with eachother. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cifsglob.h | 3 ++- fs/cifs/cifsproto.h | 2 +- fs/cifs/file.c | 34 +++++++++++++++++++++------------- 3 files changed, 24 insertions(+), 15 deletions(-)