Xen devel - Jul 2008 - Minor synchronisation quibble in scsifront

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.


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

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@jp.fujitsu.com>
Signed-off-by: Jun Kamada <kama@jp.fujitsu.com>


On Thu, 10 Jul 2008 16:26:56 +0100
Steven Smith <steven.smith@citrix.com> 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


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

> 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.
Thanks, that looks better.

Steven.


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