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

[Ming Lei <ming.lei@redhat.com>]

On Fri, Sep 24, 2021 at 05:04:32PM +0100, Christoph Hellwig wrote:
> 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?

There isn't such sync for strlcpy, blkcg_set_fc_appid is called from
sysfs attribute write, and cgroup_id is specified on the write buffer,
so cmpxchg is needed. Also the comment in blkcg_set_fc_appid() explained
that:

        /*
         * There is a slight race condition on setting the appid.
         * Worst case an I/O may not find the right id.
         * This is no different from the I/O we let pass while obtaining
         * the vmid from the fabric.
         * Adding the overhead of a lock is not necessary.
         */

> 
> >  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?

There isn't the sync, so freeing the buffer will cause trouble easily.


Thanks,
Ming