Re: [PATCH-v2 11/12] xen-scsiback: Convert to percpu_ida tag allocation

[Juergen Gross <jgross@suse.com>]

On 27/01/16 07:28, Nicholas A. Bellinger wrote:
> On Tue, 2016-01-26 at 10:45 +0100, Juergen Gross wrote:
>> On 25/01/16 09:11, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Hannes Reinecke <hare@suse.de>
>>> Cc: David Vrabel <david.vrabel@citrix.com>
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>>  drivers/xen/xen-scsiback.c | 163 ++++++++++++++++++++++++---------------------
>>>  1 file changed, 87 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
>>> index 594f8a7..640fb22 100644
>>> --- a/drivers/xen/xen-scsiback.c
>>> +++ b/drivers/xen/xen-scsiback.c
>>> @@ -190,7 +190,6 @@ module_param_named(max_buffer_pages, scsiback_max_buffer_pages, int, 0644);
>>>  MODULE_PARM_DESC(max_buffer_pages,
>>>  "Maximum number of free pages to keep in backend buffer");
>>>  
>>> -static struct kmem_cache *scsiback_cachep;
>>>  static DEFINE_SPINLOCK(free_pages_lock);
>>>  static int free_pages_num;
>>>  static LIST_HEAD(scsiback_free_pages);
>>> @@ -322,7 +321,8 @@ static void scsiback_free_translation_entry(struct kref *kref)
>>>  }
>>>  
>>>  static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
>>> -			uint32_t resid, struct vscsibk_pend *pending_req)
>>> +			uint32_t resid, struct vscsibk_pend *pending_req,
>>> +			uint16_t rqid)
>>>  {
>>>  	struct vscsiif_response *ring_res;
>>>  	struct vscsibk_info *info = pending_req->info;
>>
>> pending_req might be NULL now, so this will panic the system.
>>
> 
> Thanks for the review.
> 
> Added the following to propagate up original *info into
> scsiback_do_resp_with_sense() to address the early pending_req
> failure case.
> 
> https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=queue-next&id=5873f22a9b7c7aa16ff9a85074a07b739f1d06a5

Hmm, wouldn't it make more sense to split scsiback_do_resp_with_sense()
into two functions now? Something like:


diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..0d71467 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -321,11 +321,10 @@ static void scsiback_free_translation_entry(struct
kref *kref)
        kfree(entry);
 }

-static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
-                       uint32_t resid, struct vscsibk_pend *pending_req)
+static void scsiback_send_response(struct vscsibk_info *info,
+       char *sense_buffer, int32_t result, uint32_t resid, uint16_t rqid)
 {
        struct vscsiif_response *ring_res;
-       struct vscsibk_info *info = pending_req->info;
        int notify;
        struct scsi_sense_hdr sshdr;
        unsigned long flags;
@@ -337,7 +336,7 @@ static void scsiback_do_resp_with_sense(char
*sense_buffer, int32_t result,
        info->ring.rsp_prod_pvt++;

        ring_res->rslt   = result;
-       ring_res->rqid   = pending_req->rqid;
+       ring_res->rqid   = rqid;

        if (sense_buffer != NULL &&
            scsi_normalize_sense(sense_buffer, VSCSIIF_SENSE_BUFFERSIZE,
@@ -357,6 +356,13 @@ static void scsiback_do_resp_with_sense(char
*sense_buffer, int32_t result,

        if (notify)
                notify_remote_via_irq(info->irq);
+}
+
+static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
+                       uint32_t resid, struct vscsibk_pend *pending_req)
+{
+       scsiback_send_response(pending_req->info, sense_buffer, result,
+                              resid, pending_req->rqid);

        if (pending_req->v2p)
                kref_put(&pending_req->v2p->kref,


And then call scsiback_send_response() directly in case pending_req
is NULL.


Juergen