Re: [PATCH] block: only allocate blkcg->fc_app_id when starting to use it

[Christoph Hellwig <hch@infradead.org>]

On Fri, Sep 24, 2021 at 08:24:16PM +0800, Ming Lei wrote:
> So far the feature of BLK_CGROUP_FC_APPID is only used for LPFC, and
> only when it is setup via sysfs. It is very likely for one system to
> never use the feature, so allocate the application id buffer in case
> that someone starts to use it, then we save 129 bytes in each blkcg
> if no one uses the feature.
> 
> Cc: Muneendra Kumar <muneendra.kumar@broadcom.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-cgroup.c         |  3 +++
>  include/linux/blk-cgroup.h | 15 ++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 38b9f7684952..e452adf5f4f6 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1061,6 +1061,9 @@ static void blkcg_css_free(struct cgroup_subsys_state *css)
>  
>  	mutex_unlock(&blkcg_pol_mutex);
>  
> +#ifdef CONFIG_BLK_CGROUP_FC_APPID
> +	kfree(blkcg->fc_app_id);
> +#endif
>  	kfree(blkcg);
>  }
>  
> diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
> index b4de2010fba5..75094c0a752b 100644
> --- a/include/linux/blk-cgroup.h
> +++ b/include/linux/blk-cgroup.h
> @@ -58,7 +58,7 @@ struct blkcg {
>  
>  	struct list_head		all_blkcgs_node;
>  #ifdef CONFIG_BLK_CGROUP_FC_APPID
> -	char                            fc_app_id[FC_APPID_LEN];
> +	char                            *fc_app_id;
>  #endif
>  #ifdef CONFIG_CGROUP_WRITEBACK
>  	struct list_head		cgwb_list;
> @@ -699,7 +699,16 @@ static inline int blkcg_set_fc_appid(char *app_id, u64 cgrp_id, size_t app_id_le
>  	 * the vmid from the fabric.
>  	 * Adding the overhead of a lock is not necessary.
>  	 */
> -	strlcpy(blkcg->fc_app_id, app_id, app_id_len);
> +	if (!blkcg->fc_app_id) {
> +		char *buf = kzalloc(FC_APPID_LEN, GFP_KERNEL);
> +
> +		if (cmpxchg(&blkcg->fc_app_id, NULL, buf))
> +			kfree(buf);
> +	}
> +	if (blkcg->fc_app_id)
> +		strlcpy(blkcg->fc_app_id, app_id, app_id_len);
> +	else
> +		ret = -ENOMEM;

This looks a little cumbersome.  Why not return -ENOMEM using a new
label directly after the kzalloc?  More importantly there alredy must
be something synchronizing the strlcpy, so why do we even need the
cmpxchg?

>  static inline char *blkcg_get_fc_appid(struct bio *bio)
>  {
> -	if (bio && bio->bi_blkg &&
> +	if (bio && bio->bi_blkg && bio->bi_blkg->blkcg->fc_app_id &&
>  		(bio->bi_blkg->blkcg->fc_app_id[0] != '\0'))
>  		return bio->bi_blkg->blkcg->fc_app_id;
>  	return NULL;

And given that we must have some synchronization anyway, why not just
free the appid when it is set to an empty string rather than adding yet
another check here in the fast path?