xen-scsifront: Add a missing call to kfree
Subsystems (4)
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(struif (scsifront_enter (info)) {spin_unlock_irq (host->host_lock); +kfree (shadow);return FAILED ; }
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.
Signed-off-by Quentin Lambert <[email protected]>
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(struif (scsifront_enter (info)) {spin_unlock_irq (host->host_lock); +kfree (shadow);return FAILED ; }
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.
Signed-off-by Quentin Lambert <[email protected]>
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
Juergen Gross
maintainerQuoted 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
Signed-off-by Quentin Lambert <[email protected]>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
Juergen Gross
maintainerQuoted 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
Signed-off-by Quentin Lambert <[email protected]>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
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.
Signed-off-by Quentin Lambert <[email protected]>
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
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.
Signed-off-by Quentin Lambert <[email protected]>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
Juergen Gross
maintainerPATCH 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
@@ -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
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
Re: [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to Juergen Grossdrivers/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]
Juergen Gross
maintainerRe: [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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In the case corrected this would advance req_prod by two after the error case before, even if only one request would have made it to the ring.
As an alternative I could have decremented req_prod_pvt in case of an error, but I like my current solution better.
Juergen
Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error
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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
Quoted 6 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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_pvtin 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
Juergen Gross
maintainerRe: [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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().Quoted 6 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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_pvtin 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_pvtshould 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
Juergen Gross
maintainerRe: [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to David VrabelQuoted 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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().Quoted 6 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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_pvtin 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_pvtshould 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
Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to Juergen GrossQuoted 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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
Quoted 3 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby two after the error case before, even if only one request would have made it to the ring.
Okay, then I may have been mislead by the patch description: I understood it to say that you want to avoid the backend seeing requests which haven't been filled fully, but it looks like you're instead saying that for these requests the filling will never be completed (because of some unrelated(?) error). Iirc other frontend drivers behave similarly to the unpatched scsifront, and incrementing req_prod_pvt late has possible (perhaps just theoretical) other issues, like parallel retrieval and filling of them on mor than one CPU. Wouldn't it be better to obtain a request structure only when everything else is ready (and hence no further errors can occur)? After all you also need to deal with the acquired ID upon errors, and seems odd to me to deal with the two parts of cleanup in different places (and even in different ways).
Jan
Re: [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to Juergen GrossQuoted 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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
Quoted 3 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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]
Juergen Gross
maintainerRe: [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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().Quoted 3 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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_pvtlate 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
Juergen Gross
maintainerRe: [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to a message not in the archiveQuoted 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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_PUSH_REQUESTS_AND_CHECK_NOTIFY().Quoted 3 lines
Sure, but
RING_PUSH_REQUESTS()will copyreq_prod_pvttoreq_prod. In the case corrected this would advancereq_prodby 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_pvtlate 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
Juergen Gross
maintainerRe: [PATCH] xen/scsifront: don't advance ring request pointer in case of error
In reply to a message not in the archiveQuoted 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
-1ring_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. Onlyring->sringfields are shared, and the updating of the shared field happens inRING_PUSH_REQUESTS()andRING_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
Juergen Gross
maintainerPATCH 1/1 xen/scsifront: don't advance ring request pointer in case of error
In reply to Dan CarpenterWhen 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
@@ -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
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
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
-2ring_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
Juergen Gross
maintainerRe: [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_pvtbe 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_grantsis not going to haveVSCSIIF_SG_GRANTbit set?
Absolutely, yes. Can't be larger than VSCSIIF_SG_TABLESIZE which is 26.
Juergen
Juergen Gross
maintainerRe: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready
In reply to Boris OstrovskyQuoted 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_pvtbe 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_grantsis not going to haveVSCSIIF_SG_GRANTbit 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
Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready
In reply to Juergen GrossQuoted 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.
Suggested-by Jan Beulich <[email protected]>Signed-off-by Juergen Gross <[email protected]>
(although it would be good to have someone familiar with this code look at this as well).
Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready
In reply to Juergen GrossQuoted 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.
Suggested-by Jan Beulich <[email protected]>Signed-off-by Juergen Gross <[email protected]>
(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
Juergen Gross
maintainerRe: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready
In reply to Juergen GrossQuoted 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.
Suggested-by Jan Beulich <[email protected]>Signed-off-by Juergen Gross <[email protected]>
Commited to xen/tip.git for-linus-4.10
Juergen
Juergen Gross
maintainerRe: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready
In reply to Juergen GrossQuoted 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.
Suggested-by Jan Beulich <[email protected]>Signed-off-by Juergen Gross <[email protected]>
Commited to xen/tip.git for-linus-4.10
Juergen
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Juergen Gross
maintainerPATCH v2 1/1 xen/scsifront: don't request a slot on the ring until request is ready
In reply to Dan CarpenterInstead 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
@@ -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
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.
Signed-off-by Quentin Lambert <[email protected]>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
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.
Signed-off-by Quentin Lambert <[email protected]>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
Juergen Gross
maintainerQuoted 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
Signed-off-by Quentin Lambert <[email protected]>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
Oops. Sorry for the noise.
regards,
dan carpenter
Juergen Gross
maintainerQuoted 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
Signed-off-by Quentin Lambert <[email protected]>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
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.
Signed-off-by Quentin Lambert <[email protected]>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
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.
Signed-off-by Quentin Lambert <[email protected]>
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(struif (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
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.
Signed-off-by Quentin Lambert <[email protected]>
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(struif (scsifront_enter (info)) {spin_unlock_irq (host->host_lock); +kfree (shadow);return FAILED ; }
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.
Signed-off-by Quentin Lambert <[email protected]>
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(struif (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