[PATCH 1/3] CIFS: Fix the VFS brlock cache usage in posix locking case

[PATCH 1/3] CIFS: Fix the VFS brlock cache usage in posix locking case

[Pavel Shilovsky]

Request to the cache in FL_POSIX case only.

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/file.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 91e05f2..c1f063c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -793,6 +793,9 @@ cifs_posix_lock_test(struct file *file, struct file_lock *flock)
 	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
 	unsigned char saved_type = flock->fl_type;
 
+	if ((flock->fl_flags & FL_POSIX) == 0)
+		return 1;
+
 	mutex_lock(&cinode->lock_mutex);
 	posix_test_lock(file, flock);
 
@@ -809,12 +812,15 @@ static int
 cifs_posix_lock_set(struct file *file, struct file_lock *flock)
 {
 	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
-	int rc;
+	int rc = 1;
+
+	if ((flock->fl_flags & FL_POSIX) == 0)
+		return rc;
 
 	mutex_lock(&cinode->lock_mutex);
 	if (!cinode->can_cache_brlcks) {
 		mutex_unlock(&cinode->lock_mutex);
-		return 1;
+		return rc;
 	}
 	rc = posix_lock_file_wait(file, flock);
 	mutex_unlock(&cinode->lock_mutex);
-- 
1.7.1

[PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Pavel Shilovsky]

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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 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);
 		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);
+			goto out;
 		}
+
+		cifs_lock_add(cinode, lock);
 	} else if (unlock)
 		rc = cifs_unlock_range(cfile, flock, xid);
 
-- 
1.7.1

[PATCH 3/3] CIFS: Cleanup byte-range locking code style

[Pavel Shilovsky]

Reorder parms of cifs_lock_init, trivially simplify getlk code and
remove extra {} in cifs_lock_add_if.

Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/file.c |   43 +++++++++++++++++++------------------------
 1 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index d9cc07f..cf0b153 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -645,20 +645,20 @@ int cifs_closedir(struct inode *inode, struct file *file)
 }
 
 static struct cifsLockInfo *
-cifs_lock_init(__u64 len, __u64 offset, __u8 type, __u16 netfid)
+cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 netfid)
 {
-	struct cifsLockInfo *li =
+	struct cifsLockInfo *lock =
 		kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
-	if (!li)
-		return li;
-	li->netfid = netfid;
-	li->offset = offset;
-	li->length = len;
-	li->type = type;
-	li->pid = current->tgid;
-	INIT_LIST_HEAD(&li->blist);
-	init_waitqueue_head(&li->block_q);
-	return li;
+	if (!lock)
+		return lock;
+	lock->offset = offset;
+	lock->length = length;
+	lock->type = type;
+	lock->netfid = netfid;
+	lock->pid = current->tgid;
+	INIT_LIST_HEAD(&lock->blist);
+	init_waitqueue_head(&lock->block_q);
+	return lock;
 }
 
 static void
@@ -770,10 +770,8 @@ try_again:
 					(lock->blist.next == &lock->blist));
 		if (!rc)
 			goto try_again;
-		else {
-			mutex_lock(&cinode->lock_mutex);
-			list_del_init(&lock->blist);
-		}
+		mutex_lock(&cinode->lock_mutex);
+		list_del_init(&lock->blist);
 	}
 
 	mutex_unlock(&cinode->lock_mutex);
@@ -927,7 +925,7 @@ cifs_push_posix_locks(struct cifsFileInfo *cfile)
 		else
 			type = CIFS_WRLCK;
 
-		lck = cifs_lock_init(length, flock->fl_start, type,
+		lck = cifs_lock_init(flock->fl_start, length, type,
 				     cfile->netfid);
 		if (!lck) {
 			rc = -ENOMEM;
@@ -1064,14 +1062,12 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
 		if (rc != 0)
 			cERROR(1, "Error unlocking previously locked "
 				   "range %d during test of lock", rc);
-		rc = 0;
-		return rc;
+		return 0;
 	}
 
 	if (type & LOCKING_ANDX_SHARED_LOCK) {
 		flock->fl_type = F_WRLCK;
-		rc = 0;
-		return rc;
+		return 0;
 	}
 
 	rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
@@ -1089,8 +1085,7 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
 	} else
 		flock->fl_type = F_WRLCK;
 
-	rc = 0;
-	return rc;
+	return 0;
 }
 
 static void
