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

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

[Ming Lei]

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;
 	css_put(css);
 out_cgrp_put:
 	cgroup_put(cgrp);
@@ -714,7 +723,7 @@ static inline int blkcg_set_fc_appid(char *app_id, u64 cgrp_id, size_t app_id_le
  */
 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;
-- 
2.31.1

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

[Christoph Hellwig]

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?

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

[Ming Lei]

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

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

[Christoph Hellwig]

On Sun, Sep 26, 2021 at 03:55:07PM +0800, Ming Lei wrote:
> 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.
>          */

But it isn't really a good idea.  If you have e.g. IDs with a shared
prefix this will lead to incorrect results.  I think we need a proper
locking or RCU scheme here, which should also give you the dynamic
allocation and freeing for free.