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.
--
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
