Re: [PATCH 7/9] staging: lustre: ldlm: tidy list walking in ldlm_flock()

[Dilger, Andreas]

On Oct 22, 2017, at 18:53, NeilBrown <neilb@xxxxxxxx> wrote:
> 
> Use list_for_each_entry variants to
> avoid the explicit list_entry() calls.
> This allows us to use list_for_each_entry_safe_from()
> instread of adding a local list-walking macro.
> 
> Also improve some comments so that it is more obvious
> that the locks are sorted per-owner and that we need
> to find the insertion point.
> 
> Signed-off-by: NeilBrown <neilb@xxxxxxxx>

The conversion looks a bit tricky, but appears to be correct.

Reviewed-by: Andreas Dilger <andreas.dilger@xxxxxxxxx>

> ---
> drivers/staging/lustre/lustre/ldlm/ldlm_flock.c |   45 ++++++++++-------------
> 1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> index 1bf56892fcf5..0bf6dce1c5b1 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> @@ -59,17 +59,6 @@
> #include <linux/list.h>
> #include "ldlm_internal.h"
> 
> -/**
> - * list_for_remaining_safe - iterate over the remaining entries in a list
> - *	      and safeguard against removal of a list entry.
> - * \param pos   the &struct list_head to use as a loop counter. pos MUST
> - *	      have been initialized prior to using it in this macro.
> - * \param n     another &struct list_head to use as temporary storage
> - * \param head  the head for your list.
> - */
> -#define list_for_remaining_safe(pos, n, head) \
> -	for (n = pos->next; pos != (head); pos = n, n = pos->next)
> -
> static inline int
> ldlm_same_flock_owner(struct ldlm_lock *lock, struct ldlm_lock *new)
> {
> @@ -125,8 +114,8 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
> {
> 	struct ldlm_resource *res = req->l_resource;
> 	struct ldlm_namespace *ns = ldlm_res_to_ns(res);
> -	struct list_head *tmp;
> -	struct list_head *ownlocks = NULL;
> +	struct ldlm_lock *tmp;
> +	struct ldlm_lock *ownlocks = NULL;
> 	struct ldlm_lock *lock = NULL;
> 	struct ldlm_lock *new = req;
> 	struct ldlm_lock *new2 = NULL;
> @@ -151,23 +140,23 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
> 	/* This loop determines where this processes locks start
> 	 * in the resource lr_granted list.
> 	 */
> -	list_for_each(tmp, &res->lr_granted) {
> -		lock = list_entry(tmp, struct ldlm_lock,
> -				  l_res_link);
> +	list_for_each_entry(lock, &res->lr_granted, l_res_link) {
> 		if (ldlm_same_flock_owner(lock, req)) {
> -			ownlocks = tmp;
> +			ownlocks = lock;
> 			break;
> 		}
> 	}
> 
> -	/* Scan the locks owned by this process that overlap this request.
> +	/* Scan the locks owned by this process to find the insertion point
> +	 * (as locks are ordered), and to handle overlaps.
> 	 * We may have to merge or split existing locks.
> 	 */
> -	if (!ownlocks)
> -		ownlocks = &res->lr_granted;
> -
> -	list_for_remaining_safe(ownlocks, tmp, &res->lr_granted) {
> -		lock = list_entry(ownlocks, struct ldlm_lock, l_res_link);
> +	if (ownlocks)
> +		lock = ownlocks;
> +	else
> +		lock = list_entry(&res->lr_granted,
> +				  struct ldlm_lock, l_res_link);
> +	list_for_each_entry_safe_from(lock, tmp, &res->lr_granted, l_res_link) {
> 
> 		if (!ldlm_same_flock_owner(lock, new))
> 			break;
> @@ -295,7 +284,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
> 						 lock->l_granted_mode);
> 
> 		/* insert new2 at lock */
> -		ldlm_resource_add_lock(res, ownlocks, new2);
> +		ldlm_resource_add_lock(res, &lock->l_res_link, new2);
> 		LDLM_LOCK_RELEASE(new2);
> 		break;
> 	}
> @@ -309,8 +298,12 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req)
> 
> 	if (!added) {
> 		list_del_init(&req->l_res_link);
> -		/* insert new lock before ownlocks in list. */
> -		ldlm_resource_add_lock(res, ownlocks, req);
> +		/* insert new lock before "lock", which might be the
> +		 * next lock for this owner, or might be the first
> +		 * lock for the next owner, or might not be a lock at
> +		 * all, but instead points at the head of the list
> +		 */
> +		ldlm_resource_add_lock(res, &lock->l_res_link, req);
> 	}
> 
> 	/* In case we're reprocessing the requested lock we can't destroy
> 
> 

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation