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@xxxxxxxxxx>
> ---
> 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;
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
