Skip to contentSkip to conversation

xen-scsifront: Add a missing call to kfree

PATCH Applied Quentin Lambert 7 participants 42 messages Nov 19, 2016 – Dec 9, 2016 2 messages not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xen-scsifront: Add a missing call to kfree

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.

---
 drivers/scsi/xen-scsifront.c |    1 +
 1 file changed, 1 insertion(+)
drivers/scsi/xen-scsifront.c +1 -0
@@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
 
 	if (scsifront_enter(info)) {
 		spin_unlock_irq(host->host_lock);
+		kfree(shadow);
 		return FAILED;
 	}
 
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · 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.

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

Quoted 16 lines
---
 drivers/scsi/xen-scsifront.c |    1 +
 1 file changed, 1 insertion(+)
drivers/scsi/xen-scsifront.c +1 -0
@@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
 
 	if (scsifront_enter(info)) {
 		spin_unlock_irq(host->host_lock);
+		kfree(shadow);
 		return FAILED;
 	}
 
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Juergen" = Juergen Gross <[email protected]> writes:

Juergen,

Quoted 6 lines · Quentin Lambert
Quoted 6 lines

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.

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

Juergen> Reviewed-by: Juergen Gross <[email protected]>

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

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
maintainer
#3 lore raw
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Juergen" = Juergen Gross <[email protected]> writes:

Juergen,

Quoted 5 lines · Quentin Lambert
Quoted 5 lines

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.

Quoted 8 lines
Quoted 1 line
Quoted 1 line

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

Juergen> Reviewed-by: Juergen Gross <[email protected]>

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

maintainer
#4 lore raw
In reply to Martin K. Petersen
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Juergen" == Juergen Gross <[email protected]> writes:

Juergen,

Quoted 5 lines · Quentin Lambert
Quoted 5 lines

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.

Quoted 8 lines
Quoted 1 line
Quoted 1 line

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

Juergen> Reviewed-by: Juergen Gross <[email protected]>

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#5 lore raw
In reply to Juergen Gross
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Juergen" == Juergen Gross <[email protected]> writes:

Juergen,

Quoted 6 lines · Quentin Lambert
Quoted 6 lines

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.

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

Juergen> Reviewed-by: Juergen Gross <[email protected]>

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

Signature · 6 lines
Martin K. Petersen	Oracle Linux Engineering

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#6 lore raw
In reply to Juergen Gross
Quoted 11 lines · Juergen Gross
Quoted 7 lines · 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.

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

maintainer
#7 lore raw

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

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.

---
In case accepted I'll take this patch through the Xen tree as it is based on another patch by [email protected] which is going through Xen, too.

---
 drivers/scsi/xen-scsifront.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/scsi/xen-scsifront.c +3 -2
index c01316c..33805f6 100644
@@ -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;
Signature · 1 line
2.10.2
#8 lore raw

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

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -1
@@ -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

#9 lore raw

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

In reply to Juergen Gross

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -1
@@ -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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#11 lore raw

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

Quoted 5 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
Quoted 8 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 3 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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

#12 lore raw

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

Quoted 15 lines · Juergen Gross
Quoted 5 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
Quoted 8 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 3 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 6 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#13 lore raw

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

Quoted 26 lines · David Vrabel
Quoted 15 lines · Juergen Gross
Quoted 5 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
Quoted 8 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 3 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 6 lines

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.

Quoted 6 lines

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

maintainer
#14 lore raw

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

In reply to David Vrabel
Quoted 26 lines · David Vrabel
Quoted 15 lines · Juergen Gross
Quoted 5 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
Quoted 8 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 3 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 6 lines

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.

Quoted 6 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#15 lore raw

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

In reply to Juergen Gross
Quoted 17 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 12:19, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 

vscsifrnt_info *info)

Quoted 9 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 4 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 3 lines

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

#16 lore raw

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

In reply to Juergen Gross
Quoted 17 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 12:19, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 

vscsifrnt_info *info)

Quoted 9 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 4 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 3 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#18 lore raw

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

Quoted 21 lines · Jan Beulich
Quoted 17 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 12:19, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 

vscsifrnt_info *info)

Quoted 9 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 4 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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

maintainer
#19 lore raw

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

In reply to a message not in the archive
Quoted 21 lines · Jan Beulich
Quoted 17 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 12:19, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 

vscsifrnt_info *info)

Quoted 9 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 4 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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().

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#20 lore raw

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

In reply to a message not in the archive
Quoted 5 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 29.11.16 at 11:50, <[email protected]> wrote:
drivers/scsi/xen-scsifront.c +0 -0
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
 
Quoted 8 lines

drivers/scsi/xen-scsifront.c static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) · line 187

Quoted 3 lines
drivers/scsi/xen-scsifront.c +0 -1
 	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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#21 lore raw

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

In reply to Dan Carpenter

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.

---
In case accepted I'll take this patch through the Xen tree as it is based on another patch by [email protected] which is going through Xen, too.

---
 drivers/scsi/xen-scsifront.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/scsi/xen-scsifront.c +3 -2
index c01316c..33805f6 100644
@@ -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;
Signature · 7 lines
2.10.2


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
maintainer
#22 lore raw

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

