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

[Juergen Gross]

On 05/02/16 16:50, Boris Ostrovsky wrote:
> 
> 
> 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?

I don't call scsiback_add_translation_entry() in the critical case.

> 
> 
>>
>> 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.

I just didn't want to change more than necessary. In case it is
okay to do some cleanup as well I'd rather change the return type
to "bool".


Juergen

> 
> -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)) {
> 
>