Re: [PATCH v9 03/13] nvme: Added a newsysfs attribute appid_store

From: Benjamin Block <bblock@linux.ibm.com>
To: James Smart <jsmart2021@gmail.com>
Cc: Muneendra Kumar M <muneendra.kumar@broadcom.com>,
	Benjamin Block <lkml@mageta.org>,
	linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	tj@kernel.org, linux-nvme@lists.infradead.org, hare@suse.de,
	emilne@redhat.com, mkumar@redhat.com,
	Steffen Maier <maier@linux.ibm.com>
Subject: Re: [PATCH v9 03/13] nvme: Added a newsysfs attribute appid_store
Date: Fri, 23 Apr 2021 12:14:53 +0200	[thread overview]
Message-ID: <YIKeHaCpbWwJT6Lh@t480-pf1aa2c2.linux.ibm.com> (raw)
In-Reply-To: <2eec720b-401a-32b4-80e6-900e136939b3@gmail.com>

On Thu, Apr 22, 2021 at 04:29:10PM -0700, James Smart wrote:
> On 4/20/2021 4:09 AM, Benjamin Block wrote:
> > On Tue, Apr 20, 2021 at 12:24:41PM +0530, Muneendra Kumar M wrote:
> > > Hi Benjamin,
> > > 
> > > > > ---
> > > > >   drivers/nvme/host/fc.c | 73
> > > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > >   1 file changed, 72 insertions(+), 1 deletion(-)
> > > 
> > > > Hmm, I wonder why only NVMe-FC? Or is this just for the moment? We also
> > > > have the FC transport class for SCSI; I assume this could feed the same
> > > > IDs into the LLDs.
> > > 
> > > At present it supports only for SCSI-FC .
> > 
> > It does? By adding it to the implementation under `drivers/nvme/host/`?
> > I am confused.
> 
> Yeah, this is a little odd.
> 
> This history is: we know we need to merge the scsi fc transport and the nvme
> fc transport as the two independent things are creating difficulties and
> duplications (devloss is an example). But it's a bit of work to change this
> as it will move where the objects are in the topology tree.
> 
> As we tried to figure out how to do the interaction with cgroups, we wanted
> to support SCSI and nvme. If you look at how this new attribute sets the
> vmid, it's somewhat generic - it just sets the fc appid field on a cgrp id.
> There's really nothing that says the cgrp is on a block device that is scsi
> or is nvme.  It should work on devices regardless of their underlying
> protocol type. Which then left the question - where to place such an
> attribute.
> 
> Given this is an "fc service" and as we knew the transport will be merged
> eventually we picked to put it under /sys/class/fc point which is where we
> expect to root the common transport. 

Ah ok, I think I confused it with the already existing
`/sys/class/fc_host`/`/sys/class/fc_transport/`/..., but thats something
different. Yeah, having some common ancestor makes sense, if the HBA
offers multiple ULPs.

> This class point happens to be owned/created by the nvme fc host code
> for requesting nve-fc rediscovery events. It is odd that it creates a
> requirement to load the nvme-fc transport in order to set values for
> scsi fc devices, but we deemed it acceptable.

Well, I mean, right now I don't have a immediate need for zfcp in this
regard, so I don't want to blow this out of proportions. But like I
said, we (zfcp) don't have a NVMe personality, so we never hook into
NVMe-FC.

> Guess we need to get going on that merged transport...

Feel free to reach out, if there is anything coming up.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294