Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Steffen Maier <maier@linux.ibm.com>]

Hi Ming,

On 6/26/19 5:07 AM, Ming Lei wrote:
> On Tue, Jun 25, 2019 at 12:51:04PM +0200, Steffen Maier wrote:
>> I don't mind doing this change for zfcp. However, I'm having doubts
>> regarding the rationale in the commit description.

> However, I still suggest to do it because it will make us to audit SCSI chained
> sg uses much easier. And the change shouldn't have performance effect.

>> If I was not mistaken above, the following could be more descriptive parts
>> of a patch/commit description, with hopefully less confusion for anyone
>> having to look at zfcp git history a few weeks/months/years from now:
>>
>> "While not required for this SCSI MQ change regarding scatterlist
>> allocation, change all other scatterlist iterators in zfcp to the safe
>> sg_next() even if not necessary as these changed zfcp-internal scatterlists
>> are linear and unchained. This may avoid confusion about a potential need
>> for conversions in the future."

Sure, let's change the code, but could you please update the description of 
your below new patch to something like I drafted above regarding rationale?

If you would like to send a V2, I'll be happy to give a Reviewed-by.

>>>   From c9c368308fefbf034d670984fe9746a4181fe514 Mon Sep 17 00:00:00 2001
>>> From: Ming Lei <ming.lei@redhat.com>
>>> Date: Tue, 25 Jun 2019 09:15:34 +0800
>>> Subject: [PATCH] s390: scsi: use sg helper to iterate over scatterlist

>>> Unlike the legacy I/O path, scsi-mq preallocates a large array to hold
>>> the scatterlist for each request. This static allocation can consume
>>> substantial amounts of memory on modern controllers which support a
>>> large number of concurrently outstanding requests.

>>> To facilitate a switch to a smaller static allocation combined with a
>>> dynamic allocation for requests that need it, we need to make sure all
>>> SCSI drivers handle chained scatterlists correctly.

>>> Convert remaining drivers that directly dereference the scatterlist
>>> array to using the iterator functions.

> OK, I still suggest to apply the patch for the mentioned reason if you
> are fine.



-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294