[PATCH] xen-scsifront: Add a missing call to kfree

[PATCH] xen-scsifront: Add a missing call to kfree

[Quentin Lambert]

Most error branches following the call to kmalloc contain
a call to kfree. This patch add these calls where they are
missing.

This issue was found with Hector.

Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>

---
 drivers/scsi/xen-scsifront.c |    1 +
 1 file changed, 1 insertion(+)

--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
 
 	if (scsifront_enter(info)) {
 		spin_unlock_irq(host->host_lock);
+		kfree(shadow);
 		return FAILED;
 	}
 

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 19/11/16 19:22, Quentin Lambert wrote:
> Most error branches following the call to kmalloc contain
> a call to kfree. This patch add these calls where they are
> missing.
> 
> This issue was found with Hector.
> 
> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>

Nice catch. I think this will need some more work, I'll do a
follow-on patch.

Reviewed-by: Juergen Gross <jgross@suse.com>

> 
> ---
>  drivers/scsi/xen-scsifront.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
>  
>  	if (scsifront_enter(info)) {
>  		spin_unlock_irq(host->host_lock);
> +		kfree(shadow);
>  		return FAILED;
>  	}
>  
> 

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Martin K. Petersen]

>>>>> "Juergen" == Juergen Gross <jgross@suse.com> writes:

Juergen,

Juergen> On 19/11/16 19:22, Quentin Lambert wrote:
>> Most error branches following the call to kmalloc contain a call to
>> kfree. This patch add these calls where they are missing.
>> 
>> This issue was found with Hector.
>> 
>> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>

Juergen> Nice catch. I think this will need some more work, I'll do a
Juergen> follow-on patch.

Juergen> Reviewed-by: Juergen Gross <jgross@suse.com>

Are you taking this patch through the Xen tree or should I queue it up?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 22/11/16 04:40, Martin K. Petersen wrote:
>>>>>> "Juergen" == Juergen Gross <jgross@suse.com> writes:
> 
> Juergen,
> 
> Juergen> On 19/11/16 19:22, Quentin Lambert wrote:
>>> Most error branches following the call to kmalloc contain a call to
>>> kfree. This patch add these calls where they are missing.
>>>
>>> This issue was found with Hector.
>>>
>>> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
> 
> Juergen> Nice catch. I think this will need some more work, I'll do a
> Juergen> follow-on patch.
> 
> Juergen> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Are you taking this patch through the Xen tree or should I queue it up?

I'm taking it through the xen tree, thanks.


Juergen

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Dan Carpenter]

On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote:
> On 19/11/16 19:22, Quentin Lambert wrote:
> > Most error branches following the call to kmalloc contain
> > a call to kfree. This patch add these calls where they are
> > missing.
> > 
> > This issue was found with Hector.
> > 
> > Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
> 
> Nice catch. I think this will need some more work, I'll do a
> follow-on patch.

Yeah.  It's weird how we free it on the success path and all the failure
paths except one.  But it looks so deliberate.  What's going on with
that?

Could you send your follow on patch as a reply to the thread?

regards,
dan carpenter

[PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

When allocating a new ring slot for a request don't advance the
producer index until the request is really ready to go to the backend.
Otherwise only partially filled requests will be visible to the
backend in case of errors on the frontend side.

In scsifront_action_handler() free the request id in case of an error.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
In case accepted I'll take this patch through the Xen tree as it is based
on another patch by lambert.quentin@gmail.com which is going through Xen, too.
---
 drivers/scsi/xen-scsifront.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..33805f6 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
 	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
 
-	ring->req_prod_pvt++;
-
 	ring_req->rqid = (uint16_t)id;
 
 	return ring_req;
@@ -196,6 +194,8 @@ static void scsifront_do_request(struct vscsifrnt_info *info)
 	struct vscsiif_front_ring *ring = &(info->ring);
 	int notify;
 
+	ring->req_prod_pvt++;
+
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
 	if (notify)
 		notify_remote_via_irq(info->irq);
@@ -626,6 +626,7 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	}
 
 	if (scsifront_enter(info)) {
+		scsifront_put_rqid(info, shadow->rqid);
 		spin_unlock_irq(host->host_lock);
 		kfree(shadow);
 		return FAILED;
-- 
2.10.2

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Jan Beulich]

>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>  
>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>  
> -	ring->req_prod_pvt++;

Please note the "_pvt" suffix, which stands for "private": This field is
not visible to the backend. Only ring->sring fields are shared, and
the updating of the shared field happens in RING_PUSH_REQUESTS()
and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().

Jan

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:14, Jan Beulich wrote:
>>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
>> --- a/drivers/scsi/xen-scsifront.c
>> +++ b/drivers/scsi/xen-scsifront.c
>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>>  
>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>  
>> -	ring->req_prod_pvt++;
> 
> Please note the "_pvt" suffix, which stands for "private": This field is
> not visible to the backend. Only ring->sring fields are shared, and
> the updating of the shared field happens in RING_PUSH_REQUESTS()
> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().

Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
the case corrected this would advance req_prod by two after the error
case before, even if only one request would have made it to the ring.

As an alternative I could have decremented req_prod_pvt in case of an
error, but I like my current solution better.


Juergen

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[David Vrabel]

On 29/11/16 11:19, Juergen Gross wrote:
> On 29/11/16 12:14, Jan Beulich wrote:
>>>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
>>> --- a/drivers/scsi/xen-scsifront.c
>>> +++ b/drivers/scsi/xen-scsifront.c
>>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>>>  
>>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>>  
>>> -	ring->req_prod_pvt++;
>>
>> Please note the "_pvt" suffix, which stands for "private": This field is
>> not visible to the backend. Only ring->sring fields are shared, and
>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
> 
> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
> the case corrected this would advance req_prod by two after the error
> case before, even if only one request would have made it to the ring.
> 
> As an alternative I could have decremented req_prod_pvt in case of an
> error, but I like my current solution better.

FWIW, I found the commit message a bit misleading and also came to the
same conclusion as Jan initially.

Perhaps,

"When adding a new request to the ring, an error may cause the
(partially constructed) request to be discarded and used for the next.
Thus ring->req_prod_pvt should not be advanced until we know the request
will be successfully added to the ring."

Or similar.

David

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:28, David Vrabel wrote:
> On 29/11/16 11:19, Juergen Gross wrote:
>> On 29/11/16 12:14, Jan Beulich wrote:
>>>>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
>>>> --- a/drivers/scsi/xen-scsifront.c
>>>> +++ b/drivers/scsi/xen-scsifront.c
>>>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>>>>  
>>>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>>>  
>>>> -	ring->req_prod_pvt++;
>>>
>>> Please note the "_pvt" suffix, which stands for "private": This field is
>>> not visible to the backend. Only ring->sring fields are shared, and
>>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
>>
>> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
>> the case corrected this would advance req_prod by two after the error
>> case before, even if only one request would have made it to the ring.
>>
>> As an alternative I could have decremented req_prod_pvt in case of an
>> error, but I like my current solution better.
> 
> FWIW, I found the commit message a bit misleading and also came to the
> same conclusion as Jan initially.
> 
> Perhaps,
> 
> "When adding a new request to the ring, an error may cause the
> (partially constructed) request to be discarded and used for the next.
> Thus ring->req_prod_pvt should not be advanced until we know the request
> will be successfully added to the ring."

This is indeed much better, thanks.

In case there are no other objections I'll fix this up when
committing.


Juergen

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Jan Beulich]

>>> On 29.11.16 at 12:19, <JGross@suse.com> wrote:
> On 29/11/16 12:14, Jan Beulich wrote:
>>>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
>>> --- a/drivers/scsi/xen-scsifront.c
>>> +++ b/drivers/scsi/xen-scsifront.c
>>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 
> vscsifrnt_info *info)
>>>  
>>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>>  
>>> -	ring->req_prod_pvt++;
>> 
>> Please note the "_pvt" suffix, which stands for "private": This field is
>> not visible to the backend. Only ring->sring fields are shared, and
>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
> 
> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
> the case corrected this would advance req_prod by two after the error
> case before, even if only one request would have made it to the ring.

Okay, then I may have been mislead by the patch description: I
understood it to say that you want to avoid the backend seeing
requests which haven't been filled fully, but it looks like you're
instead saying that for these requests the filling will never be
completed (because of some unrelated(?) error). Iirc other
frontend drivers behave similarly to the unpatched scsifront, and
incrementing req_prod_pvt late has possible (perhaps just
theoretical) other issues, like parallel retrieval and filling of them
on mor than one CPU. Wouldn't it be better to obtain a request
structure only when everything else is ready (and hence no further
errors can occur)? After all you also need to deal with the acquired
ID upon errors, and seems odd to me to deal with the two parts of
cleanup in different places (and even in different ways).

Jan

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:40, Jan Beulich wrote:
>>>> On 29.11.16 at 12:19, <JGross@suse.com> wrote:
>> On 29/11/16 12:14, Jan Beulich wrote:
>>>>>> On 29.11.16 at 11:50, <JGross@suse.com> wrote:
>>>> --- a/drivers/scsi/xen-scsifront.c
>>>> +++ b/drivers/scsi/xen-scsifront.c
>>>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 
>> vscsifrnt_info *info)
>>>>  
>>>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>>>  
>>>> -	ring->req_prod_pvt++;
>>>
>>> Please note the "_pvt" suffix, which stands for "private": This field is
>>> not visible to the backend. Only ring->sring fields are shared, and
>>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
>>
>> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
>> the case corrected this would advance req_prod by two after the error
>> case before, even if only one request would have made it to the ring.
> 
> Okay, then I may have been mislead by the patch description: I
> understood it to say that you want to avoid the backend seeing
> requests which haven't been filled fully, but it looks like you're
> instead saying that for these requests the filling will never be
> completed (because of some unrelated(?) error). Iirc other
> frontend drivers behave similarly to the unpatched scsifront, and

blkfront and netfront seem to be okay.

> incrementing req_prod_pvt late has possible (perhaps just
> theoretical) other issues, like parallel retrieval and filling of them
> on mor than one CPU. Wouldn't it be better to obtain a request

In scsifront the complete critical path is guarded by a lock.

> structure only when everything else is ready (and hence no further
> errors can occur)? After all you also need to deal with the acquired
> ID upon errors, and seems odd to me to deal with the two parts of
> cleanup in different places (and even in different ways).

Hmm, I can see your point.

I'll have a look how intrusive such a change would be.


Juergen

[PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
 	/* command between backend and frontend */
 	unsigned char act;
+	uint8_t nr_segments;
 	uint16_t rqid;
+	uint16_t ref_rqid;
 
 	unsigned int nr_grants;		/* number of grants in gref[] */
 	struct scsiif_request_segment *sg;	/* scatter/gather elements */
+	struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
 	/* Do reset or abort function. */
 	wait_queue_head_t wq_reset;	/* reset work queue           */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id)
 		scsifront_wake_up(info);
 }
 
-static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
+static int scsifront_do_request(struct vscsifrnt_info *info,
+				struct vscsifrnt_shadow *shadow)
 {
 	struct vscsiif_front_ring *ring = &(info->ring);
 	struct vscsiif_request *ring_req;
+	struct scsi_cmnd *sc = shadow->sc;
 	uint32_t id;
+	int i, notify;
+
+	if (RING_FULL(&info->ring))
+		return -EBUSY;
 
 	id = scsifront_get_rqid(info);	/* use id in response */
 	if (id >= VSCSIIF_MAX_REQS)
-		return NULL;
+		return -EBUSY;
+
+	info->shadow[id] = shadow;
+	shadow->rqid = id;
 
 	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
-
 	ring->req_prod_pvt++;
 
-	ring_req->rqid = (uint16_t)id;
+	ring_req->rqid        = id;
+	ring_req->act         = shadow->act;
+	ring_req->ref_rqid    = shadow->ref_rqid;
+	ring_req->nr_segments = shadow->nr_segments;
 
-	return ring_req;
-}
+	ring_req->id      = sc->device->id;
+	ring_req->lun     = sc->device->lun;
+	ring_req->channel = sc->device->channel;
+	ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-	struct vscsiif_front_ring *ring = &(info->ring);
-	int notify;
+	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+	ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+	for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+		ring_req->seg[i] = shadow->seg[i];
 
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
 	if (notify)
 		notify_remote_via_irq(info->irq);
+
+	return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+				  struct vscsifrnt_shadow *shadow)
 {
-	struct vscsifrnt_shadow *s = info->shadow[id];
 	int i;
 
-	if (s->sc->sc_data_direction == DMA_NONE)
+	if (shadow->sc->sc_data_direction == DMA_NONE)
 		return;
 
-	for (i = 0; i < s->nr_grants; i++) {
-		if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+	for (i = 0; i < shadow->nr_grants; i++) {
+		if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
 			shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 				     "grant still in use by backend\n");
 			BUG();
 		}
-		gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+		gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
 	}
 
-	kfree(s->sg);
+	kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
 				   struct vscsiif_response *ring_rsp)
 {
+	struct vscsifrnt_shadow *shadow;
 	struct scsi_cmnd *sc;
 	uint32_t id;
 	uint8_t sense_len;
 
 	id = ring_rsp->rqid;
-	sc = info->shadow[id]->sc;
+	shadow = info->shadow[id];
+	sc = shadow->sc;
 
 	BUG_ON(sc == NULL);
 
-	scsifront_gnttab_done(info, id);
+	scsifront_gnttab_done(info, shadow);
 	scsifront_put_rqid(info, id);
 
 	sc->result = ring_rsp->rslt;
@@ -366,7 +391,6 @@ static void scsifront_finish_all(struct vscsifrnt_info *info)
 
 static int map_data_for_request(struct vscsifrnt_info *info,
 				struct scsi_cmnd *sc,
-				struct vscsiif_request *ring_req,
 				struct vscsifrnt_shadow *shadow)
 {
 	grant_ref_t gref_head;
@@ -379,7 +403,6 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	struct scatterlist *sg;
 	struct scsiif_request_segment *seg;
 
-	ring_req->nr_segments = 0;
 	if (sc->sc_data_direction == DMA_NONE || !data_len)
 		return 0;
 
@@ -398,7 +421,7 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 		if (!shadow->sg)
 			return -ENOMEM;
 	}
-	seg = shadow->sg ? : ring_req->seg;
+	seg = shadow->sg ? : shadow->seg;
 
 	err = gnttab_alloc_grant_references(seg_grants + data_grants,
 					    &gref_head);
@@ -423,9 +446,9 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 				info->dev->otherend_id,
 				xen_page_to_gfn(page), 1);
 			shadow->gref[ref_cnt] = ref;
-			ring_req->seg[ref_cnt].gref   = ref;
-			ring_req->seg[ref_cnt].offset = (uint16_t)off;
-			ring_req->seg[ref_cnt].length = (uint16_t)bytes;
+			shadow->seg[ref_cnt].gref   = ref;
+			shadow->seg[ref_cnt].offset = (uint16_t)off;
+			shadow->seg[ref_cnt].length = (uint16_t)bytes;
 
 			page++;
 			len -= bytes;
@@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	}
 
 	if (seg_grants)
-		ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
+		shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
 	else
-		ring_req->nr_segments = (uint8_t)ref_cnt;
+		shadow->nr_segments = (uint8_t)ref_cnt;
 	shadow->nr_grants = ref_cnt;
 
 	return 0;
 }
 
-static struct vscsiif_request *scsifront_command2ring(
-		struct vscsifrnt_info *info, struct scsi_cmnd *sc,
-		struct vscsifrnt_shadow *shadow)
-{
-	struct vscsiif_request *ring_req;
-
-	memset(shadow, 0, sizeof(*shadow));
-
-	ring_req = scsifront_pre_req(info);
-	if (!ring_req)
-		return NULL;
-
-	info->shadow[ring_req->rqid] = shadow;
-	shadow->rqid = ring_req->rqid;
-
-	ring_req->id      = sc->device->id;
-	ring_req->lun     = sc->device->lun;
-	ring_req->channel = sc->device->channel;
-	ring_req->cmd_len = sc->cmd_len;
-
-	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
-
-	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
-
-	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
-	ring_req->timeout_per_command = sc->request->timeout / HZ;
-
-	return ring_req;
-}
-
 static int scsifront_enter(struct vscsifrnt_info *info)
 {
 	if (info->pause)
@@ -536,36 +529,25 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 				  struct scsi_cmnd *sc)
 {
 	struct vscsifrnt_info *info = shost_priv(shost);
-	struct vscsiif_request *ring_req;
 	struct vscsifrnt_shadow *shadow = scsi_cmd_priv(sc);
 	unsigned long flags;
 	int err;
-	uint16_t rqid;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	if (scsifront_enter(info)) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		return SCSI_MLQUEUE_HOST_BUSY;
-	}
-	if (RING_FULL(&info->ring))
-		goto busy;
-
-	ring_req = scsifront_command2ring(info, sc, shadow);
-	if (!ring_req)
-		goto busy;
 
 	sc->result = 0;
-
-	rqid = ring_req->rqid;
-	ring_req->act = VSCSIIF_ACT_SCSI_CDB;
+	memset(shadow, 0, sizeof(*shadow));
 
 	shadow->sc  = sc;
 	shadow->act = VSCSIIF_ACT_SCSI_CDB;
 
-	err = map_data_for_request(info, sc, ring_req, shadow);
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsifront_enter(info)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		return SCSI_MLQUEUE_HOST_BUSY;
+	}
+
+	err = map_data_for_request(info, sc, shadow);
 	if (err < 0) {
 		pr_debug("%s: err %d\n", __func__, err);
-		scsifront_put_rqid(info, rqid);
 		scsifront_return(info);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		if (err == -ENOMEM)
@@ -575,7 +557,11 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 		return 0;
 	}
 
-	scsifront_do_request(info);
+	if (scsifront_do_request(info, shadow)) {
+		scsifront_gnttab_done(info, shadow);
+		goto busy;
+	}
+
 	scsifront_return(info);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
@@ -598,26 +584,30 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	struct Scsi_Host *host = sc->device->host;
 	struct vscsifrnt_info *info = shost_priv(host);
 	struct vscsifrnt_shadow *shadow, *s = scsi_cmd_priv(sc);
-	struct vscsiif_request *ring_req;
 	int err = 0;
 
-	shadow = kmalloc(sizeof(*shadow), GFP_NOIO);
+	shadow = kzalloc(sizeof(*shadow), GFP_NOIO);
 	if (!shadow)
 		return FAILED;
 
+	shadow->act = act;
+	shadow->rslt_reset = RSLT_RESET_WAITING;
+	shadow->sc = sc;
+	shadow->ref_rqid = s->rqid;
+	init_waitqueue_head(&shadow->wq_reset);
+
 	spin_lock_irq(host->host_lock);
 
 	for (;;) {
-		if (!RING_FULL(&info->ring)) {
-			ring_req = scsifront_command2ring(info, sc, shadow);
-			if (ring_req)
-				break;
-		}
-		if (err || info->pause) {
-			spin_unlock_irq(host->host_lock);
-			kfree(shadow);
-			return FAILED;
-		}
+		if (scsifront_enter(info))
+			goto fail;
+
+		if (!scsifront_do_request(info, shadow))
+			break;
+
+		scsifront_return(info);
+		if (err)
+			goto fail;
 		info->wait_ring_available = 1;
 		spin_unlock_irq(host->host_lock);
 		err = wait_event_interruptible(info->wq_sync,
@@ -625,23 +615,6 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 		spin_lock_irq(host->host_lock);
 	}
 
-	if (scsifront_enter(info)) {
-		spin_unlock_irq(host->host_lock);
-		kfree(shadow);
-		return FAILED;
-	}
-
-	ring_req->act = act;
-	ring_req->ref_rqid = s->rqid;
-
-	shadow->act = act;
-	shadow->rslt_reset = RSLT_RESET_WAITING;
-	init_waitqueue_head(&shadow->wq_reset);
-
-	ring_req->nr_segments = 0;
-
-	scsifront_do_request(info);
-
 	spin_unlock_irq(host->host_lock);
 	err = wait_event_interruptible(shadow->wq_reset, shadow->wait_reset);
 	spin_lock_irq(host->host_lock);
@@ -660,6 +633,11 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	scsifront_return(info);
 	spin_unlock_irq(host->host_lock);
 	return err;
+
+fail:
+	spin_unlock_irq(host->host_lock);
+	kfree(shadow);
+	return FAILED;
 }
 
 static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
-- 
2.10.2

[PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
Resend with corrected mail address for Martin Peter
---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
 	/* command between backend and frontend */
 	unsigned char act;
+	uint8_t nr_segments;
 	uint16_t rqid;
+	uint16_t ref_rqid;
 
 	unsigned int nr_grants;		/* number of grants in gref[] */
 	struct scsiif_request_segment *sg;	/* scatter/gather elements */
+	struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
 	/* Do reset or abort function. */
 	wait_queue_head_t wq_reset;	/* reset work queue           */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id)
 		scsifront_wake_up(info);
 }
 
-static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
+static int scsifront_do_request(struct vscsifrnt_info *info,
+				struct vscsifrnt_shadow *shadow)
 {
 	struct vscsiif_front_ring *ring = &(info->ring);
 	struct vscsiif_request *ring_req;
+	struct scsi_cmnd *sc = shadow->sc;
 	uint32_t id;
+	int i, notify;
+
+	if (RING_FULL(&info->ring))
+		return -EBUSY;
 
 	id = scsifront_get_rqid(info);	/* use id in response */
 	if (id >= VSCSIIF_MAX_REQS)
-		return NULL;
+		return -EBUSY;
+
+	info->shadow[id] = shadow;
+	shadow->rqid = id;
 
 	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
-
 	ring->req_prod_pvt++;
 
-	ring_req->rqid = (uint16_t)id;
+	ring_req->rqid        = id;
+	ring_req->act         = shadow->act;
+	ring_req->ref_rqid    = shadow->ref_rqid;
+	ring_req->nr_segments = shadow->nr_segments;
 
-	return ring_req;
-}
+	ring_req->id      = sc->device->id;
+	ring_req->lun     = sc->device->lun;
+	ring_req->channel = sc->device->channel;
+	ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-	struct vscsiif_front_ring *ring = &(info->ring);
-	int notify;
+	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+	ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+	for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+		ring_req->seg[i] = shadow->seg[i];
 
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
 	if (notify)
 		notify_remote_via_irq(info->irq);
+
+	return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+				  struct vscsifrnt_shadow *shadow)
 {
-	struct vscsifrnt_shadow *s = info->shadow[id];
 	int i;
 
-	if (s->sc->sc_data_direction == DMA_NONE)
+	if (shadow->sc->sc_data_direction == DMA_NONE)
 		return;
 
-	for (i = 0; i < s->nr_grants; i++) {
-		if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+	for (i = 0; i < shadow->nr_grants; i++) {
+		if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
 			shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 				     "grant still in use by backend\n");
 			BUG();
 		}
-		gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+		gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
 	}
 
-	kfree(s->sg);
+	kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
 				   struct vscsiif_response *ring_rsp)
 {
+	struct vscsifrnt_shadow *shadow;
 	struct scsi_cmnd *sc;
 	uint32_t id;
 	uint8_t sense_len;
 
 	id = ring_rsp->rqid;
-	sc = info->shadow[id]->sc;
+	shadow = info->shadow[id];
+	sc = shadow->sc;
 
 	BUG_ON(sc == NULL);
 
-	scsifront_gnttab_done(info, id);
+	scsifront_gnttab_done(info, shadow);
 	scsifront_put_rqid(info, id);
 
 	sc->result = ring_rsp->rslt;
@@ -366,7 +391,6 @@ static void scsifront_finish_all(struct vscsifrnt_info *info)
 
 static int map_data_for_request(struct vscsifrnt_info *info,
 				struct scsi_cmnd *sc,
-				struct vscsiif_request *ring_req,
 				struct vscsifrnt_shadow *shadow)
 {
 	grant_ref_t gref_head;
@@ -379,7 +403,6 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	struct scatterlist *sg;
 	struct scsiif_request_segment *seg;
 
-	ring_req->nr_segments = 0;
 	if (sc->sc_data_direction == DMA_NONE || !data_len)
 		return 0;
 
@@ -398,7 +421,7 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 		if (!shadow->sg)
 			return -ENOMEM;
 	}
-	seg = shadow->sg ? : ring_req->seg;
+	seg = shadow->sg ? : shadow->seg;
 
 	err = gnttab_alloc_grant_references(seg_grants + data_grants,
 					    &gref_head);
@@ -423,9 +446,9 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 				info->dev->otherend_id,
 				xen_page_to_gfn(page), 1);
 			shadow->gref[ref_cnt] = ref;
-			ring_req->seg[ref_cnt].gref   = ref;
-			ring_req->seg[ref_cnt].offset = (uint16_t)off;
-			ring_req->seg[ref_cnt].length = (uint16_t)bytes;
+			shadow->seg[ref_cnt].gref   = ref;
+			shadow->seg[ref_cnt].offset = (uint16_t)off;
+			shadow->seg[ref_cnt].length = (uint16_t)bytes;
 
 			page++;
 			len -= bytes;
@@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	}
 
 	if (seg_grants)
-		ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
+		shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
 	else
-		ring_req->nr_segments = (uint8_t)ref_cnt;
+		shadow->nr_segments = (uint8_t)ref_cnt;
 	shadow->nr_grants = ref_cnt;
 
 	return 0;
 }
 
-static struct vscsiif_request *scsifront_command2ring(
-		struct vscsifrnt_info *info, struct scsi_cmnd *sc,
-		struct vscsifrnt_shadow *shadow)
-{
-	struct vscsiif_request *ring_req;
-
-	memset(shadow, 0, sizeof(*shadow));
-
-	ring_req = scsifront_pre_req(info);
-	if (!ring_req)
-		return NULL;
-
-	info->shadow[ring_req->rqid] = shadow;
-	shadow->rqid = ring_req->rqid;
-
-	ring_req->id      = sc->device->id;
-	ring_req->lun     = sc->device->lun;
-	ring_req->channel = sc->device->channel;
-	ring_req->cmd_len = sc->cmd_len;
-
-	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
-
-	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
-
-	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
-	ring_req->timeout_per_command = sc->request->timeout / HZ;
-
-	return ring_req;
-}
-
 static int scsifront_enter(struct vscsifrnt_info *info)
 {
 	if (info->pause)
@@ -536,36 +529,25 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 				  struct scsi_cmnd *sc)
 {
 	struct vscsifrnt_info *info = shost_priv(shost);
-	struct vscsiif_request *ring_req;
 	struct vscsifrnt_shadow *shadow = scsi_cmd_priv(sc);
 	unsigned long flags;
 	int err;
-	uint16_t rqid;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	if (scsifront_enter(info)) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		return SCSI_MLQUEUE_HOST_BUSY;
-	}
-	if (RING_FULL(&info->ring))
-		goto busy;
-
-	ring_req = scsifront_command2ring(info, sc, shadow);
-	if (!ring_req)
-		goto busy;
 
 	sc->result = 0;
-
-	rqid = ring_req->rqid;
-	ring_req->act = VSCSIIF_ACT_SCSI_CDB;
+	memset(shadow, 0, sizeof(*shadow));
 
 	shadow->sc  = sc;
 	shadow->act = VSCSIIF_ACT_SCSI_CDB;
 
-	err = map_data_for_request(info, sc, ring_req, shadow);
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsifront_enter(info)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		return SCSI_MLQUEUE_HOST_BUSY;
+	}
+
+	err = map_data_for_request(info, sc, shadow);
 	if (err < 0) {
 		pr_debug("%s: err %d\n", __func__, err);
-		scsifront_put_rqid(info, rqid);
 		scsifront_return(info);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		if (err == -ENOMEM)
@@ -575,7 +557,11 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 		return 0;
 	}
 
-	scsifront_do_request(info);
+	if (scsifront_do_request(info, shadow)) {
+		scsifront_gnttab_done(info, shadow);
+		goto busy;
+	}
+
 	scsifront_return(info);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
@@ -598,26 +584,30 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	struct Scsi_Host *host = sc->device->host;
 	struct vscsifrnt_info *info = shost_priv(host);
 	struct vscsifrnt_shadow *shadow, *s = scsi_cmd_priv(sc);
-	struct vscsiif_request *ring_req;
 	int err = 0;
 
-	shadow = kmalloc(sizeof(*shadow), GFP_NOIO);
+	shadow = kzalloc(sizeof(*shadow), GFP_NOIO);
 	if (!shadow)
 		return FAILED;
 
