On 10/23/2013 11:29 AM, Shlomo Pongratz wrote: > On 10/23/2013 7:12 PM, Mike Christie wrote: >> On 10/23/2013 11:07 AM, Mike Christie wrote: >>> On 10/23/2013 06:16 AM, Shlomo Pongratz wrote: >>>>> -----Original Message----- >>>>> From: Mike Christie [mailto:[email protected]] >>>>> Sent: Monday, October 21, 2013 7:06 PM >>>>> To: Or Gerlitz >>>>> Cc: [email protected]; Roi Dayan; Shlomo Pongratz >>>>> Subject: Re: [PATCH RFC V1 2/3] SCSI/libiscsi: Reduce locking >>>>> contention in >>>>> fast path >>>>> >>>>> On 09/17/2013 05:43 AM, Or Gerlitz wrote: >>>>>> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index >>>>>> 6ac9e17..783f031 100644 >>>>>> --- a/include/scsi/libiscsi.h >>>>>> +++ b/include/scsi/libiscsi.h >>>>>> @@ -326,12 +326,15 @@ struct iscsi_session { >>>>>> struct iscsi_transport *tt; >>>>>> struct Scsi_Host *host; >>>>>> struct iscsi_conn *leadconn; /* leading connection */ >>>>>> - spinlock_t lock; /* protects session state, * >>>>>> - * sequence numbers, * >>>>>> + spinlock_t frwd_lock; /* protects session state, * >>>>>> + * cmdsn, queued_cmdsn * >>>>>> * session resources: * >>>>>> - * - cmdpool, * >>>>>> + * - cmdpool kfifo_out , * >>>>>> * - mgmtpool, * >>>>>> * - r2tpool */ >>>>>> + spinlock_t back_lock; /* protects cmdsn_exp * >>>>>> + * cmdsn_max, * >>>>>> + * cmdpool kfifo_in */ >>>>> 1. fix up the comment names like above. cmdsn_exp should be exp_cmdsn. >>>>> Also review that we even need a lock for max/exp cmdsns. >>>>> >>>>> 2. There are references to the session->lock in the code still. Fix >>>>> those up. >>>>> >>>>> 3. Add some comments above about locking hierarchy. For example we >>>>> grab >>>>> the frwd_lock then call fail_scsi_task which grabs the back_lock >>>>> when calling >>>>> fail_scsi_task*. >>>>> >>>>> 4. Handle review comment from other mail about removing useless >>>>> chunk of >>>>> code. >>>>> >>>>> >>>>> That's all looks good. Thanks for working on it and being patient >>>>> with reviews >>>>> and also staying on top of getting me to review it. >>>>> >>>>> The iscsi tcp r2t changes are ok too. I want to fix them a >>>>> different way, once >>>>> we actually implement multiple r2t support for that driver. >>>>> Today all that code does not seem useful for just single r2t support. >>>> Hi Mike, >>>> >>>> Regarding the reference to "session->lock" I've found such >>>> references only in "drivers/scsi/scsi_transport_iscsi.c". >>>> Please note that the so called "session" there, is of type >>>> "iscsi_cls_session" and not of type "iscsi_session". >>>> Since it is not part of the data path I don't think we should bother >>>> with this lock. >>>> >>> Check for "session lock" too: >>> >>> iscsi_free_task >>> iscsi_complete_scsi_task >>> fail_scsi_task >>> iscsi_itt_to_task >>> __iscsi_complete_pdu >>> iscsi_itt_to_ctask >>> iscsi_requeue_task >>> fail_scsi_tasks >>> iscsi_suspend_queue >> One more >> >> iscsi_tcp_cleanup_task >> > Hi Mike, > > O.K. I'll rebase to Jame's "misc" branch (used to work on his "for-next" > branch). > > All the "session->lock" were the session was of type "struct > iscsi_session" were replaced by frwd and back lock.
I am not sure what you are saying. I think when I wrote code in the first comment I meant code and comments. I just want you to change the comments in the above functions so instead of having something like: /** * iscsi_free_task - free a task * @task: iscsi cmd task * * Must be called with session lock. It now says the new lock that needs to be grabbed. In the case for iscsi_free_task it seems either one can be taken depending on the context. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/groups/opt_out.
