| Message ID | 1617750397-26466-4-git-send-email-muneendra.kumar@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | blkcg:Support to track FC storage blk io traffic | expand |
On Wed, Apr 07, 2021 at 04:36:27AM +0530, Muneendra wrote: > Added a new sysfs attribute appid_store under > /sys/class/fc/fc_udev_device/* > > With this new interface the user can set the application identfier > in the blkcg associted with cgroup id. > > Once the application identifer has set with this interface it allows > identification of traffic sources at an individual cgroup based > Applications (ex:virtual machine (VM))level in both host and > fabric infrastructure(FC). > > Below is the interface provided to set the app_id > > echo "<cgroupid>:<appid>" >> /sys/class/fc/fc_udev_device/appid_store > echo "457E:100000109b521d27" >> /sys/class/fc/fc_udev_device/appid_store > > Reviewed-by: Hannes Reinecke <hare@suse.de> > Signed-off-by: Muneendra <muneendra.kumar@broadcom.com> > > --- > v9: > No change > > v8: > No change > > v7: > No change > > v6: > No change > > v5: > Replaced APPID_LEN with FC_APPID_LEN > > v4: > No change > > v3: > Replaced blkcg_set_app_identifier function with blkcg_set_fc_appid > > v2: > New Patch > --- > 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.
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 . In future we are adding the support for NVMe-FC But to make it generic and avoid duplication we added this under /sys/class/fc . Ewan was mentioning that at some point there is a plan to decouple the FC transport somewhat so that there is a layer that represents the FC stuff regardless of the FC4 type (SCSI, NVMe). When we have this layer we can move the things accordingly. Regards, Muneendra. -- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.
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. I see it adds the sysfs-attribute to `nvme_fc_attrs`, how would that be added to a FC Host that does not have a NVMe 'personality'? I was assuming this only ever appears under `/sys/class/fc` if the LLDD registers itself with the NVMe subsystem (presumably via `nvme_fc_register_localport()`). zFCP, for example, does not do that, but we do implement the SCSI FC transport class in `drivers/scsi/scsi_transport_fc.c`. > In future we are adding the support for NVMe-FC > But to make it generic and avoid duplication we added this under > /sys/class/fc . > > Ewan was mentioning that at some point there is a plan to decouple > the FC transport somewhat so that there is a layer that represents the > FC stuff regardless of the FC4 type (SCSI, NVMe). When we have this > layer we can move the things accordingly. > -- 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
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
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
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index 20dadd86e981..f0ce876700d6 100644 --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -9,7 +9,7 @@ #include <uapi/scsi/fc/fc_els.h> #include <linux/delay.h> #include <linux/overflow.h> - +#include <linux/blk-cgroup.h> #include "nvme.h" #include "fabrics.h" #include <linux/nvme-fc-driver.h> @@ -3782,10 +3782,81 @@ static ssize_t nvme_fc_nvme_discovery_store(struct device *dev, return count; } + +/*parse the Cgroup id from a buf and returns the length of cgrpid*/ +static int fc_parse_cgrpid(const char *buf, u64 *id) +{ + char cgrp_id[16+1]; + int cgrpid_len, j; + + memset(cgrp_id, 0x0, sizeof(cgrp_id)); + for (cgrpid_len = 0, j = 0; cgrpid_len < 17; cgrpid_len++) { + if (buf[cgrpid_len] != ':') + cgrp_id[cgrpid_len] = buf[cgrpid_len]; + else { + j = 1; + break; + } + } + if (!j) + return -EINVAL; + if (kstrtou64(cgrp_id, 16, id) < 0) + return -EINVAL; + return cgrpid_len; +} + +/* + * fc_update_appid :parses and updates the appid in the blkcg associated with + * cgroupid. + * @buf: buf contains both cgrpid and appid info + * @count: size of the buffer + */ +static int fc_update_appid(const char *buf, size_t count) +{ + u64 cgrp_id; + int appid_len = 0; + int cgrpid_len = 0; + char app_id[FC_APPID_LEN]; + int ret = 0; + + if (buf[count-1] == '\n') + count--; + + if ((count > (16+1+FC_APPID_LEN)) || (!strchr(buf, ':'))) + return -EINVAL; + + cgrpid_len = fc_parse_cgrpid(buf, &cgrp_id); + if (cgrpid_len < 0) + return -EINVAL; + /*appid len is count - cgrpid_len -1 (: + \n) */ + appid_len = count - cgrpid_len - 1; + if (appid_len > FC_APPID_LEN) + return -EINVAL; + + memset(app_id, 0x0, sizeof(app_id)); + memcpy(app_id, &buf[cgrpid_len+1], appid_len); + ret = blkcg_set_fc_appid(app_id, cgrp_id, sizeof(app_id)); + if (ret < 0) + return ret; + return count; +} + +static ssize_t fc_appid_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t count) +{ + int ret = 0; + + ret = fc_update_appid(buf, count); + if (ret < 0) + return -EINVAL; + return count; +} static DEVICE_ATTR(nvme_discovery, 0200, NULL, nvme_fc_nvme_discovery_store); +static DEVICE_ATTR(appid_store, 0200, NULL, fc_appid_store); static struct attribute *nvme_fc_attrs[] = { &dev_attr_nvme_discovery.attr, + &dev_attr_appid_store.attr, NULL };