+	shadow->act = act;
+	shadow->rslt_reset = RSLT_RESET_WAITING;
+	shadow->sc = sc;
+	shadow->ref_rqid = s->rqid;
+	init_waitqueue_head(&shadow->wq_reset);
+
 	spin_lock_irq(host->host_lock);
 
 	for (;;) {
-		if (!RING_FULL(&info->ring)) {
-			ring_req = scsifront_command2ring(info, sc, shadow);
-			if (ring_req)
-				break;
-		}
-		if (err || info->pause) {
-			spin_unlock_irq(host->host_lock);
-			kfree(shadow);
-			return FAILED;
-		}
+		if (scsifront_enter(info))
+			goto fail;
+
+		if (!scsifront_do_request(info, shadow))
+			break;
+
+		scsifront_return(info);
+		if (err)
+			goto fail;
 		info->wait_ring_available = 1;
 		spin_unlock_irq(host->host_lock);
 		err = wait_event_interruptible(info->wq_sync,
@@ -625,23 +615,6 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 		spin_lock_irq(host->host_lock);
 	}
 
-	if (scsifront_enter(info)) {
-		spin_unlock_irq(host->host_lock);
-		kfree(shadow);
-		return FAILED;
-	}
-
-	ring_req->act = act;
-	ring_req->ref_rqid = s->rqid;
-
-	shadow->act = act;
-	shadow->rslt_reset = RSLT_RESET_WAITING;
-	init_waitqueue_head(&shadow->wq_reset);
-
-	ring_req->nr_segments = 0;
-
-	scsifront_do_request(info);
-
 	spin_unlock_irq(host->host_lock);
 	err = wait_event_interruptible(shadow->wq_reset, shadow->wait_reset);
 	spin_lock_irq(host->host_lock);
@@ -660,6 +633,11 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	scsifront_return(info);
 	spin_unlock_irq(host->host_lock);
 	return err;
+
+fail:
+	spin_unlock_irq(host->host_lock);
+	kfree(shadow);
+	return FAILED;
 }
 
 static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
-- 
2.10.2

[PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
Aargh! Need more coffee! Resend with corrected mail address for Martin Petersen
---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
 	/* command between backend and frontend */
 	unsigned char act;
+	uint8_t nr_segments;
 	uint16_t rqid;
+	uint16_t ref_rqid;
 
 	unsigned int nr_grants;		/* number of grants in gref[] */
 	struct scsiif_request_segment *sg;	/* scatter/gather elements */
+	struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
 	/* Do reset or abort function. */
 	wait_queue_head_t wq_reset;	/* reset work queue           */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id)
 		scsifront_wake_up(info);
 }
 
-static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
+static int scsifront_do_request(struct vscsifrnt_info *info,
+				struct vscsifrnt_shadow *shadow)
 {
 	struct vscsiif_front_ring *ring = &(info->ring);
 	struct vscsiif_request *ring_req;
+	struct scsi_cmnd *sc = shadow->sc;
 	uint32_t id;
+	int i, notify;
+
+	if (RING_FULL(&info->ring))
+		return -EBUSY;
 
 	id = scsifront_get_rqid(info);	/* use id in response */
 	if (id >= VSCSIIF_MAX_REQS)
-		return NULL;
+		return -EBUSY;
+
+	info->shadow[id] = shadow;
+	shadow->rqid = id;
 
 	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
-
 	ring->req_prod_pvt++;
 
-	ring_req->rqid = (uint16_t)id;
+	ring_req->rqid        = id;
+	ring_req->act         = shadow->act;
+	ring_req->ref_rqid    = shadow->ref_rqid;
+	ring_req->nr_segments = shadow->nr_segments;
 
-	return ring_req;
-}
+	ring_req->id      = sc->device->id;
+	ring_req->lun     = sc->device->lun;
+	ring_req->channel = sc->device->channel;
+	ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-	struct vscsiif_front_ring *ring = &(info->ring);
-	int notify;
+	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+	ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+	for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+		ring_req->seg[i] = shadow->seg[i];
 
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
 	if (notify)
 		notify_remote_via_irq(info->irq);
+
+	return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+				  struct vscsifrnt_shadow *shadow)
 {
-	struct vscsifrnt_shadow *s = info->shadow[id];
 	int i;
 
-	if (s->sc->sc_data_direction == DMA_NONE)
+	if (shadow->sc->sc_data_direction == DMA_NONE)
 		return;
 
-	for (i = 0; i < s->nr_grants; i++) {
-		if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+	for (i = 0; i < shadow->nr_grants; i++) {
+		if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
 			shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 				     "grant still in use by backend\n");
 			BUG();
 		}
-		gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+		gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
 	}
 
-	kfree(s->sg);
+	kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
 				   struct vscsiif_response *ring_rsp)
 {
+	struct vscsifrnt_shadow *shadow;
 	struct scsi_cmnd *sc;
 	uint32_t id;
 	uint8_t sense_len;
 
 	id = ring_rsp->rqid;
-	sc = info->shadow[id]->sc;
+	shadow = info->shadow[id];
+	sc = shadow->sc;
 
 	BUG_ON(sc == NULL);
 
-	scsifront_gnttab_done(info, id);
+	scsifront_gnttab_done(info, shadow);
 	scsifront_put_rqid(info, id);
 
 	sc->result = ring_rsp->rslt;
@@ -366,7 +391,6 @@ static void scsifront_finish_all(struct vscsifrnt_info *info)
 
 static int map_data_for_request(struct vscsifrnt_info *info,
 				struct scsi_cmnd *sc,
-				struct vscsiif_request *ring_req,
 				struct vscsifrnt_shadow *shadow)
 {
 	grant_ref_t gref_head;
@@ -379,7 +403,6 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	struct scatterlist *sg;
 	struct scsiif_request_segment *seg;
 
-	ring_req->nr_segments = 0;
 	if (sc->sc_data_direction == DMA_NONE || !data_len)
 		return 0;
 
@@ -398,7 +421,7 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 		if (!shadow->sg)
 			return -ENOMEM;
 	}
-	seg = shadow->sg ? : ring_req->seg;
+	seg = shadow->sg ? : shadow->seg;
 
 	err = gnttab_alloc_grant_references(seg_grants + data_grants,
 					    &gref_head);
@@ -423,9 +446,9 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 				info->dev->otherend_id,
 				xen_page_to_gfn(page), 1);
 			shadow->gref[ref_cnt] = ref;
