From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sagi Grimberg Subject: Re: [PATCH 8/8] IB/srp: Add multichannel support Date: Wed, 24 Sep 2014 15:22:35 +0300 Message-ID: <5422B78B.4000501@dev.mellanox.co.il> References: <541C27BF.6070609@acm.org> <541C28E0.7010705@acm.org> <5421A093.1070203@dev.mellanox.co.il> <5421C3DF.5000102@acm.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5421C3DF.5000102@acm.org> Sender: linux-scsi-owner@vger.kernel.org To: Bart Van Assche , "linux-scsi@vger.kernel.org" Cc: linux-rdma , Christoph Hellwig , Jens Axboe , Robert Elliott , Ming Lei List-Id: linux-rdma@vger.kernel.org On 9/23/2014 10:02 PM, Bart Van Assche wrote: > On 23/09/2014 10:32, Sagi Grimberg wrote: >> On 9/19/2014 4:00 PM, Bart Van Assche wrote: >>> Improve performance by using multiple RDMA/RC channels per SCSI host >>> for communicating with an SRP target. >>> >> >> Hey Bart, >> >> Since you don't seem to negotiate/declare multichannel with the target, >> did you test this code with some target implementations other than SCST >> that happen to be out there? >> >> Overall, I think this patch would be easier to review if you also >> provide a list of logical changes (which obviously are introduced in >> this patch). Patch 7/8 can use some more information of target-channel >> relations as well. > > Hello Sagi, > > That's a good question. So far this patch series has only been tested > against the SCST SRP target driver. However, as you probably noticed, if > setting up a second or later RDMA channel fails SRP login is not failed > but communication proceeds with the number of channels that have been > established. This mechanism should retain backwards compatibility with > SRP target systems that do not support multichannel communication. > However, if the new code for SRP login turns out to be triggering bugs > in existing SRP target implementations we can still add a blacklist for > these implementations. I'm more concerned that a target will accept multichannel and then starts flipping since that wasn't tested in I don't know when, probably never... Since SRP_LOGIN_REQ/RESP has some free bits why not declare it and activate it when both sides *says* they support it? I'd be much calmer knowing we're on the safe side on this... > > I will provide a more detailed list of logical changes in the second > version of this patch series. Thanks, Plus, I would like to run it on my performance setups. can you point me to the SCST repo? is multichannel supported in scst trunk? Sagi.