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@xxxxxxxxxxxx>
> > Cc: Tejun Heo <tj@xxxxxxxxxx>
> > Cc: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > 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
