Hi Pavel,
Thanks for the review!
On August 2, 2018 7:17:42 PM GMT-03:00, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>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.
OK.
>
>>
>> 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).
You're right. I will fix it.
>
>> + 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?
Nope, it isn't. Your comments make sense and the logic is broken. I will fix it in next version.
>
>
>> 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
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
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
