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

[Hannes Reinecke <hare@suse.de>]

On 11/9/20 5:23 AM, 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
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v4:
> No change
> 
> v3:
> Renamed the functions and app_id to more specific
> 
> Addressed the reference leaks in blkcg_set_app_identifier
> 
> Added a new config BLK_CGROUP_FC_APPID and moved the changes
> under this config
> 
> Added blkcg_get_fc_appid,blkcg_set_fc_appid as inline functions
> 
> v2:
> renamed app_identifier to app_id
> removed the  sysfs interface blkio.app_identifie under
> ---
>   block/Kconfig              |  9 ++++++
>   include/linux/blk-cgroup.h | 65 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 74 insertions(+)
> 
> diff --git a/block/Kconfig b/block/Kconfig
> index bbad5e8bbffe..ed22df654ce5 100644
> --- a/block/Kconfig
> +++ b/block/Kconfig
> @@ -144,6 +144,15 @@ config BLK_CGROUP_IOLATENCY
>   
>   	Note, this is an experimental interface and could be changed someday.
>   
> +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
> +	identify, monitor, and handle FC traffic based on vm tags by inserting
> +	application specific identification into the FC frame.
> +

Please replace 'io' with 'I/O'.

>   config BLK_CGROUP_IOCOST
>   	bool "Enable support for cost model based cgroup IO controller"
>   	depends on BLK_CGROUP=y
> diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
> index c8fc9792ac77..00ea1cfa3420 100644
> --- a/include/linux/blk-cgroup.h
> +++ b/include/linux/blk-cgroup.h
> @@ -30,6 +30,8 @@
>   
>   /* Max limits for throttle policy */
>   #define THROTL_IOPS_MAX		UINT_MAX
> +#define APPID_LEN              128
> +
>   
>   #ifdef CONFIG_BLK_CGROUP
>   
> @@ -55,6 +57,9 @@ struct blkcg {
>   	struct blkcg_policy_data	*cpd[BLKCG_MAX_POLS];
>   
>   	struct list_head		all_blkcgs_node;
> +#ifdef CONFIG_BLK_CGROUP_FC_APPID
> +	char                            fc_app_id[APPID_LEN];
> +#endif
>   #ifdef CONFIG_CGROUP_WRITEBACK
>   	struct list_head		cgwb_list;
>   #endif

If this is an UUID, wouldn't it be better to use 'uuid_t' here?

> @@ -660,4 +665,64 @@ static inline void blk_cgroup_bio_start(struct bio *bio) { }
>   
>   #endif	/* CONFIG_BLOCK */
>   #endif	/* CONFIG_BLK_CGROUP */
> +
> +#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)
> +{
> +	struct cgroup *cgrp = NULL;
> +	struct cgroup_subsys_state *css = NULL;
> +	struct blkcg *blkcg = 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;
> +	}
> +	if (len > APPID_LEN) {
> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +	strlcpy(blkcg->fc_app_id, buf, len);

String or UUID?
And will it be NULL-terminated?
Wouldn't it be better to use 'memcpy' here?
And it is NULL terminated: what happens if the user sets a 128-byte 
string? IE shouldn't the app_id be 129 bytes long if it's assumed to be 
NULL-terminated?

> +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.
> + */
> +static inline char *blkcg_get_fc_appid(struct bio *bio)
> +{
> +	if (bio && bio->bi_blkg &&
> +			strlen(bio->bi_blkg->blkcg->fc_app_id))
> +		return bio->bi_blkg->blkcg->fc_app_id;
> +	return NULL;
> +}

Same question as above: Is the string NULL terminated or not?
And if not, how do you specify the length of the 'app_id' variable?

> +#else
> +static inline int blkcg_set_fc_appid(char *buf, u64 id, size_t len) { return -EINVAL; }
> +static inline char *blkcg_get_fc_appid(struct bio *bio) { return NULL; }
> +#endif /*CONFIG_BLK_CGROUP_FC_APPID*/
>   #endif	/* _BLK_CGROUP_H */
> 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme