2018-08-02 11:52 GMT-07:00 Paulo Alcantara <paulo@xxxxxxxx>:
> OFD locks never conflict each other.
I guess it should be mentioned that OFD locks never conflicts if they
are set through the same FD.
>
> Signed-off-by: Paulo Alcantara <palcantara@xxxxxxx>
> ---
> fs/cifs/cifsglob.h | 1 +
> fs/cifs/cifsproto.h | 4 --
> fs/cifs/file.c | 109 +++++++++++++++++++++++++++++---------------
> 3 files changed, 74 insertions(+), 40 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0553929e8339..bb55e9aa4fe3 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1069,6 +1069,7 @@ struct cifsLockInfo {
> __u64 length;
> __u32 pid;
> __u32 type;
> + unsigned int flags; /* file lock flags */
> };
>
> /*
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 20adda4de83b..e6a72a3f597b 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -218,10 +218,6 @@ extern void cifs_umount(struct cifs_sb_info *);
> 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,
> - struct cifsLockInfo **conf_lock,
> - int rw_check);
> extern void cifs_add_pending_open(struct cifs_fid *fid,
> struct tcon_link *tlink,
> struct cifs_pending_open *open);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 8d41ca7bfcf1..58ee94550e42 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -864,14 +864,15 @@ int cifs_closedir(struct inode *inode, struct file *file)
> }
>
> static struct cifsLockInfo *
> -cifs_lock_init(__u64 offset, __u64 length, __u8 type)
> +cifs_lock_init(struct file_lock *fl, __u8 type)
> {
> struct cifsLockInfo *lock =
> kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
> if (!lock)
> return lock;
> - lock->offset = offset;
> - lock->length = length;
> + lock->offset = fl->fl_start;
> + lock->length = fl->fl_end - fl->fl_start + 1;
> + lock->flags = fl->fl_flags;
> lock->type = type;
> lock->pid = current->tgid;
> INIT_LIST_HEAD(&lock->blist);
> @@ -893,30 +894,60 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
> #define CIFS_READ_OP 1
> #define CIFS_WRITE_OP 2
>
> -/* @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,
> - struct cifsLockInfo **conf_lock, int rw_check)
> +static inline bool locks_overlap(struct cifsLockInfo *li, __u64 offset,
> + __u64 length)
> +{
> + return offset + length > li->offset &&
> + offset < li->offset + li->length;
> +}
> +
> +static inline bool
> +locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo *cur_cfile,
> + struct cifsLockInfo *li, __u64 offset, __u64 length,
> + __u8 type, unsigned int flags, struct TCP_Server_Info *server,
> + int rw_check)
> +{
> + if (!locks_overlap(li, offset, length))
> + return false;
> + if (!server->ops->compare_fids(cfile, cur_cfile))
> + return true;
If FIDs don't match locks are considered conflicting which is not true
for shared locks.
> + if (current->tgid != li->pid)
> + return true;
> +
> + if (rw_check == CIFS_LOCK_OP) {
> + if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK))
> + return false;
> + if ((flags & FL_POSIX) && (li->flags & FL_POSIX))
> + return false;
Here you are changing the behavior for POSIX locks that doesn't
reflect the patch title/message.
> + if (li->type & server->vals->exclusive_lock_type)
> + return true;
With the change above locking a file with a shared lock already locked
by an exclusive lock from the same FD and PID won't be allowed which
contradict LockFileEx semantics
(https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex).
> + if (type & server->vals->exclusive_lock_type)
> + return true;
> + return false;
> + }
> +
> + /* shared lock prevents write op through the same fid */
> + if (!(li->type & server->vals->shared_lock_type) ||
> + rw_check != CIFS_WRITE_OP)
> + return false;
> +
> + return true;
> +}
> +
> +static inline bool __fid_lock_conflicts(struct cifs_fid_locks *fdlocks,
> + __u64 offset, __u64 length,
> + __u8 type, unsigned int flags,
> + struct cifsFileInfo *cfile,
> + struct cifsLockInfo **conf_lock,
> + int rw_check)
> {
> struct cifsLockInfo *li;
> struct cifsFileInfo *cur_cfile = fdlocks->cfile;
> struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
>
> list_for_each_entry(li, &fdlocks->locks, llist) {
> - if (offset + length <= li->offset ||
> - offset >= li->offset + li->length)
> - continue;
> - if (rw_check != CIFS_LOCK_OP && current->tgid == li->pid &&
> - server->ops->compare_fids(cfile, cur_cfile)) {
> - /* shared lock prevents write op through the same fid */
> - if (!(li->type & server->vals->shared_lock_type) ||
> - rw_check != CIFS_WRITE_OP)
> - continue;
> - }
> - if ((type & server->vals->shared_lock_type) &&
> - ((server->ops->compare_fids(cfile, cur_cfile) &&
> - current->tgid == li->pid) || type == li->type))
> + if (!locks_conflict(cfile, cur_cfile, li, offset, length, type,
> + flags, server, rw_check))
I am remembering that the existing logic is trying to match the SMB
server behavior for SMB locks (see LockFileEx), so we won't have
conflicts when we get a oplock/lease break and push byte-range locks
to the server.
This change will break the existing behavior for both POSIX and OFD
locks: while you allow locks to be set locally, server will reject
them and we will end up in a mixed state. Is it desired behavior?
> continue;
> if (conf_lock)
> *conf_lock = li;
> @@ -925,18 +956,19 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
> return false;
> }
>
> -bool
> -cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
> - __u8 type, struct cifsLockInfo **conf_lock,
> - int rw_check)
> +static bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
> + __u64 length, __u8 type, unsigned int flags,
> + struct cifsLockInfo **conf_lock,
> + int rw_check)
> {
> bool rc = false;
> struct cifs_fid_locks *cur;
> struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry));
>
> list_for_each_entry(cur, &cinode->llist, llist) {
> - rc = cifs_find_fid_lock_conflict(cur, offset, length, type,
> - cfile, conf_lock, rw_check);
> + rc = __fid_lock_conflicts(cur, offset, length, type,
> + flags, cfile, conf_lock,
> + rw_check);
> if (rc)
> break;
> }
> @@ -964,7 +996,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 +1044,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);
> @@ -1307,8 +1341,15 @@ static void
> cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
> bool *wait_flag, struct TCP_Server_Info *server)
> {
> + unsigned int flags = FL_POSIX | FL_OFDLCK | FL_FLOCK | FL_SLEEP |
> + FL_ACCESS | FL_LEASE | FL_CLOSE;
> +
> + if (flock->fl_flags & ~flags)
> + cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
> if (flock->fl_flags & FL_POSIX)
> cifs_dbg(FYI, "Posix\n");
> + if (flock->fl_flags & FL_OFDLCK)
> + cifs_dbg(FYI, "OFD lock\n");
> if (flock->fl_flags & FL_FLOCK)
> cifs_dbg(FYI, "Flock\n");
> if (flock->fl_flags & FL_SLEEP) {
> @@ -1319,10 +1360,6 @@ cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
> cifs_dbg(FYI, "Process suspended by mandatory locking - not implemented yet\n");
> if (flock->fl_flags & FL_LEASE)
> 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)))
> - cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
>
> *type = server->vals->large_lock_type;
> if (flock->fl_type == F_WRLCK) {
> @@ -1584,7 +1621,7 @@ 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, type);
> if (!lock)
> return -ENOMEM;
>
> @@ -2817,7 +2854,7 @@ 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,
> + server->vals->exclusive_lock_type, 0, NULL,
> CIFS_WRITE_OP))
> rc = __generic_file_write_iter(iocb, from);
> else
> @@ -3388,7 +3425,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;
> --
> 2.18.0
>
> --
> 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
Best regards,
Pavel Shilovsky
--
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