-			ring_req->seg[ref_cnt].gref   = ref;
-			ring_req->seg[ref_cnt].offset = (uint16_t)off;
-			ring_req->seg[ref_cnt].length = (uint16_t)bytes;
+			shadow->seg[ref_cnt].gref   = ref;
+			shadow->seg[ref_cnt].offset = (uint16_t)off;
+			shadow->seg[ref_cnt].length = (uint16_t)bytes;
 
 			page++;
 			len -= bytes;
@@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
 	}
 
 	if (seg_grants)
-		ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
+		shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
 	else
-		ring_req->nr_segments = (uint8_t)ref_cnt;
+		shadow->nr_segments = (uint8_t)ref_cnt;
 	shadow->nr_grants = ref_cnt;
 
 	return 0;
 }
 
-static struct vscsiif_request *scsifront_command2ring(
-		struct vscsifrnt_info *info, struct scsi_cmnd *sc,
-		struct vscsifrnt_shadow *shadow)
-{
-	struct vscsiif_request *ring_req;
-
-	memset(shadow, 0, sizeof(*shadow));
-
-	ring_req = scsifront_pre_req(info);
-	if (!ring_req)
-		return NULL;
-
-	info->shadow[ring_req->rqid] = shadow;
-	shadow->rqid = ring_req->rqid;
-
-	ring_req->id      = sc->device->id;
-	ring_req->lun     = sc->device->lun;
-	ring_req->channel = sc->device->channel;
-	ring_req->cmd_len = sc->cmd_len;
-
-	BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
-
-	memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
-
-	ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
-	ring_req->timeout_per_command = sc->request->timeout / HZ;
-
-	return ring_req;
-}
-
 static int scsifront_enter(struct vscsifrnt_info *info)
 {
 	if (info->pause)
@@ -536,36 +529,25 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 				  struct scsi_cmnd *sc)
 {
 	struct vscsifrnt_info *info = shost_priv(shost);
-	struct vscsiif_request *ring_req;
 	struct vscsifrnt_shadow *shadow = scsi_cmd_priv(sc);
 	unsigned long flags;
 	int err;
-	uint16_t rqid;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	if (scsifront_enter(info)) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		return SCSI_MLQUEUE_HOST_BUSY;
-	}
-	if (RING_FULL(&info->ring))
-		goto busy;
-
-	ring_req = scsifront_command2ring(info, sc, shadow);
-	if (!ring_req)
-		goto busy;
 
 	sc->result = 0;
-
-	rqid = ring_req->rqid;
-	ring_req->act = VSCSIIF_ACT_SCSI_CDB;
+	memset(shadow, 0, sizeof(*shadow));
 
 	shadow->sc  = sc;
 	shadow->act = VSCSIIF_ACT_SCSI_CDB;
 
-	err = map_data_for_request(info, sc, ring_req, shadow);
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsifront_enter(info)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		return SCSI_MLQUEUE_HOST_BUSY;
+	}
+
+	err = map_data_for_request(info, sc, shadow);
 	if (err < 0) {
 		pr_debug("%s: err %d\n", __func__, err);
-		scsifront_put_rqid(info, rqid);
 		scsifront_return(info);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		if (err == -ENOMEM)
@@ -575,7 +557,11 @@ static int scsifront_queuecommand(struct Scsi_Host *shost,
 		return 0;
 	}
 
-	scsifront_do_request(info);
+	if (scsifront_do_request(info, shadow)) {
+		scsifront_gnttab_done(info, shadow);
+		goto busy;
+	}
+
 	scsifront_return(info);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
@@ -598,26 +584,30 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	struct Scsi_Host *host = sc->device->host;
 	struct vscsifrnt_info *info = shost_priv(host);
 	struct vscsifrnt_shadow *shadow, *s = scsi_cmd_priv(sc);
-	struct vscsiif_request *ring_req;
 	int err = 0;
 
-	shadow = kmalloc(sizeof(*shadow), GFP_NOIO);
+	shadow = kzalloc(sizeof(*shadow), GFP_NOIO);
 	if (!shadow)
 		return FAILED;
 
+	shadow->act = act;
+	shadow->rslt_reset = RSLT_RESET_WAITING;
+	shadow->sc = sc;
+	shadow->ref_rqid = s->rqid;
+	init_waitqueue_head(&shadow->wq_reset);
+
 	spin_lock_irq(host->host_lock);
 
 	for (;;) {
-		if (!RING_FULL(&info->ring)) {
-			ring_req = scsifront_command2ring(info, sc, shadow);
-			if (ring_req)
-				break;
-		}
-		if (err || info->pause) {
-			spin_unlock_irq(host->host_lock);
-			kfree(shadow);
-			return FAILED;
-		}
+		if (scsifront_enter(info))
+			goto fail;
+
+		if (!scsifront_do_request(info, shadow))
+			break;
+
+		scsifront_return(info);
+		if (err)
+			goto fail;
 		info->wait_ring_available = 1;
 		spin_unlock_irq(host->host_lock);
 		err = wait_event_interruptible(info->wq_sync,
@@ -625,23 +615,6 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 		spin_lock_irq(host->host_lock);
 	}
 
-	if (scsifront_enter(info)) {
-		spin_unlock_irq(host->host_lock);
-		kfree(shadow);
-		return FAILED;
-	}
-
-	ring_req->act = act;
-	ring_req->ref_rqid = s->rqid;
-
-	shadow->act = act;
-	shadow->rslt_reset = RSLT_RESET_WAITING;
-	init_waitqueue_head(&shadow->wq_reset);
-
-	ring_req->nr_segments = 0;
-
-	scsifront_do_request(info);
-
 	spin_unlock_irq(host->host_lock);
 	err = wait_event_interruptible(shadow->wq_reset, shadow->wait_reset);
 	spin_lock_irq(host->host_lock);
@@ -660,6 +633,11 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
 	scsifront_return(info);
 	spin_unlock_irq(host->host_lock);
 	return err;
+
+fail:
+	spin_unlock_irq(host->host_lock);
+	kfree(shadow);
+	return FAILED;
 }
 
 static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
-- 
2.10.2

Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Boris Ostrovsky]

