2011/10/31 Pavel Shilovsky <piastry@xxxxxxxxxxx>:
> 2011/10/31 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>:
>> On Sat, 29 Oct 2011 17:17:58 +0400
>> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>>
>>> Now we allocate a lock structure at first, then we request to the server
>>> and save the lock if server returned OK though void function - it prevents
>>> the situation when we locked a file on the server and then return -ENOMEM
>>> from setlk.
>>>
>>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>>> ---
>>> fs/cifs/file.c | 64 ++++++++++++++++++++++++++++----------------------------
>>> 1 files changed, 32 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>>> index c1f063c..d9cc07f 100644
>>> --- a/fs/cifs/file.c
>>> +++ b/fs/cifs/file.c
>>> @@ -672,7 +672,7 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
>>> }
>>>
>>> static bool
>>> -cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>>> +__cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>>> __u64 length, __u8 type, __u16 netfid,
>>> struct cifsLockInfo **conf_lock)
>>> {
>>> @@ -694,6 +694,14 @@ cifs_find_lock_conflict(struct cifsInodeInfo *cinode, __u64 offset,
>>> return false;
>>> }
>>>
>>> +static bool
>>> +cifs_find_lock_conflict(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
>>> + struct cifsLockInfo **conf_lock)
>>> +{
>>> + return __cifs_find_lock_conflict(cinode, lock->offset, lock->length,
>>> + lock->type, lock->netfid, conf_lock);
>>> +}
>>> +
>>> static int
>>> cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>>> __u8 type, __u16 netfid, struct file_lock *flock)
>>> @@ -704,8 +712,8 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>>>
>>> mutex_lock(&cinode->lock_mutex);
>>>
>>> - exist = cifs_find_lock_conflict(cinode, offset, length, type, netfid,
>>> - &conf_lock);
>>> + exist = __cifs_find_lock_conflict(cinode, offset, length, type, netfid,
>>> + &conf_lock);
>>> if (exist) {
>>> flock->fl_start = conf_lock->offset;
>>> flock->fl_end = conf_lock->offset + conf_lock->length - 1;
>>> @@ -723,40 +731,27 @@ cifs_lock_test(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>>> return rc;
>>> }
>>>
>>> -static int
>>> -cifs_lock_add(struct cifsInodeInfo *cinode, __u64 len, __u64 offset,
>>> - __u8 type, __u16 netfid)
>>> +static void
>>> +cifs_lock_add(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock)
>>> {
>>> - struct cifsLockInfo *li;
>>> -
>>> - li = cifs_lock_init(len, offset, type, netfid);
>>> - if (!li)
>>> - return -ENOMEM;
>>> -
>>> mutex_lock(&cinode->lock_mutex);
>>> - list_add_tail(&li->llist, &cinode->llist);
>>> + list_add_tail(&lock->llist, &cinode->llist);
>>> mutex_unlock(&cinode->lock_mutex);
>>> - return 0;
>>> }
>>>
>>> static int
>>> -cifs_lock_add_if(struct cifsInodeInfo *cinode, __u64 offset, __u64 length,
>>> - __u8 type, __u16 netfid, bool wait)
>>> +cifs_lock_add_if(struct cifsInodeInfo *cinode, struct cifsLockInfo *lock,
>>> + bool wait)
>>> {
>>> - struct cifsLockInfo *lock, *conf_lock;
>>> + struct cifsLockInfo *conf_lock;
>>> bool exist;
>>> int rc = 0;
>>>
>>> - lock = cifs_lock_init(length, offset, type, netfid);
>>> - if (!lock)
>>> - return -ENOMEM;
>>> -
>>> try_again:
>>> exist = false;
>>> mutex_lock(&cinode->lock_mutex);
>>>
>>> - exist = cifs_find_lock_conflict(cinode, offset, length, type, netfid,
>>> - &conf_lock);
>>> + exist = cifs_find_lock_conflict(cinode, lock, &conf_lock);
>>> if (!exist && cinode->can_cache_brlcks) {
>>> list_add_tail(&lock->llist, &cinode->llist);
>>> mutex_unlock(&cinode->lock_mutex);
>>> @@ -781,7 +776,6 @@ try_again:
>>> }
>>> }
>>>
>>> - kfree(lock);
>>> mutex_unlock(&cinode->lock_mutex);
>>> return rc;
>>> }
>>> @@ -1254,20 +1248,26 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>>> }
>>>
>>> if (lock) {
>>> - rc = cifs_lock_add_if(cinode, flock->fl_start, length,
>>> - type, netfid, wait_flag);
>>> + struct cifsLockInfo *lock;
>>> +
>>> + lock = cifs_lock_init(length, flock->fl_start, type, netfid);
>>> + if (!lock)
>>> + return -ENOMEM;
>>> +
>>> + rc = cifs_lock_add_if(cinode, lock, wait_flag);
>>
>> Here, you're adding "lock" to the list...
>
> If we added the lock to the list cifs_lock_add_if returns 0 and we
> will jump to out label.
>
>>
>>> if (rc < 0)
>>> - return rc;
>>> - else if (!rc)
>>> + kfree(lock);
>>> + if (rc <= 0)
>>> goto out;
>>>
>>> rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>>> flock->fl_start, 0, 1, type, wait_flag, 0);
>>> - if (rc == 0) {
>>> - /* For Windows locks we must store them. */
>>> - rc = cifs_lock_add(cinode, length, flock->fl_start,
>>> - type, netfid);
>>> + if (rc) {
>>> + kfree(lock);
>>
>> ...and here you're freeing "lock" without removing it from the list.
>> Isn't that like to cause a problem?
>
> So, if CIFSSMBLock returns a error we free the lock that hasn't been
> added to the list. If CIFSSMBLock returns ok, we will add it to the
> list with cifs_lock_add void function.
>
> Seems no problem with it.
>
> --
> Best regards,
> Pavel Shilovsky.
>
So, to be clear - brlock cache function cifs_lock_add_if returns:
"0" if we store the lock in the cache and have an oplock for batching
brlocks - no need to request to the server.
"1" if there is no locks preventing us to set this lock but we haven't
an oplock for batching brlocks - need to request to the server and
then add the lock to the list if server accepts.
"-EACCESS" if we have a lock on the client that prevent us and this
call is non-blocked.
--
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
