Re: [Xen-devel] [PATCH 2/2] xen/scsiback: avoid warnings when adding multiple LUNs to a domain

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/05/2016 08:21 AM, Juergen Gross wrote:
> When adding more than one LUN to a frontend a warning for a failed
> assignment is issued in dom0 for each already existing LUN. Avoid this
> warning.
Aren't you just factoring out the check? The warning is still printed 
for each scsiback_add_translation_entry() invocation, no?

> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>   drivers/xen/xen-scsiback.c | 65 ++++++++++++++++++++++++++--------------------
>   1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 51387d7..69de879 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -849,15 +849,31 @@ static int scsiback_map(struct vscsibk_info *info)
>   }
>   
>   /*
> +  Check for a translation entry being present
> +*/
> +static struct v2p_entry *scsiback_chk_translation_entry(
> +       struct vscsibk_info *info, struct ids_tuple *v)
> +{
> +       struct list_head *head = &(info->v2p_entry_lists);
> +       struct v2p_entry *entry;
> +
> +       list_for_each_entry(entry, head, l)
> +               if ((entry->v.chn == v->chn) &&
> +                   (entry->v.tgt == v->tgt) &&
> +                   (entry->v.lun == v->lun))
> +                       return entry;
> +
> +       return NULL;
> +}
> +
> +/*
>     Add a new translation entry
>   */
>   static int scsiback_add_translation_entry(struct vscsibk_info *info,
>                                           char *phy, struct ids_tuple *v)
>   {
>         int err = 0;
> -       struct v2p_entry *entry;
>         struct v2p_entry *new;
> -       struct list_head *head = &(info->v2p_entry_lists);
>         unsigned long flags;
>         char *lunp;
>         unsigned long long unpacked_lun;
> @@ -917,15 +933,10 @@ static int scsiback_add_translation_entry(struct 
> vscsibk_info *info,
>         spin_lock_irqsave(&info->v2p_lock, flags);
>   
>   	/* Check double assignment to identical virtual ID */
> -       list_for_each_entry(entry, head, l) {
> -               if ((entry->v.chn == v->chn) &&
> -                   (entry->v.tgt == v->tgt) &&
> -                   (entry->v.lun == v->lun)) {
> -                       pr_warn("Virtual ID is already used. Assignment was not 
> performed.\n");
> -                       err = -EEXIST;
> -                       goto out;
> -               }
> -
> +       if (scsiback_chk_translation_entry(info, v)) {
> +               pr_warn("Virtual ID is already used. Assignment was not 
> performed.\n");
> +               err = -EEXIST;
> +               goto out;
>         }
>   
>   	/* Create a new translation entry and add to the list */
> @@ -933,7 +944,7 @@ static int scsiback_add_translation_entry(struct 
> vscsibk_info *info,
>         new->v = *v;
>         new->tpg = tpg;
>         new->lun = unpacked_lun;
> -       list_add_tail(&new->l, head);
> +       list_add_tail(&new->l, &info->v2p_entry_lists);
>   
>   out:
>         spin_unlock_irqrestore(&info->v2p_lock, flags);
> @@ -962,33 +973,31 @@ static int scsiback_del_translation_entry(struct 
> vscsibk_info *info,
>                                           struct ids_tuple *v)
>   {
>         struct v2p_entry *entry;
> -       struct list_head *head = &(info->v2p_entry_lists);
>         unsigned long flags;
>   
>   	spin_lock_irqsave(&info->v2p_lock, flags);
>         /* Find out the translation entry specified */
> -       list_for_each_entry(entry, head, l) {
> -               if ((entry->v.chn == v->chn) &&
> -                   (entry->v.tgt == v->tgt) &&
> -                   (entry->v.lun == v->lun)) {
> -                       goto found;
> -               }
> -       }
> -
> -       spin_unlock_irqrestore(&info->v2p_lock, flags);
> -       return 1;
> -
> -found:
> -       /* Delete the translation entry specfied */
> -       __scsiback_del_translation_entry(entry);
> +       entry = scsiback_chk_translation_entry(info, v);
> +       if (entry)
> +               __scsiback_del_translation_entry(entry);
>   
>   	spin_unlock_irqrestore(&info->v2p_lock, flags);
> -       return 0;
> +       return entry == NULL;
Might be better to return -ENOENT instead of 1 above and -EEXISTS if 
entry!=NULL, given that this returns an int.
-boris

>   }
>   
>   static void scsiback_do_add_lun(struct vscsibk_info *info, const char *state,
>                                 char *phy, struct ids_tuple *vir, int try)
>   {
> +       struct v2p_entry *entry;
> +       unsigned long flags;
> +
> +       if (try) {
> +               spin_lock_irqsave(&info->v2p_lock, flags);
> +               entry = scsiback_chk_translation_entry(info, vir);
> +               spin_unlock_irqrestore(&info->v2p_lock, flags);
> +               if (entry)
> +                       return;
> +       }
>         if (!scsiback_add_translation_entry(info, phy, vir)) {
>                 if (xenbus_printf(XBT_NIL, info->dev->nodename, state,
>                                   "%d", XenbusStateInitialised)) {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel