Received-SPF: pass (google.com: domain of linux-kernel+bounces-187773-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f1f52715-78b3-42e1-bc25-9984dd178321@redhat.com>
Date: Thu, 23 May 2024 12:07:58 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5] nvme: multipath: Implemented new iopolicy
 "queue-depth"
To: Christoph Hellwig <hch@lst.de>
Cc: kbusch@kernel.org, sagi@grimberg.me, emilne@redhat.com,
 linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
 jrani@purestorage.com, randyj@purestorage.com, hare@kernel.org
References: <20240522165406.702362-1-jmeneghi@redhat.com>
 <20240523081443.GD1086@lst.de>
Content-Language: en-US
From: John Meneghini <jmeneghi@redhat.com>
Organization: RHEL Core Storge Team
In-Reply-To: <20240523081443.GD1086@lst.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/23/24 04:14, Christoph Hellwig wrote:
> Oh, and there is really something of with the patch format.
> 
> First the subject line is completely wrong, this should be:
> 
> nvme-multipath: implement "queue-depth" iopolicy.

Will fix.

> On Wed, May 22, 2024 at 12:54:06PM -0400, John Meneghini wrote:
>> From: "Ewan D. Milne" <emilne@redhat.com>
> 
>> Signed-off-by: Thomas Song <tsong@purestorage.com>
>> [emilne: patch developed by Thomas Song @ Pure Storage, fixed whitespace
>>        and compilation warnings, updated MODULE_PARM description, and
>>        fixed potential issue with ->current_path[] being used]
>> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> 
> Second the patch author needs to be the first signoff.  I'm not entirely
> sure who is supposed to be the author, but either it should be Thomas,
> or if the patch has changed so much that it's someone else the original
> signoff should turn into a Co-Developed-by.

This patch was originally authored by Thomas Song, an intern who worked at Purestorage. The original patch was almost a year 
ago. Thomas no longer works for Purestorage, and he is no where to be found. Ewan and I have both made many changes since then, 
but the basic concept, which adds a controller scoped atomic counter in nvme_find_path(), remains the same. Ewan worked on the 
patch for several months, trying different counter implementations, all in an effort to avoid the atomic counter.  However, 
nothing else worked and in the end this patch retains the essential features of Thomas Songs original patch.

I'd like Thomas to get credit for this patch, so I've changed the author from Ewan Milne to Thomas Song.

As for the Co-Developed-By: checkpatch.pl is insisting that that each "Co-developed-by:" be followed by a signoff.

   jmeneghi:linux > scripts/checkpatch.pl --git HEAD
   WARNING: Co-developed-by: must be immediately followed by Signed-off-by:
   #26:
   Co-developed-by: Ewan D. Milne <emilne@redhat.com>
   [jmeneghi: vairious changes and improvements, addressed review comments]

   WARNING: Co-developed-by: must be immediately followed by Signed-off-by:
   #28:
   Co-developed-by: John Meneghini <jmeneghi@redhat.com>
   Link: https://lore.kernel.org/linux-nvme/20240509202929.831680-1-jmeneghi@redhat.com/

   total: 0 errors, 2 warnings, 197 lines checked


So this is what I currently have for a message log.

Author: Thomas Song <tsong@purestorage.com>
Date:   Tue Nov 7 16:23:29 2023 -0500

     nvme-multipath: implemented new iopolicy "queue-depth"

     The round-robin path selector is inefficient in cases where there is a
     difference in latency between paths.  In the presence of one or more
     high latency paths the round-robin selector continues to use the high
     latency path equally. This results in a bias towards the highest latency
     path and can cause a significant decrease in overall performance as IOs
     pile on the highest latency path. This problem is acute with NVMe-oF
     controllers.

     The queue-depth policy instead sends I/O requests down the path with the
     least amount of requests in its request queue. Paths with lower latency
     will clear requests more quickly and have less requests in their queues
     compared to higher latency paths. The goal of this path selector is to
     make more use of lower latency paths which will bring down overall IO
     latency and increase throughput and performance.

     Signed-off-by: Thomas Song <tsong@purestorage.com>
     [emilne: patch developed by Thomas Song @ Pure Storage, fixed whitespace
           and compilation warnings, updated MODULE_PARM description, and
           fixed potential issue with ->current_path[] being used]
     Co-developed-by: Ewan D. Milne <emilne@redhat.com>
     Signed-off-by: Ewan D. Milne <emilne@redhat.com>
     [jmeneghi: vairious changes and improvements, addressed review comments]
     Co-developed-by: John Meneghini <jmeneghi@redhat.com>
     Signed-off-by: John Meneghini <jmeneghi@redhat.com>
     Link: https://lore.kernel.org/linux-nvme/20240509202929.831680-1-jmeneghi@redhat.com/
     Tested-by: Marco Patalano <mpatalan@redhat.com>
     Reviewed-by: Randy Jennings <randyj@purestorage.com>
     Tested-by: Jyoti Rani <jrani@purestorage.com>