[PATCH v2 0/2] xen/scsiback: correct two issues

[PATCH v2 0/2] xen/scsiback: correct two issues

[Juergen Gross]

Correct two issues in the Xen pvscsi backend.

Changes in V2:
- Patch 2: scsiback_del_translation_entry() returns error code instead of 1
    (Boris Ostrovsky)

Juergen Gross (2):
  xen/scsiback: correct frontend counting
  xen/scsiback: avoid warnings when adding multiple LUNs to a domain

 drivers/xen/xen-scsiback.c | 80 ++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

-- 
2.6.2

[PATCH v2 1/2] xen/scsiback: correct frontend counting

[Juergen Gross]

When adding a new frontend to xen-scsiback don't decrement the number
of active frontends in case of no error. Doing so results in a failure
when trying to remove the xen-pvscsi nexus even if no domain is using
it.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: stable@vger.kernel.org
---
 drivers/xen/xen-scsiback.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..51387d7 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -939,12 +939,12 @@ out:
 	spin_unlock_irqrestore(&info->v2p_lock, flags);
 
 out_free:
-	mutex_lock(&tpg->tv_tpg_mutex);
-	tpg->tv_tpg_fe_count--;
-	mutex_unlock(&tpg->tv_tpg_mutex);
-
-	if (err)
+	if (err) {
+		mutex_lock(&tpg->tv_tpg_mutex);
+		tpg->tv_tpg_fe_count--;
+		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(new);
+	}
 
 	return err;
 }
-- 
2.6.2

[PATCH v2 1/2] xen/scsiback: correct frontend counting

[Juergen Gross]

When adding a new frontend to xen-scsiback don't decrement the number
of active frontends in case of no error. Doing so results in a failure
when trying to remove the xen-pvscsi nexus even if no domain is using
it.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: stable@vger.kernel.org
---
 drivers/xen/xen-scsiback.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..51387d7 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -939,12 +939,12 @@ out:
 	spin_unlock_irqrestore(&info->v2p_lock, flags);
 
 out_free:
-	mutex_lock(&tpg->tv_tpg_mutex);
-	tpg->tv_tpg_fe_count--;
-	mutex_unlock(&tpg->tv_tpg_mutex);
-
-	if (err)
+	if (err) {
+		mutex_lock(&tpg->tv_tpg_mutex);
+		tpg->tv_tpg_fe_count--;
+		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(new);
+	}
 
 	return err;
 }
-- 
2.6.2

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

[Juergen Gross]

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 by checking for a LUN already existing when existence is
allowed (scsiback_do_add_lun() called with try == 1).

As the LUN existence check is needed now for a third time, factor it
out into a function. This in turn leads to a more or less complete
rewrite of scsiback_del_translation_entry() which will now return a
proper error code in case of failure.

Signed-off-by: Juergen Gross <jgross@suse.com>
Cc: stable@vger.kernel.org
---
v2: scsiback_del_translation_entry() returns error code instead of 1
    (Boris Ostrovsky)
---
 drivers/xen/xen-scsiback.c | 70 +++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 51387d7..c46ee18 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);
@@ -956,39 +967,40 @@ static void __scsiback_del_translation_entry(struct v2p_entry *entry)
 }
 
 /*
-  Delete the translation entry specfied
+  Delete the translation entry specified
 */
 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;
+	int ret = 0;
 
 	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);
+	else
+		ret = -ENOENT;
 
 	spin_unlock_irqrestore(&info->v2p_lock, flags);
-	return 0;
+	return ret;
 }
 
 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)) {
-- 
2.6.2

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

[Juergen Gross]

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 by checking for a LUN already existing when existence is
allowed (scsiback_do_add_lun() called with try == 1).

As the LUN existence check is needed now for a third time, factor it
out into a function. This in turn leads to a more or less complete
rewrite of scsiback_del_translation_entry() which will now return a
proper error code in case of failure.

Signed-off-by: Juergen Gross <jgross@suse.com>
Cc: stable@vger.kernel.org
---
v2: scsiback_del_translation_entry() returns error code instead of 1
    (Boris Ostrovsky)
---
 drivers/xen/xen-scsiback.c | 70 +++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 51387d7..c46ee18 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);
@@ -956,39 +967,40 @@ static void __scsiback_del_translation_entry(struct v2p_entry *entry)
 }
 
 /*
-  Delete the translation entry specfied
+  Delete the translation entry specified
 */
 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;
+	int ret = 0;
 
 	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);
+	else
+		ret = -ENOENT;
 
 	spin_unlock_irqrestore(&info->v2p_lock, flags);