@@ -1250,7 +1245,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
 	if (lock) {
 		struct cifsLockInfo *lock;
 
-		lock = cifs_lock_init(length, flock->fl_start, type, netfid);
+		lock = cifs_lock_init(flock->fl_start, length, type, netfid);
 		if (!lock)
 			return -ENOMEM;
 
-- 
1.7.1

Re: [PATCH 1/3] CIFS: Fix the VFS brlock cache usage in posix locking case

[Jeff Layton]

On Sat, 29 Oct 2011 17:17:57 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> Request to the cache in FL_POSIX case only.
> 
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/file.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 91e05f2..c1f063c 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -793,6 +793,9 @@ cifs_posix_lock_test(struct file *file, struct file_lock *flock)
>  	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>  	unsigned char saved_type = flock->fl_type;
>  
> +	if ((flock->fl_flags & FL_POSIX) == 0)
> +		return 1;
> +
>  	mutex_lock(&cinode->lock_mutex);
>  	posix_test_lock(file, flock);
>  
> @@ -809,12 +812,15 @@ static int
>  cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>  {
>  	struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
> -	int rc;
> +	int rc = 1;
> +
> +	if ((flock->fl_flags & FL_POSIX) == 0)
> +		return rc;
>  
>  	mutex_lock(&cinode->lock_mutex);
>  	if (!cinode->can_cache_brlcks) {
>  		mutex_unlock(&cinode->lock_mutex);
> -		return 1;
> +		return rc;
>  	}
>  	rc = posix_lock_file_wait(file, flock);
>  	mutex_unlock(&cinode->lock_mutex);

Hmm... I do have an open samba.org bug to allow flock() to work on cifs
as well. Would that be doable with your new locking code? In any case,
this seems ok:

Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 1/3] CIFS: Fix the VFS brlock cache usage in posix locking case

[Pavel Shilovsky]

2011/10/30 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Sat, 29 Oct 2011 17:17:57 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> Request to the cache in FL_POSIX case only.
>>
>> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>>  fs/cifs/file.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 91e05f2..c1f063c 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -793,6 +793,9 @@ cifs_posix_lock_test(struct file *file, struct file_lock *flock)
>>       struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>>       unsigned char saved_type = flock->fl_type;
>>
>> +     if ((flock->fl_flags & FL_POSIX) == 0)
>> +             return 1;
>> +
>>       mutex_lock(&cinode->lock_mutex);
>>       posix_test_lock(file, flock);
>>
>> @@ -809,12 +812,15 @@ static int
>>  cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>>  {
>>       struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>> -     int rc;
>> +     int rc = 1;
>> +
>> +     if ((flock->fl_flags & FL_POSIX) == 0)
>> +             return rc;
>>
>>       mutex_lock(&cinode->lock_mutex);
>>       if (!cinode->can_cache_brlcks) {
>>               mutex_unlock(&cinode->lock_mutex);
>> -             return 1;
>> +             return rc;
>>       }
>>       rc = posix_lock_file_wait(file, flock);
>>       mutex_unlock(&cinode->lock_mutex);
>
> Hmm... I do have an open samba.org bug to allow flock() to work on cifs
> as well. Would that be doable with your new locking code? In any case,
> this seems ok:
>
> Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Thanks for the review.

Do you mean https://bugzilla.samba.org/show_bug.cgi?id=6843 ?

I don't see any problems with implementing flock() on top of the new
locking code. We will treat it as usual brlock from 0 to offset_max -
the only difference is when we negotiate posix locking we should call
flock_lock_file_wait rather than posix_lock_file_wait (add such a
check in cifs_posix_lock_test and cifs_posix_lock_set).

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/3] CIFS: Fix the VFS brlock cache usage in posix locking case

[Steve French]

On Sun, Oct 30, 2011 at 4:26 AM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/10/30 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> On Sat, 29 Oct 2011 17:17:57 +0400
>> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>>
>>> Request to the cache in FL_POSIX case only.
>>>
>>> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>>> ---
>>>  fs/cifs/file.c |   10 ++++++++--
>>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>>> index 91e05f2..c1f063c 100644
>>> --- a/fs/cifs/file.c
>>> +++ b/fs/cifs/file.c
>>> @@ -793,6 +793,9 @@ cifs_posix_lock_test(struct file *file, struct file_lock *flock)
>>>       struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>>>       unsigned char saved_type = flock->fl_type;
>>>
>>> +     if ((flock->fl_flags & FL_POSIX) == 0)
>>> +             return 1;
>>> +
>>>       mutex_lock(&cinode->lock_mutex);
>>>       posix_test_lock(file, flock);
>>>
>>> @@ -809,12 +812,15 @@ static int
>>>  cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>>>  {
>>>       struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>>> -     int rc;
>>> +     int rc = 1;
>>> +
>>> +     if ((flock->fl_flags & FL_POSIX) == 0)
>>> +             return rc;
>>>
>>>       mutex_lock(&cinode->lock_mutex);
>>>       if (!cinode->can_cache_brlcks) {
>>>               mutex_unlock(&cinode->lock_mutex);
>>> -             return 1;
>>> +             return rc;
>>>       }
>>>       rc = posix_lock_file_wait(file, flock);
>>>       mutex_unlock(&cinode->lock_mutex);
>>
>> Hmm... I do have an open samba.org bug to allow flock() to work on cifs
>> as well. Would that be doable with your new locking code? In any case,
>> this seems ok:
>>
>> Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
> Thanks for the review.
>
> Do you mean https://bugzilla.samba.org/show_bug.cgi?id=6843 ?
>
> I don't see any problems with implementing flock() on top of the new
> locking code. We will treat it as usual brlock from 0 to offset_max -
> the only difference is when we negotiate posix locking we should call
> flock_lock_file_wait rather than posix_lock_file_wait (add such a
> check in cifs_posix_lock_test and cifs_posix_lock_set).

yes


-- 
Thanks,

Steve

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Jeff Layton]

On Sat, 29 Oct 2011 17:17:58 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  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 (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?

> +			goto out;
>  		}
> +
> +		cifs_lock_add(cinode, lock);
>  	} else if (unlock)
>  		rc = cifs_unlock_range(cfile, flock, xid);
>  


-- 
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Pavel Shilovsky]

2011/10/31 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> On Sat, 29 Oct 2011 17:17:58 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>>  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.

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Pavel Shilovsky]

2011/10/31 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> 2011/10/31 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
>> On Sat, 29 Oct 2011 17:17:58 +0400
>> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>>> ---
>>>  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.

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Jeff Layton]

On Mon, 31 Oct 2011 14:04:00 +0300
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2011/10/31 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> > 2011/10/31 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> >> On Sat, 29 Oct 2011 17:17:58 +0400
> >> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >>> ---
> >>>  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.
> 

Ok, it looks like this patch is probably ok then, but it would be nice
to have the above explanation enshrined in a comment over
cifs_lock_add_if to ensure that other people working in this code
understand it.

Can you spin up a patch to add that when you get time? For this patch
though...


Acked-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Pavel Shilovsky]

2011/11/1 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Mon, 31 Oct 2011 14:04:00 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2011/10/31 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
>> > 2011/10/31 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
>> >> On Sat, 29 Oct 2011 17:17:58 +0400
>> >> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> >>> ---
>> >>>  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.
>>
>
> Ok, it looks like this patch is probably ok then, but it would be nice
> to have the above explanation enshrined in a comment over
> cifs_lock_add_if to ensure that other people working in this code
> understand it.
>
> Can you spin up a patch to add that when you get time? For this patch
> though...

Your are right - I will do it when I get time.

>
>
> Acked-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Thank you for the review.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/3] CIFS: Cleanup byte-range locking code style

[Pavel Shilovsky]

2011/10/29 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> Reorder parms of cifs_lock_init, trivially simplify getlk code and
> remove extra {} in cifs_lock_add_if.
>
> Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/file.c |   43 +++++++++++++++++++------------------------
>  1 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index d9cc07f..cf0b153 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -645,20 +645,20 @@ int cifs_closedir(struct inode *inode, struct file *file)
>  }
>
>  static struct cifsLockInfo *
> -cifs_lock_init(__u64 len, __u64 offset, __u8 type, __u16 netfid)
> +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 netfid)
>  {
> -       struct cifsLockInfo *li =
> +       struct cifsLockInfo *lock =
>                kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
> -       if (!li)
> -               return li;
> -       li->netfid = netfid;
> -       li->offset = offset;
> -       li->length = len;
> -       li->type = type;
> -       li->pid = current->tgid;
> -       INIT_LIST_HEAD(&li->blist);
> -       init_waitqueue_head(&li->block_q);
> -       return li;
> +       if (!lock)
> +               return lock;
> +       lock->offset = offset;
> +       lock->length = length;
> +       lock->type = type;
> +       lock->netfid = netfid;
> +       lock->pid = current->tgid;
> +       INIT_LIST_HEAD(&lock->blist);
> +       init_waitqueue_head(&lock->block_q);
> +       return lock;
>  }
>
>  static void
> @@ -770,10 +770,8 @@ try_again:
>                                        (lock->blist.next == &lock->blist));
>                if (!rc)
>                        goto try_again;
> -               else {
> -                       mutex_lock(&cinode->lock_mutex);
> -                       list_del_init(&lock->blist);
> -               }
> +               mutex_lock(&cinode->lock_mutex);
> +               list_del_init(&lock->blist);
>        }
>
>        mutex_unlock(&cinode->lock_mutex);
> @@ -927,7 +925,7 @@ cifs_push_posix_locks(struct cifsFileInfo *cfile)
>                else
>                        type = CIFS_WRLCK;
>
> -               lck = cifs_lock_init(length, flock->fl_start, type,
> +               lck = cifs_lock_init(flock->fl_start, length, type,
>                                     cfile->netfid);
>                if (!lck) {
>                        rc = -ENOMEM;
> @@ -1064,14 +1062,12 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>                if (rc != 0)
>                        cERROR(1, "Error unlocking previously locked "
>                                   "range %d during test of lock", rc);
> -               rc = 0;
> -               return rc;
> +               return 0;
>        }
>
>        if (type & LOCKING_ANDX_SHARED_LOCK) {
>                flock->fl_type = F_WRLCK;
> -               rc = 0;
> -               return rc;
> +               return 0;
>        }
>
>        rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
> @@ -1089,8 +1085,7 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>        } else
>                flock->fl_type = F_WRLCK;
>
> -       rc = 0;
> -       return rc;
> +       return 0;
>  }
>
>  static void
> @@ -1250,7 +1245,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>        if (lock) {
>                struct cifsLockInfo *lock;
>
> -               lock = cifs_lock_init(length, flock->fl_start, type, netfid);
> +               lock = cifs_lock_init(flock->fl_start, length, type, netfid);
>                if (!lock)
>                        return -ENOMEM;
>
> --
> 1.7.1
>
>

Steve, Jeff,

Since #1 and #2 of this series of three are reviewed and ok could you
look at this one, please? It's rather simple, I think.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 3/3] CIFS: Cleanup byte-range locking code style

[Jeff Layton]

On Sat, 29 Oct 2011 17:17:59 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> Reorder parms of cifs_lock_init, trivially simplify getlk code and
> remove extra {} in cifs_lock_add_if.
> 
> Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/file.c |   43 +++++++++++++++++++------------------------
>  1 files changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index d9cc07f..cf0b153 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -645,20 +645,20 @@ int cifs_closedir(struct inode *inode, struct file *file)
>  }
>  
>  static struct cifsLockInfo *
> -cifs_lock_init(__u64 len, __u64 offset, __u8 type, __u16 netfid)
> +cifs_lock_init(__u64 offset, __u64 length, __u8 type, __u16 netfid)
>  {
> -	struct cifsLockInfo *li =
> +	struct cifsLockInfo *lock =
>  		kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
> -	if (!li)
> -		return li;
> -	li->netfid = netfid;
> -	li->offset = offset;
> -	li->length = len;
> -	li->type = type;
> -	li->pid = current->tgid;
> -	INIT_LIST_HEAD(&li->blist);
> -	init_waitqueue_head(&li->block_q);
> -	return li;
> +	if (!lock)
> +		return lock;
> +	lock->offset = offset;
> +	lock->length = length;
> +	lock->type = type;
> +	lock->netfid = netfid;
> +	lock->pid = current->tgid;
> +	INIT_LIST_HEAD(&lock->blist);
> +	init_waitqueue_head(&lock->block_q);
> +	return lock;
>  }
>  
>  static void
> @@ -770,10 +770,8 @@ try_again:
>  					(lock->blist.next == &lock->blist));
>  		if (!rc)
>  			goto try_again;
> -		else {
> -			mutex_lock(&cinode->lock_mutex);
> -			list_del_init(&lock->blist);
> -		}
> +		mutex_lock(&cinode->lock_mutex);
> +		list_del_init(&lock->blist);
>  	}
>  
>  	mutex_unlock(&cinode->lock_mutex);
> @@ -927,7 +925,7 @@ cifs_push_posix_locks(struct cifsFileInfo *cfile)
>  		else
>  			type = CIFS_WRLCK;
>  
> -		lck = cifs_lock_init(length, flock->fl_start, type,
> +		lck = cifs_lock_init(flock->fl_start, length, type,
>  				     cfile->netfid);
>  		if (!lck) {
>  			rc = -ENOMEM;
> @@ -1064,14 +1062,12 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>  		if (rc != 0)
>  			cERROR(1, "Error unlocking previously locked "
>  				   "range %d during test of lock", rc);
> -		rc = 0;
> -		return rc;
> +		return 0;
>  	}
>  
>  	if (type & LOCKING_ANDX_SHARED_LOCK) {
>  		flock->fl_type = F_WRLCK;
> -		rc = 0;
> -		return rc;
> +		return 0;
>  	}
>  
>  	rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
> @@ -1089,8 +1085,7 @@ cifs_getlk(struct file *file, struct file_lock *flock, __u8 type,
>  	} else
>  		flock->fl_type = F_WRLCK;
>  
> -	rc = 0;
> -	return rc;
> +	return 0;
>  }
>  
>  static void
> @@ -1250,7 +1245,7 @@ cifs_setlk(struct file *file,  struct file_lock *flock, __u8 type,
>  	if (lock) {
>  		struct cifsLockInfo *lock;
>  
> -		lock = cifs_lock_init(length, flock->fl_start, type, netfid);
> +		lock = cifs_lock_init(flock->fl_start, length, type, netfid);
>  		if (!lock)
>  			return -ENOMEM;
>  

Looks good:

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Sachin Prabhu]

Pavel,

I may have found a bug in this patch. I've commented inline.

On Sat, 2011-10-29 at 17:17 +0400, Pavel Shilovsky 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  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);
>  		if (rc < 0)
> -			return rc;
> -		else if (!rc)
> +			kfree(lock);
> +		if (rc <= 0)
>  			goto out;

The return values for cifs_lock_add_if could be
0 - If no locks exist and we can cache byte range locks
1 - If lock doesn't exist and we do not cache byte range locks.
< 0 in case of an error

In case of an error, ie. return value < 0, shouldn't we just return rc
and skip the call to posix_lock_file_wait below?

This should be 
                if (rc < 0) {
                        kfree(lock);
			return rc;
                if (!rc)
                        goto out;

Sachin Prabhu


>  
>  		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);
> +			goto out;
>  		}
> +
> +		cifs_lock_add(cinode, lock);
>  	} else if (unlock)
>  		rc = cifs_unlock_range(cfile, flock, xid);
>  

Re: [PATCH 2/3] CIFS: Simplify setlk error handling for mandatory locking

[Pavel Shilovsky]

2011/11/8 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Pavel,
>
> I may have found a bug in this patch. I've commented inline.
>
>> +             rc = cifs_lock_add_if(cinode, lock, wait_flag);
>>               if (rc < 0)
>> -                     return rc;
>> -             else if (!rc)
>> +                     kfree(lock);
>> +             if (rc <= 0)
>>                       goto out;
>
> The return values for cifs_lock_add_if could be
> 0 - If no locks exist and we can cache byte range locks
> 1 - If lock doesn't exist and we do not cache byte range locks.
> < 0 in case of an error
>
> In case of an error, ie. return value < 0, shouldn't we just return rc
> and skip the call to posix_lock_file_wait below?
>
> This should be
>                if (rc < 0) {
>                        kfree(lock);
>                        return rc;
>                if (!rc)
>                        goto out;
>

You may be right - I was thinking about it and decided not to bring
any new behavior with my lock patchset. So, this
patch revert this change (that was previously made by my lock patchset).

>
>>
>>               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);
>> +                     goto out;
>>               }

I do the similar thing here. I agree with you that this may be not what we want.

>> +
>> +             cifs_lock_add(cinode, lock);
>>       } else if (unlock)
>>               rc = cifs_unlock_range(cfile, flock, xid);
>>
>
>
>

-- 
Best regards,
Pavel Shilovsky.