Re: [PATCH v4 02/19] blkcg: Added a app identifier support for blkcg

From: Tejun Heo <tj@kernel.org>
To: Muneendra <muneendra.kumar@broadcom.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-nvme@lists.infradead.org, jsmart2021@gmail.com,
	emilne@redhat.com, mkumar@redhat.com, pbonzini@redhat.com
Subject: Re: [PATCH v4 02/19] blkcg: Added a app identifier support for blkcg
Date: Mon, 9 Nov 2020 08:55:19 -0500	[thread overview]
Message-ID: <20201109135519.GD7496@mtj.duckdns.org> (raw)
In-Reply-To: <1604895845-2587-3-git-send-email-muneendra.kumar@broadcom.com>

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