[PATCH 1/4] leases: fix a return-value mixup

[PATCH 1/4] leases: fix a return-value mixup

[David M. Richter]

Fixes a return-value mixup from 85c59580b30c82aa771aa33b37217a6b6851bc14
"locks: Fix potential OOPS in generic_setlease()", in which -ENOMEM replaced
what had been intended to stay -EAGAIN in the variable "error".

Signed-off-by: David M. Richter <richterd@citi.umich.edu>
---
 fs/locks.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 592faad..b9f3a0b 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1404,6 +1404,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 			rdlease_count++;
 	}
 
+	error = -EAGAIN;
 	if ((arg == F_RDLCK && (wrlease_count > 0)) ||
 	    (arg == F_WRLCK && ((rdlease_count + wrlease_count) > 0)))
 		goto out;
-- 
1.5.4

[PATCH 2/4] leases: when unlocking, skip locking-related steps

[David M. Richter]

In generic_setlease(), we don't need to allocate a new struct file_lock
or check for readers or writers when called with F_UNLCK.

Signed-off-by: David M. Richter <richterd@citi.umich.edu>
---
 fs/locks.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index b9f3a0b..da1d0dd 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1367,18 +1367,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 
 	lease = *flp;
 
-	error = -EAGAIN;
-	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
-		goto out;
-	if ((arg == F_WRLCK)
-	    && ((atomic_read(&dentry->d_count) > 1)
-		|| (atomic_read(&inode->i_count) > 1)))
-		goto out;
+	if (arg != F_UNLCK) {
+		error = -EAGAIN;
+		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
+			goto out;
+		if ((arg == F_WRLCK)
+		    && ((atomic_read(&dentry->d_count) > 1)
+			|| (atomic_read(&inode->i_count) > 1)))
+			goto out;
 
-	error = -ENOMEM;
-	new_fl = locks_alloc_lock();
-	if (new_fl == NULL)
-		goto out;
+		error = -ENOMEM;
+		new_fl = locks_alloc_lock();
+		if (new_fl == NULL)
+			goto out;
+	}
 
 	/*
 	 * At this point, we know that if there is an exclusive
-- 
1.5.4

[PATCH 3/4] leases: move lock allocation earlier in generic_setlease()

[David M. Richter]

In generic_setlease(), the struct file_lock is allocated after tests for the
presence of conflicting readers/writers is done, despite the fact that the
allocation might block; this patch moves the allocation earlier.  A subsequent
set of patches will rely on this behavior to properly serialize between a
modified __break_lease() and generic_setlease().

Signed-off-by: David M. Richter <richterd@citi.umich.edu>
---
 fs/locks.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index da1d0dd..6a132cd 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1368,6 +1368,11 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 	lease = *flp;
 
 	if (arg != F_UNLCK) {
+		error = -ENOMEM;
+		new_fl = locks_alloc_lock();
+		if (new_fl == NULL)
+			goto out;
+
 		error = -EAGAIN;
 		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
 			goto out;
@@ -1375,11 +1380,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 		    && ((atomic_read(&dentry->d_count) > 1)
 			|| (atomic_read(&inode->i_count) > 1)))
 			goto out;
-
-		error = -ENOMEM;
-		new_fl = locks_alloc_lock();
-		if (new_fl == NULL)
-			goto out;
 	}
 
 	/*
-- 
1.5.4

[PATCH 4/4] leases: remove unneeded variable from fcntl_setlease().

[David M. Richter]

fcntl_setlease() has a struct dentry* that is used only once; this patch
removes it.

Signed-off-by: David M. Richter <richterd@citi.umich.edu>
---
 fs/locks.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 6a132cd..2e0fa66 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1493,8 +1493,7 @@ EXPORT_SYMBOL_GPL(vfs_setlease);
 int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
 {
 	struct file_lock fl, *flp = &fl;
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = filp->f_path.dentry->d_inode;
 	int error;
 
 	locks_init_lock(&fl);
-- 
1.5.4

Re: [PATCH 4/4] leases: remove unneeded variable from fcntl_setlease().

[J. Bruce Fields]

On Wed, Apr 23, 2008 at 04:29:02PM -0400, David M. Richter wrote:
> fcntl_setlease() has a struct dentry* that is used only once; this patch
> removes it.

Thanks, I've applied all four.

It seems to me that generic_setlease() has a lot of special handling for
the unlock case.  I wonder if it'd work out to be simpler split out as a
separate function calling helper functions for common code.

--b.

> 
> Signed-off-by: David M. Richter <richterd@citi.umich.edu>
> ---
>  fs/locks.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 6a132cd..2e0fa66 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1493,8 +1493,7 @@ EXPORT_SYMBOL_GPL(vfs_setlease);
>  int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
>  {
>  	struct file_lock fl, *flp = &fl;
> -	struct dentry *dentry = filp->f_path.dentry;
> -	struct inode *inode = dentry->d_inode;
> +	struct inode *inode = filp->f_path.dentry->d_inode;
>  	int error;
>  
>  	locks_init_lock(&fl);
> -- 
> 1.5.4
> 

Re: [PATCH 2/4] leases: when unlocking, skip locking-related steps

[Chuck Lever]

Hi David-

On Apr 23, 2008, at 4:29 PM, David M. Richter wrote:
> In generic_setlease(), we don't need to allocate a new struct  
> file_lock
> or check for readers or writers when called with F_UNLCK.

Since you re-ordered the locks_alloc_lock call in the next patch, it  
seems to me this would look a lot cleaner with a switch statement  
instead of all these "if (arg == F_FOO)".

> Signed-off-by: David M. Richter <richterd@citi.umich.edu>
> ---
> fs/locks.c |   24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index b9f3a0b..da1d0dd 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1367,18 +1367,20 @@ int generic_setlease(struct file *filp, long  
> arg, struct file_lock **flp)
>
> 	lease = *flp;
>
> -	error = -EAGAIN;
> -	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> -		goto out;
> -	if ((arg == F_WRLCK)
> -	    && ((atomic_read(&dentry->d_count) > 1)
> -		|| (atomic_read(&inode->i_count) > 1)))
> -		goto out;
> +	if (arg != F_UNLCK) {
> +		error = -EAGAIN;
> +		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> +			goto out;
> +		if ((arg == F_WRLCK)
> +		    && ((atomic_read(&dentry->d_count) > 1)
> +			|| (atomic_read(&inode->i_count) > 1)))
> +			goto out;
>
> -	error = -ENOMEM;
> -	new_fl = locks_alloc_lock();
> -	if (new_fl == NULL)
> -		goto out;
> +		error = -ENOMEM;
> +		new_fl = locks_alloc_lock();
> +		if (new_fl == NULL)
> +			goto out;
> +	}
>
> 	/*
> 	 * At this point, we know that if there is an exclusive
> -- 
> 1.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux- 
> fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/4] leases: when unlocking, skip locking-related steps

[J. Bruce Fields]

On Thu, Apr 24, 2008 at 10:32:39AM -0400, Chuck Lever wrote:
> Hi David-
>
> On Apr 23, 2008, at 4:29 PM, David M. Richter wrote:
>> In generic_setlease(), we don't need to allocate a new struct  
>> file_lock
>> or check for readers or writers when called with F_UNLCK.
>
> Since you re-ordered the locks_alloc_lock call in the next patch, it  
> seems to me this would look a lot cleaner with a switch statement  
> instead of all these "if (arg == F_FOO)".

This could certainly be cleaner, but I took a quick look and didn't see
how to do what you were suggesting--probably just me being dense.

David's changes seem reasonable enough on their own, so I'd take any
cleanup as an incremental patch on top.

--b.

>
>> Signed-off-by: David M. Richter <richterd@citi.umich.edu>
>> ---
>> fs/locks.c |   24 +++++++++++++-----------
>> 1 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index b9f3a0b..da1d0dd 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -1367,18 +1367,20 @@ int generic_setlease(struct file *filp, long  
>> arg, struct file_lock **flp)
>>
>> 	lease = *flp;
>>
>> -	error = -EAGAIN;
>> -	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
>> -		goto out;
>> -	if ((arg == F_WRLCK)
>> -	    && ((atomic_read(&dentry->d_count) > 1)
>> -		|| (atomic_read(&inode->i_count) > 1)))
>> -		goto out;
>> +	if (arg != F_UNLCK) {
>> +		error = -EAGAIN;
>> +		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
>> +			goto out;
>> +		if ((arg == F_WRLCK)
>> +		    && ((atomic_read(&dentry->d_count) > 1)
>> +			|| (atomic_read(&inode->i_count) > 1)))
>> +			goto out;
>>
>> -	error = -ENOMEM;
>> -	new_fl = locks_alloc_lock();
>> -	if (new_fl == NULL)
>> -		goto out;
>> +		error = -ENOMEM;
>> +		new_fl = locks_alloc_lock();
>> +		if (new_fl == NULL)
>> +			goto out;
>> +	}
>>
>> 	/*
>> 	 * At this point, we know that if there is an exclusive
>> -- 
>> 1.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>