Re: [RFC v2 3/4] locks: Split insert/delete block functions into flock/posix parts

[Jeff Layton]

On Mon,  2 Mar 2015 15:25:12 +0100
Daniel Wagner <daniel.wagner@xxxxxxxxxxxx> wrote:

> The locks_insert/delete_block() functions are used for flock, posix
> and leases types. blocked_lock_lock is used to serialize all access to
> fl_link, fl_block, fl_next and blocked_hash. Here, we prepare the
> stage for using blocked_lock_lock to protect blocked_hash.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> Cc: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> Cc: "J. Bruce Fields" <bfields@xxxxxxxxxxxx>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
>  fs/locks.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 4498da0..02821dd 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -611,11 +611,20 @@ static void locks_delete_global_blocked(struct file_lock *waiter)
>   */
>  static void __locks_delete_block(struct file_lock *waiter)
>  {
> -	locks_delete_global_blocked(waiter);
>  	list_del_init(&waiter->fl_block);
>  	waiter->fl_next = NULL;
>  }
>  
> +/* Posix block variant of __locks_delete_block.
> + *
> + * Must be called with blocked_lock_lock held.
> + */
> +static void __locks_delete_posix_block(struct file_lock *waiter)
> +{
> +	locks_delete_global_blocked(waiter);
> +	__locks_delete_block(waiter);
> +}
> +
>  static void locks_delete_block(struct file_lock *waiter)
>  {
>  	spin_lock(&blocked_lock_lock);
> @@ -623,6 +632,13 @@ static void locks_delete_block(struct file_lock *waiter)
>  	spin_unlock(&blocked_lock_lock);
>  }
>  
> +static void locks_delete_posix_block(struct file_lock *waiter)
> +{
> +	spin_lock(&blocked_lock_lock);
> +	__locks_delete_posix_block(waiter);
> +	spin_unlock(&blocked_lock_lock);
> +}
> +
>  /* Insert waiter into blocker's block list.
>   * We use a circular list so that processes can be easily woken up in
>   * the order they blocked. The documentation doesn't require this but
> @@ -639,7 +655,17 @@ static void __locks_insert_block(struct file_lock *blocker,
>  	BUG_ON(!list_empty(&waiter->fl_block));
>  	waiter->fl_next = blocker;
>  	list_add_tail(&waiter->fl_block, &blocker->fl_block);
> -	if (IS_POSIX(blocker) && !IS_OFDLCK(blocker))
> +}
> +
> +/* Posix block variant of __locks_insert_block.
> + *
> + * Must be called with flc_lock and blocked_lock_lock held.
> + */
> +static void __locks_insert_posix_block(struct file_lock *blocker,
> +					struct file_lock *waiter)
> +{
> +	__locks_insert_block(blocker, waiter);
> +	if (!IS_OFDLCK(blocker))
>  		locks_insert_global_blocked(waiter);
>  }
>

In many ways OFD locks act more like flock locks than POSIX ones. In
particular, there is no deadlock detection there, so once your
conversion is done to more widely use the percpu locks, then you should
be able to avoid taking the blocked_lock_lock for OFD locks as well.
The 4th patch in this series doesn't currently do that.

You may want to revisit this patch such that the IS_OFDLCK checks are
done earlier, so that you can avoid taking the blocked_lock_lock if
IS_POSIX and !IS_OFDLCK.

> @@ -675,7 +701,10 @@ static void locks_wake_up_blocks(struct
> file_lock *blocker) 
>  		waiter = list_first_entry(&blocker->fl_block,
>  				struct file_lock, fl_block);
> -		__locks_delete_block(waiter);
> +		if (IS_POSIX(blocker))

...again, you probably want to make this read:

		if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)

...so that later you can avoid taking the blocked_lock_lock if IS_OFDLOCK(blocker).


> +			__locks_delete_posix_block(waiter);
> +		else
> +			__locks_delete_block(waiter);
>  		if (waiter->fl_lmops && waiter->fl_lmops->lm_notify)
>  			waiter->fl_lmops->lm_notify(waiter);
>  		else
> @@ -985,7 +1014,7 @@ static int __posix_lock_file(struct inode
> *inode, struct file_lock *request, str spin_lock(&blocked_lock_lock);
>  			if (likely(!posix_locks_deadlock(request,
> fl))) { error = FILE_LOCK_DEFERRED;
> -				__locks_insert_block(fl, request);
> +				__locks_insert_posix_block(fl,
> request); }
>  			spin_unlock(&blocked_lock_lock);
>  			goto out;
> @@ -1186,7 +1215,7 @@ int posix_lock_file_wait(struct file *filp,
> struct file_lock *fl) if (!error)
>  			continue;
>  
> -		locks_delete_block(fl);
> +		locks_delete_posix_block(fl);
>  		break;
>  	}
>  	return error;
> @@ -1283,7 +1312,7 @@ int locks_mandatory_area(int read_write, struct
> inode *inode, continue;
>  		}
>  
> -		locks_delete_block(&fl);
> +		locks_delete_posix_block(&fl);
>  		break;
>  	}
>  
> @@ -2103,7 +2132,10 @@ static int do_lock_file_wait(struct file
> *filp, unsigned int cmd, if (!error)
>  			continue;
>  
> -		locks_delete_block(fl);
> +		if (IS_POSIX(fl))
> +			locks_delete_posix_block(fl);
> +		else
> +			locks_delete_block(fl);
>  		break;
>  	}
>  
> @@ -2467,7 +2499,7 @@ posix_unblock_lock(struct file_lock *waiter)
>  
>  	spin_lock(&blocked_lock_lock);
>  	if (waiter->fl_next)
> -		__locks_delete_block(waiter);
> +		__locks_delete_posix_block(waiter);
>  	else
>  		status = -ENOENT;
>  	spin_unlock(&blocked_lock_lock);

-- 
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/