On 12/02/2016 01:15 AM, Juergen Gross wrote:
>  
> -static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
> +static int scsifront_do_request(struct vscsifrnt_info *info,
> +				struct vscsifrnt_shadow *shadow)
>  {
>  	struct vscsiif_front_ring *ring = &(info->ring);
>  	struct vscsiif_request *ring_req;
> +	struct scsi_cmnd *sc = shadow->sc;
>  	uint32_t id;
> +	int i, notify;
> +
> +	if (RING_FULL(&info->ring))
> +		return -EBUSY;
>  
>  	id = scsifront_get_rqid(info);	/* use id in response */
>  	if (id >= VSCSIIF_MAX_REQS)
> -		return NULL;
> +		return -EBUSY;
> +
> +	info->shadow[id] = shadow;
> +	shadow->rqid = id;
>  
>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
> -
>  	ring->req_prod_pvt++;
>  
> -	ring_req->rqid = (uint16_t)id;
> +	ring_req->rqid        = id;
> +	ring_req->act         = shadow->act;
> +	ring_req->ref_rqid    = shadow->ref_rqid;
> +	ring_req->nr_segments = shadow->nr_segments;

Shouldn't req_prod_pvt be incremented after you've copied everything? (I
realize that there is not error until everything has been copied but still.)



> @@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
>  	}
>  
>  	if (seg_grants)
> -		ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
> +		shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;

Are we guaranteed that seg_grants is not going to have VSCSIIF_SG_GRANT
bit set?

-boris

Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Juergen Gross]

On 05/12/16 16:32, Boris Ostrovsky wrote:
> On 12/02/2016 01:15 AM, Juergen Gross wrote:
>>  
>> -static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>> +static int scsifront_do_request(struct vscsifrnt_info *info,
>> +				struct vscsifrnt_shadow *shadow)
>>  {
>>  	struct vscsiif_front_ring *ring = &(info->ring);
>>  	struct vscsiif_request *ring_req;
>> +	struct scsi_cmnd *sc = shadow->sc;
>>  	uint32_t id;
>> +	int i, notify;
>> +
>> +	if (RING_FULL(&info->ring))
>> +		return -EBUSY;
>>  
>>  	id = scsifront_get_rqid(info);	/* use id in response */
>>  	if (id >= VSCSIIF_MAX_REQS)
>> -		return NULL;
>> +		return -EBUSY;
>> +
>> +	info->shadow[id] = shadow;
>> +	shadow->rqid = id;
>>  
>>  	ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>> -
>>  	ring->req_prod_pvt++;
>>  
>> -	ring_req->rqid = (uint16_t)id;
>> +	ring_req->rqid        = id;
>> +	ring_req->act         = shadow->act;
>> +	ring_req->ref_rqid    = shadow->ref_rqid;
>> +	ring_req->nr_segments = shadow->nr_segments;
> 
> Shouldn't req_prod_pvt be incremented after you've copied everything? (I
> realize that there is not error until everything has been copied but still.)

That doesn't really matter as it is private.

>> @@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
>>  	}
>>  
>>  	if (seg_grants)
>> -		ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
>> +		shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
> 
> Are we guaranteed that seg_grants is not going to have VSCSIIF_SG_GRANT
> bit set?

Absolutely, yes. Can't be larger than VSCSIIF_SG_TABLESIZE which is 26.


Juergen

Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Boris Ostrovsky]

On 12/02/2016 01:15 AM, Juergen Gross wrote:
> Instead of requesting a new slot on the ring to the backend early, do
> so only after all has been setup for the request to be sent. This
> makes error handling easier as we don't need to undo the request id
> allocation and ring slot allocation.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

(although it would be good to have someone familiar with this code look 
at this as well).

Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

[Juergen Gross]

On 02/12/16 07:15, Juergen Gross wrote:
> Instead of requesting a new slot on the ring to the backend early, do
> so only after all has been setup for the request to be sent. This
> makes error handling easier as we don't need to undo the request id
> allocation and ring slot allocation.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Commited to xen/tip.git for-linus-4.10


Juergen

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Dan Carpenter]

On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote:
> On 19/11/16 19:22, Quentin Lambert wrote:
> > Most error branches following the call to kmalloc contain
> > a call to kfree. This patch add these calls where they are
> > missing.
> > 
> > This issue was found with Hector.
> > 
> > Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
> 
> Nice catch. I think this will need some more work, I'll do a
> follow-on patch.
> 

The error handling is really weird.  Could you send your follow on to
this thread?

regards,
dan carpenter

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 05/12/16 21:53, Dan Carpenter wrote:
> On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote:
>> On 19/11/16 19:22, Quentin Lambert wrote:
>>> Most error branches following the call to kmalloc contain
>>> a call to kfree. This patch add these calls where they are
>>> missing.
>>>
>>> This issue was found with Hector.
>>>
>>> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
>>
>> Nice catch. I think this will need some more work, I'll do a
>> follow-on patch.
>>
> 
> The error handling is really weird.  Could you send your follow on to
> this thread?

I did:

https://lkml.org/lkml/2016/12/2/17


Juergen

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Dan Carpenter]

Oops.  Sorry for the noise.

regards,
dan carpenter

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 19/11/16 19:22, Quentin Lambert wrote:
> Most error branches following the call to kmalloc contain
> a call to kfree. This patch add these calls where they are
> missing.
> 
> This issue was found with Hector.
> 
> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>

Applied to xen/tip.git for-linus-4.10


Juergen

> 
> ---
>  drivers/scsi/xen-scsifront.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
>  
>  	if (scsifront_enter(info)) {
>  		spin_unlock_irq(host->host_lock);
> +		kfree(shadow);
>  		return FAILED;
>  	}
>  
> 
>