In reply to Dan Carpenter

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.

---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 1 line
2.10.2
maintainer
#23 lore raw

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

In reply to Dan Carpenter

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.

---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 7 lines
2.10.2


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
maintainer
#24 lore raw

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

In reply to Dan Carpenter

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.

---
Resend with corrected mail address for Martin Peter

---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 1 line
2.10.2
maintainer
#25 lore raw

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

In reply to Dan Carpenter

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.

---
Resend with corrected mail address for Martin Peter

---
 drivers/scsi/xen-scsifront.c | 190 +++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 7 lines
2.10.2


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
maintainer
#26 lore raw

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

In reply to Dan Carpenter

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.

---
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(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 1 line
2.10.2
#27 lore raw

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

Quoted 21 lines · Juergen Gross
drivers/scsi/xen-scsifront.c +11 -2
-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;

drivers/scsi/xen-scsifront.c static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id) · line 203

Quoted 10 lines
drivers/scsi/xen-scsifront.c +4 -2
 
 	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.)

drivers/scsi/xen-scsifront.c static int map_data_for_request(struct vscsifrnt_info *info, · line 499

Quoted 6 lines
drivers/scsi/xen-scsifront.c +1 -1
@@ -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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#28 lore raw

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

Quoted 27 lines · Boris Ostrovsky
Quoted 26 lines · Juergen Gross
drivers/scsi/xen-scsifront.c +11 -3
-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++;
Quoted 8 lines

drivers/scsi/xen-scsifront.c static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id) · line 203

Quoted 5 lines
drivers/scsi/xen-scsifront.c +4 -1
-	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.

Quoted 9 lines

drivers/scsi/xen-scsifront.c static int map_data_for_request(struct vscsifrnt_info *info, · line 499

Quoted 6 lines
drivers/scsi/xen-scsifront.c +1 -1
@@ -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

maintainer
#29 lore raw

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

In reply to Boris Ostrovsky
Quoted 27 lines · Boris Ostrovsky
Quoted 26 lines · Juergen Gross
drivers/scsi/xen-scsifront.c +11 -3
-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++;
Quoted 8 lines

drivers/scsi/xen-scsifront.c static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id) · line 203

Quoted 5 lines
drivers/scsi/xen-scsifront.c +4 -1
-	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.

Quoted 9 lines

drivers/scsi/xen-scsifront.c static int map_data_for_request(struct vscsifrnt_info *info, · line 499

Quoted 6 lines
drivers/scsi/xen-scsifront.c +1 -1
@@ -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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Reviewed-by on v2 1/1
#30 lore raw

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

In reply to Juergen Gross
Quoted 7 lines · 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.

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

Reviewed-by on v2 1/1
#31 lore raw

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

In reply to Juergen Gross
Quoted 7 lines · 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.

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#32 lore raw

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

In reply to Juergen Gross
Quoted 7 lines · 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.

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

Juergen

maintainer
#33 lore raw

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

In reply to Juergen Gross
Quoted 7 lines · 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.

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

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer
#34 lore raw

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

In reply to Dan Carpenter

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.

---
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(-)
drivers/scsi/xen-scsifront.c +84 -106
index c01316c..9aa1fe1 100644
@@ -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)
Signature · 7 lines
2.10.2


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#35 lore raw
In reply to Juergen Gross
Quoted 11 lines · Juergen Gross
Quoted 7 lines · 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.

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#36 lore raw
In reply to Juergen Gross
Quoted 5 lines · Juergen Gross
Quoted 4 lines · Quentin Lambert

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

Quoted 7 lines
Quoted 3 lines

This issue was found with Hector.

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

maintainer
#37 lore raw
Quoted 8 lines · Dan Carpenter
Quoted 7 lines · Juergen Gross
Quoted 6 lines · 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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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

maintainer
#39 lore raw
In reply to Dan Carpenter
Quoted 8 lines · Dan Carpenter
Quoted 7 lines · Juergen Gross
Quoted 6 lines · 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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#40 lore raw
In reply to Juergen Gross
Quoted 5 lines · Juergen Gross
Quoted 4 lines · Quentin Lambert

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

Quoted 7 lines
Quoted 3 lines

This issue was found with Hector.

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer Reviewed-by on v1 1/1
#41 lore raw
In reply to Quentin Lambert
Quoted 7 lines · 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.

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

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

drivers/scsi/xen-scsifront.c static int scsifront_action_handler(stru · line 633

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

maintainer Applied
#42 lore raw
In reply to Quentin Lambert
Quoted 7 lines · 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.

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

Juergen

Quoted 17 lines
---
 drivers/scsi/xen-scsifront.c |    1 +
 1 file changed, 1 insertion(+)
drivers/scsi/xen-scsifront.c +1 -0
@@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
 
 	if (scsifront_enter(info)) {
 		spin_unlock_irq(host->host_lock);
+		kfree(shadow);
 		return FAILED;
 	}
 
maintainer Applied
#43 lore raw
In reply to Quentin Lambert
Quoted 7 lines · 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.

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

Juergen

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

drivers/scsi/xen-scsifront.c static int scsifront_action_handler(stru · line 633

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel