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

From: James Smart <jsmart2021@gmail.com>
To: Benjamin Block <bblock@linux.ibm.com>,
	Muneendra Kumar M <muneendra.kumar@broadcom.com>
Cc: 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: Thu, 22 Apr 2021 16:29:10 -0700	[thread overview]
Message-ID: <2eec720b-401a-32b4-80e6-900e136939b3@gmail.com> (raw)
In-Reply-To: <YH62VB+SVfnG+GoI@t480-pf1aa2c2.linux.ibm.com>

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. 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.  Guess we need to get going on that merged 
transport...

-- james