-	return 0;
+	return ret;
 }
 
 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)) {
-- 
2.6.2

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

[Boris Ostrovsky]

On 02/08/2016 09:30 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 by checking for a LUN already existing when existence is
> allowed (scsiback_do_add_lun() called with try == 1).
>
> As the LUN existence check is needed now for a third time, factor it
> out into a function. This in turn leads to a more or less complete
> rewrite of scsiback_del_translation_entry() which will now return a
> proper error code in case of failure.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

[Boris Ostrovsky]

On 02/08/2016 09:30 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 by checking for a LUN already existing when existence is
> allowed (scsiback_do_add_lun() called with try == 1).
>
> As the LUN existence check is needed now for a third time, factor it
> out into a function. This in turn leads to a more or less complete
> rewrite of scsiback_del_translation_entry() which will now return a
> proper error code in case of failure.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

[David Vrabel]

On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
> allowed (scsiback_do_add_lun() called with try == 1).
> 
> As the LUN existence check is needed now for a third time, factor it
> out into a function. This in turn leads to a more or less complete
> rewrite of scsiback_del_translation_entry() which will now return a
> proper error code in case of failure.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Cc: stable@vger.kernel.org

Avoiding warnings doesn't seem like a viable candidate for stable.  Is
there more to this patch than the description suggests?

David

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

[David Vrabel]

On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
> allowed (scsiback_do_add_lun() called with try == 1).
> 
> As the LUN existence check is needed now for a third time, factor it
> out into a function. This in turn leads to a more or less complete
> rewrite of scsiback_del_translation_entry() which will now return a
> proper error code in case of failure.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Cc: stable@vger.kernel.org

Avoiding warnings doesn't seem like a viable candidate for stable.  Is
there more to this patch than the description suggests?

David

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

[Juergen Gross]

On 08/02/16 17:46, David Vrabel wrote:
> On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
>> allowed (scsiback_do_add_lun() called with try == 1).
>>
>> As the LUN existence check is needed now for a third time, factor it
>> out into a function. This in turn leads to a more or less complete
>> rewrite of scsiback_del_translation_entry() which will now return a
>> proper error code in case of failure.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Cc: stable@vger.kernel.org
> 
> Avoiding warnings doesn't seem like a viable candidate for stable.  Is
> there more to this patch than the description suggests?

No.

Should I resend without the Cc: stable?


Juergen

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

[Juergen Gross]

On 08/02/16 17:46, David Vrabel wrote:
> On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
>> allowed (scsiback_do_add_lun() called with try == 1).
>>
>> As the LUN existence check is needed now for a third time, factor it
>> out into a function. This in turn leads to a more or less complete
>> rewrite of scsiback_del_translation_entry() which will now return a
>> proper error code in case of failure.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Cc: stable@vger.kernel.org
> 
> Avoiding warnings doesn't seem like a viable candidate for stable.  Is
> there more to this patch than the description suggests?

No.

Should I resend without the Cc: stable?


Juergen

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

[David Vrabel]

On 08/02/16 16:50, Juergen Gross wrote:
> On 08/02/16 17:46, David Vrabel wrote:
>> On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
>>> allowed (scsiback_do_add_lun() called with try == 1).
>>>
>>> As the LUN existence check is needed now for a third time, factor it
>>> out into a function. This in turn leads to a more or less complete
>>> rewrite of scsiback_del_translation_entry() which will now return a
>>> proper error code in case of failure.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> Cc: stable@vger.kernel.org
>>
>> Avoiding warnings doesn't seem like a viable candidate for stable.  Is
>> there more to this patch than the description suggests?
> 
> No.
> 
> Should I resend without the Cc: stable?

No, I've dropped it.

I've applied these two to for-linus-4.5, thanks.

David

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

[David Vrabel]

On 08/02/16 16:50, Juergen Gross wrote:
> On 08/02/16 17:46, David Vrabel wrote:
>> On 08/02/16 14:30, 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 by checking for a LUN already existing when existence is
>>> allowed (scsiback_do_add_lun() called with try == 1).
>>>
>>> As the LUN existence check is needed now for a third time, factor it
>>> out into a function. This in turn leads to a more or less complete
>>> rewrite of scsiback_del_translation_entry() which will now return a
>>> proper error code in case of failure.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> Cc: stable@vger.kernel.org
>>
>> Avoiding warnings doesn't seem like a viable candidate for stable.  Is
>> there more to this patch than the description suggests?
> 
> No.
> 
> Should I resend without the Cc: stable?

No, I've dropped it.

I've applied these two to for-linus-4.5, thanks.

David