Re: [PATCH v1 1/3] IB/srp: Fix crash when unmapping data loop

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

On 03/11/14 14:38, Sagi Grimberg wrote:
>> And before the SCSI host lock push-down the SCSI host lock was
>> held around the srp_queuecommand() invocation. In other words, holding a
>> spin lock around the srp_post_send() call in srp_queuecommand() is
>> neither new nor disallowed by the RDMA API documentation.
> 
> I just noted that I think it should be avoided due to performance reasons.
> We are going towards scsi-mq and I suspect fast-path spinlocks will
> become much
> more expensive in this era, and we should try to avoid them rather than
> take them.

Once the SRP initiator driver will support multiple queues the lock
around srp_post_send() will be per queue instead of per target so if the
number of queues is large enough that lock won't be a contention point.
I have experimental code ready that follows this approach.

>> Holding
>> target->lock around the srp_post_send() call and the req->scmnd
>> assignment is only needed to avoid the (theoretical) race where
>> processing of an SRP reply would already have been started before
>> req->scmnd has been assigned by srp_queuecommand().
> 
> I see... will assigning scmnd = NULL only if srp_post_send will fail,
> and restore the assignment as before help?

Sorry but I'm not enthusiast about that approach because it could result
in resources being freed while srp_post_send() is in progress. I prefer
to avoid this kind of race conditions.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html