Again, some nits.
On Mon, Nov 09, 2020 at 09:53:48AM +0530, Muneendra wrote:
> This Patch added a unique application identifier i.e
> app_id knob to blkcg which allows identification of traffic
> sources at an individual cgroup based Applications
> (ex:virtual machine (VM))level in both host and
> fabric infrastructure.
>
> Provided the interface blkcg_get_fc_appid to
> grab the app identifier associated with a bio.
>
> Provided the interface blkcg_set_fc_appid to
> set the app identifier in a blkcgrp associated with cgroup id
>
> Added a new config BLK_CGROUP_FC_APPID and moved the changes
> under this config
Please reflow the paragraphs.
> +config BLK_CGROUP_FC_APPID
> + bool "Enable support to track FC io Traffic across cgroup applications"
> + depends on BLK_CGROUP=y
> + help
> + Enabling this option enables the support to track FC io traffic across
> + cgroup applications.It enables the Fabric and the storage targets to
^
space
> + identify, monitor, and handle FC traffic based on vm tags by inserting
> + application specific identification into the FC frame.
...
> /* Max limits for throttle policy */
> #define THROTL_IOPS_MAX UINT_MAX
> +#define APPID_LEN 128
Can you add the FC prefix for the above too?
> +#ifdef CONFIG_BLK_CGROUP_FC_APPID
> +/*
/**
> + * Sets the fc_app_id field associted to blkcg
> + * @buf: application identifier
> + * @id: cgrp id
> + * @len: size of appid
> + */
> +static inline int blkcg_set_fc_appid(char *buf, u64 id, size_t len)
I find the arguments really confusing, how about (@cgrp_id, @app_id,
@app_id_len)?
> +{
> + struct cgroup *cgrp = NULL;
> + struct cgroup_subsys_state *css = NULL;
> + struct blkcg *blkcg = NULL;
No need to set these to NULL.
> + int ret = 0;
> +
> + cgrp = cgroup_get_from_kernfs_id(id);
> + if (!cgrp)
> + return -ENOENT;
> + css = cgroup_get_e_css(cgrp, &io_cgrp_subsys);
> + if (!css) {
> + ret = -ENOENT;
> + goto out_cgrp_put;
> + }
> + blkcg = css_to_blkcg(css);
> + if (!blkcg) {
> + ret = -ENOENT;
> + goto out_put;
> + }
css_to_blkcg() can't fail. It's a simple pointer cast.
> + if (len > APPID_LEN) {
> + ret = -EINVAL;
> + goto out_put;
> + }
This is input validation. Maybe do this at the beginning?
> + strlcpy(blkcg->fc_app_id, buf, len);
> +out_put:
> + css_put(css);
> +out_cgrp_put:
> + cgroup_put(cgrp);
> + return ret;
> +}
> +
> +/**
> + * blkcg_get_fc_appid - grab the app identifier associated with a bio
> + * @bio: target bio
> + *
> + * This returns the app identifier associated with a bio,
> + * %NULL if not associated.
> + * Callers are expected to either handle %NULL or know association has been
> + * done prior to calling this.
Reflow.
> + */
> +static inline char *blkcg_get_fc_appid(struct bio *bio)
> +{
> + if (bio && bio->bi_blkg &&
> + strlen(bio->bi_blkg->blkcg->fc_app_id))
You can test fc_app_id[0] against '\0' instead of scanning the whole string.
> + return bio->bi_blkg->blkcg->fc_app_id;
> + return NULL;
Thanks.
--
tejun
