Message-ID: <87lgnlxcug.fsf@notabene.neil.brown.name>
Date:   Wed, 19 Jul 2017 14:36:07 +1000
From:   NeilBrown <neilb@...e.com>
To:     Patrick Farrell <paf@...y.com>,
        Oleg Drokin <oleg.drokin@...el.com>,
        Greg Kroah-Hartman <greg@...ah.com>,
        Andreas Dilger <andreas.dilger@...el.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lustre Development List <lustre-devel@...ts.lustre.org>
Subject: Re: [lustre-devel] [PATCH 10/12] staging: lustre: ldlm: tidy list walking in ldlm_flock()

On Wed, Jul 19 2017, Patrick Farrell wrote:

> Neil,
>
> Minor...
> "order might not be a lock" looks like it should say "or"?

Yes: s/order/or/ as you say.

Thanks,
NeilBrown


>
> - Patrick
> ________________________________
> From: lustre-devel <lustre-devel-bounces@...ts.lustre.org> on behalf of NeilBrown <neilb@...e.com>
> Sent: Tuesday, July 18, 2017 6:26:47 PM
> To: Oleg Drokin; Greg Kroah-Hartman; Andreas Dilger
> Cc: Linux Kernel Mailing List; Lustre Development List
> Subject: [lustre-devel] [PATCH 10/12] staging: lustre: ldlm: tidy list walking in ldlm_flock()
>
> 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@...e.com>
> ---
>  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 9a888e1ce923..58227728a002 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, order 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
>
>
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@...ts.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists