[Patch]: pvSCSI (Re: [Xen-devel] Minor synchronisation quibble in scsifront)

[Jun Kamada <kama@xxxxxxxxxxxxxx>]

Hi Steven-san,

Thank you for your advice.
I will attach a patch to solve the lock problem. And also, the patch
contains few modifications about SCSI Reset, which you pointed out in
the past.

Best regards,


Signed-off-by: Tomonari Horikoshi <t.horikoshi@xxxxxxxxxxxxxx>
Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>


On Thu, 10 Jul 2008 16:26:56 +0100
Steven Smith <steven.smith@xxxxxxxxxx> wrote:

> I've been having a look through scsifront again, and I saw this bit:
> 
>       ring_req->timeout_per_command = (sc->timeout_per_command / HZ);
>       ring_req->nr_segments         = 0;
> 
>       spin_unlock_irq(host->host_lock);
>       scsifront_do_request(info);     
>       wait_event_interruptible(info->shadow[ring_req->rqid].wq_reset,
>                        info->shadow[ring_req->rqid].wait_reset);
> 
> in scsifront_dev_reset_handler().  Looking at scsifront_do_request():
> 
> static void scsifront_do_request(struct vscsifrnt_info *info)
> {
>       struct vscsiif_front_ring *ring = &(info->ring);
>       unsigned int irq = info->irq;
>       int notify;
> 
>       RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
>       if (notify)
>               notify_remote_via_irq(irq);
> }
> 
> scsifront_do_request() is also called from scsifront_queuecommand(),
> where it's under the host lock.  I can't see any other relevant
> synchronisation, so I think that you might be able to end up with
> several processors pushing requests at the same time.  I'm not sure
> what'll happen then, but I doubt it's a good idea.
> 
> The issue could be avoided if you just swapped two lines in
> scsifront_dev_reset_handler():
> 
>       ring_req->timeout_per_command = (sc->timeout_per_command / HZ);
>       ring_req->nr_segments         = 0;
> 
>       scsifront_do_request(info);     
>       spin_unlock_irq(host->host_lock);
>       wait_event_interruptible(info->shadow[ring_req->rqid].wq_reset,
>                        info->shadow[ring_req->rqid].wait_reset);
> 
> Does that sound sane?
> 
> On the plus side, that's the only strange bit I could see in current
> scsifront. :)
> 
> Steven.


-----
Jun Kamada

Attachment: linux_pvscsi_scsifront_fix_synchronisation_quibble.patch
Description: Binary data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel