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 Also make sure you refresh James's scsi misc tree when you make the new patchset. Some be2iscsi patches where merged to that driver that conflict with you patches. -- 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.
