diff mbox series

[RFC,v2,3/7] block: export some API

Message ID 20240618031751.3470464-4-yukuai1@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series blk-iocost: support to build iocost as kernel module | expand

Commit Message

Yu Kuai June 18, 2024, 3:17 a.m. UTC 
From: Yu Kuai <yukuai3@huawei.com>

These APIs are used in iocost, prepare to build iocost as kernel module.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.c | 4 ++++
 block/blk-rq-qos.c | 2 ++
 2 files changed, 6 insertions(+)

Comments

Christoph Hellwig June 18, 2024, 7:14 a.m. UTC | #1 
On Tue, Jun 18, 2024 at 11:17:47AM +0800, Yu Kuai wrote:
>  bool blkcg_debug_stats = false;
> +EXPORT_SYMBOL_GPL(blkcg_debug_stats);

exporting variables is lways a bad idea.a

>  
>  static DEFINE_RAW_SPINLOCK(blkg_stat_lock);
>  
> @@ -688,6 +689,7 @@ const char *blkg_dev_name(struct blkcg_gq *blkg)
>  		return NULL;
>  	return bdi_dev_name(blkg->q->disk->bdi);
>  }
> +EXPORT_SYMBOL_GPL(blkg_dev_name);

And this helper is just horribly to be honest.  The bdi_dev_name
should not be used anywhere in block code, and something like this
certainly should not be exported even if we have to keep it for
legacy reasons.

>  /**
>   * blkcg_print_blkgs - helper for printing per-blkg data
> @@ -815,6 +817,7 @@ int blkg_conf_open_bdev(struct blkg_conf_ctx *ctx)
>  	ctx->bdev = bdev;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(blkg_conf_open_bdev);

This again is a horrible function papeing over blk-cgroup design
mistakes.  I absolutely should not be exported.
Yu Kuai June 18, 2024, 7:58 a.m. UTC | #2 
Hi,

在 2024/06/18 15:14, Christoph Hellwig 写道:
> On Tue, Jun 18, 2024 at 11:17:47AM +0800, Yu Kuai wrote:
>>   bool blkcg_debug_stats = false;
>> +EXPORT_SYMBOL_GPL(blkcg_debug_stats);
> 
> exporting variables is lways a bad idea.a

Yes, export variable is the easiest way here. Will it be acceptable to
make this variable static and add a helper to access it?

> 
>>   
>>   static DEFINE_RAW_SPINLOCK(blkg_stat_lock);
>>   
>> @@ -688,6 +689,7 @@ const char *blkg_dev_name(struct blkcg_gq *blkg)
>>   		return NULL;
>>   	return bdi_dev_name(blkg->q->disk->bdi);
>>   }
>> +EXPORT_SYMBOL_GPL(blkg_dev_name);
> 
> And this helper is just horribly to be honest.  The bdi_dev_name
> should not be used anywhere in block code, and something like this
> certainly should not be exported even if we have to keep it for
> legacy reasons.

And I can reuse __blkg_prfill_u64() for iocost like bfq did, then this
can be removed.

> 
>>   /**
>>    * blkcg_print_blkgs - helper for printing per-blkg data
>> @@ -815,6 +817,7 @@ int blkg_conf_open_bdev(struct blkg_conf_ctx *ctx)
>>   	ctx->bdev = bdev;
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(blkg_conf_open_bdev);
> 
> This again is a horrible function papeing over blk-cgroup design
> mistakes.  I absolutely should not be exported.

There is already bigger helper blkg_conf_prep() exported, as used by bfq
for a long time already, and this helper is introduced for code
optimization, as iocost configuration doesn't need to access the blkg.

Perhaps this should be another discussion about "design mistakes",
however, I'm not sure why. :( Do you have previous discussions?

Thanks,
Kuai

> 
> .
>
Christoph Hellwig June 20, 2024, 6:48 a.m. UTC | #3 
On Tue, Jun 18, 2024 at 03:58:54PM +0800, Yu Kuai wrote:
> There is already bigger helper blkg_conf_prep() exported, as used by bfq
> for a long time already, and this helper is introduced for code
> optimization, as iocost configuration doesn't need to access the blkg.
> 
> Perhaps this should be another discussion about "design mistakes",
> however, I'm not sure why. :( Do you have previous discussions?

blkg_conf_prep at least has some level of abstraction.

But at least my conclusion is:  don't make blk-iocost modular.  It's
far too messy to split out, and not really worth it.
diff mbox series

Patch

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4da70fc7775e..787e3023a366 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -57,6 +57,7 @@  static struct blkcg_policy *blkcg_policy[BLKCG_MAX_POLS];
 static LIST_HEAD(all_blkcgs);		/* protected by blkcg_pol_mutex */
 
 bool blkcg_debug_stats = false;
+EXPORT_SYMBOL_GPL(blkcg_debug_stats);
 
 static DEFINE_RAW_SPINLOCK(blkg_stat_lock);
 
@@ -688,6 +689,7 @@  const char *blkg_dev_name(struct blkcg_gq *blkg)
 		return NULL;
 	return bdi_dev_name(blkg->q->disk->bdi);
 }
+EXPORT_SYMBOL_GPL(blkg_dev_name);
 
 /**
  * blkcg_print_blkgs - helper for printing per-blkg data
@@ -815,6 +817,7 @@  int blkg_conf_open_bdev(struct blkg_conf_ctx *ctx)
 	ctx->bdev = bdev;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blkg_conf_open_bdev);
 
 /**
  * blkg_conf_prep - parse and prepare for per-blkg config update
@@ -2011,6 +2014,7 @@  void blkcg_schedule_throttle(struct gendisk *disk, bool use_memdelay)
 		current->use_memdelay = use_memdelay;
 	set_notify_resume(current);
 }
+EXPORT_SYMBOL_GPL(blkcg_schedule_throttle);
 
 /**
  * blkcg_add_delay - add delay to this blkg
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index dd7310c94713..c3fdf91ddf8d 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -332,6 +332,7 @@  int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
 	blk_mq_unfreeze_queue(q);
 	return -EBUSY;
 }
+EXPORT_SYMBOL_GPL(rq_qos_add);
 
 void rq_qos_del(struct rq_qos *rqos)
 {
@@ -353,3 +354,4 @@  void rq_qos_del(struct rq_qos *rqos)
 	blk_mq_debugfs_unregister_rqos(rqos);
 	mutex_unlock(&q->debugfs_mutex);
 }
+EXPORT_SYMBOL_GPL(rq_qos_del);