Re: [RFC v1 0/3] Unifying fabrics drivers

[James Smart <jsmart2021@gmail.com>]

On 3/1/2023 12:27 AM, Daniel Wagner wrote:
> The two fabrics rdma and tcp share a lot of common code. This here is my attempt
> to consolidate the common code.
> 
> I've picked just one function (setup admin queue) for this RFC to get a feeling
> and feedback if this is a valid approach or if people hate it. I've left out fc
> for the time being because it differs too much two the other two drivers.
> 
> I've tested quickly tcp, rdma is only compile tested.
> 
> Daniel Wagner (3):
>    nvme-rdma: stream line queue functions arguments
>    nvme-rdma: factor rdma specific queue init code out
>    nvme-fabrics: move configure admin queue code to fabrics.c
> 
>   drivers/nvme/host/fabrics.c |  52 ++++++++++++++
>   drivers/nvme/host/fabrics.h |  12 ++++
>   drivers/nvme/host/nvme.h    |   3 +
>   drivers/nvme/host/rdma.c    | 132 +++++++++++++++++++-----------------
>   drivers/nvme/host/tcp.c     |  94 ++++++++-----------------
>   5 files changed, 165 insertions(+), 128 deletions(-)
> 

Initial reaction:
The resulting nvmf_configure_admin_queue() routine does look nice. 
However, I'm not sure the callback chain is all that easier to follow.

I guess I'm most disappointed that more of the logic isn't actually 
being commonized.

Examples:
alloc_queue():
   At least pass queue_size as an arg. I assume we should to match up
   with the fabric connect routine.
init_queue():
   I'd like to see setting of ctrl.max_segments/hw_sectors in the common
   code. Unfortunately, likely needs to be a fops call. At least it
   makes sure its done at the same point in controller creation.
start_queue():
   can't common code do the nvmf_connect_admin_queue() call ?
      If its too tied to the LIVE bit - then perhaps the queue LIVE
      bits should actually be getting set by the common code after
      the successful start_queue - or has routines transport can call
      from start/stop to set/clear it.
   If LIVE bit can be dealt with - this whole block in rdma/tcp moves to
   common code.

Also I'd really like to get rid of the "new" argument everywhere. It 
should just be a state flag set on the common ctrl.

I guess not bad, but not helping much (yet).

-- james