[PATCH v2 0/5] blk-cgroup: don't abuse bdi in blk-cgroup

[PATCH v2 0/5] blk-cgroup: don't abuse bdi in blk-cgroup

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

The bdi_dev_name() should not be used in blk-cgroup code, because bdi is
not related at all, add a new helper to print device name directly from
gendisk. The helper can also fix that "unknown" will be printed for
hidden disks.

Yu Kuai (5):
  blk-cgroup: add a new helper blkg_print_dev_name()
  blk-iocost: use new helper blkg_print_dev_name()
  blk-throttle: use new helper blkg_print_dev_name()
  blk-iolatency: use new helper blkg_print_dev_name()
  blk-cgroup: use new helper blkg_print_dev_name()

 block/blk-cgroup-rwstat.c | 13 +++++++------
 block/blk-cgroup.c        | 19 ++++---------------
 block/blk-cgroup.h        | 13 ++++++++++++-
 block/blk-iocost.c        | 23 ++++++++++++-----------
 block/blk-iolatency.c     | 11 +++++++----
 block/blk-throttle.c      | 15 +++++----------
 6 files changed, 47 insertions(+), 47 deletions(-)

-- 
2.39.2

[PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

The bdi_dev_name() should not be used in blk-cgroup code, because bdi is
not related at all, add a new helper to print device name directly from
gendisk. The helper can also fix that "unknown" will be printed for
hidden disks.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index b9e3265c1eb3..d62bcc2bae14 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -239,6 +239,18 @@ static inline bool bio_issue_as_root_blkg(struct bio *bio)
 	return (bio->bi_opf & (REQ_META | REQ_SWAP)) != 0;
 }
 
+static inline bool blkg_print_dev_name(struct seq_file *sf,
+				       struct blkcg_gq *blkg)
+{
+	struct gendisk *disk = blkg->q->disk;
+
+	if (!disk)
+		return false;
+
+	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
+	return true;
+}
+
 /**
  * blkg_lookup - lookup blkg for the specified blkcg - q pair
  * @blkcg: blkcg of interest
-- 
2.39.2

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Tejun Heo]

Hello,

On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
> +static inline bool blkg_print_dev_name(struct seq_file *sf,
> +				       struct blkcg_gq *blkg)
> +{
> +	struct gendisk *disk = blkg->q->disk;
> +
> +	if (!disk)
> +		return false;
> +
> +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
> +	return true;
> +}
> +

I wonder whether we just should add a name field to disk.

Thanks.

-- 
tejun

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Yu Kuai]

Hi,

在 2024/10/01 1:11, Tejun Heo 写道:
> Hello,
> 
> On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
>> +static inline bool blkg_print_dev_name(struct seq_file *sf,
>> +				       struct blkcg_gq *blkg)
>> +{
>> +	struct gendisk *disk = blkg->q->disk;
>> +
>> +	if (!disk)
>> +		return false;
>> +
>> +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
>> +	return true;
>> +}
>> +
> 
> I wonder whether we just should add a name field to disk.
> 

Of course we can, however, I'm not sure if this is better, because
this field is not used in the fast path.

Thanks,
Kuai

> Thanks.
> 

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 09:39:05AM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2024/10/01 1:11, Tejun Heo 写道:
> > Hello,
> > 
> > On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
> > > +static inline bool blkg_print_dev_name(struct seq_file *sf,
> > > +				       struct blkcg_gq *blkg)
> > > +{
> > > +	struct gendisk *disk = blkg->q->disk;
> > > +
> > > +	if (!disk)
> > > +		return false;
> > > +
> > > +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
> > > +	return true;
> > > +}
> > > +
> > 
> > I wonder whether we just should add a name field to disk.
> > 
> 
> Of course we can, however, I'm not sure if this is better, because
> this field is not used in the fast path.

Struct gendisk does have a (disk_)name field aleady.

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Yu Kuai]

Hi,

在 2024/10/08 13:01, Christoph Hellwig 写道:
> On Tue, Oct 08, 2024 at 09:39:05AM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2024/10/01 1:11, Tejun Heo 写道:
>>> Hello,
>>>
>>> On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
>>>> +static inline bool blkg_print_dev_name(struct seq_file *sf,
>>>> +				       struct blkcg_gq *blkg)
>>>> +{
>>>> +	struct gendisk *disk = blkg->q->disk;
>>>> +
>>>> +	if (!disk)
>>>> +		return false;
>>>> +
>>>> +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
>>>> +	return true;
>>>> +}
>>>> +
>>>
>>> I wonder whether we just should add a name field to disk.
>>>
>>
>> Of course we can, however, I'm not sure if this is better, because
>> this field is not used in the fast path.
> 
> Struct gendisk does have a (disk_)name field aleady.

Yes, but this name is not major and minor(for example, sda instead of
8:0), Tejun was probably talking about major and minor name field.

Thanks,
Kuai

> 
> .
> 

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 02:24:45PM +0800, Yu Kuai wrote:
> Yes, but this name is not major and minor(for example, sda instead of
> 8:0), Tejun was probably talking about major and minor name field.

Yes, I don't really want to add that as it is a horrible user interface.

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Yu Kuai]

Hi, Tejun!

在 2024/10/01 1:11, Tejun Heo 写道:
> Hello,
> 
> On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
>> +static inline bool blkg_print_dev_name(struct seq_file *sf,
>> +				       struct blkcg_gq *blkg)
>> +{
>> +	struct gendisk *disk = blkg->q->disk;
>> +
>> +	if (!disk)
>> +		return false;
>> +
>> +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
>> +	return true;
>> +}
>> +
> 
> I wonder whether we just should add a name field to disk.

And suggestions on this set now? I guess add a name filed is not
appropriate. :(

Thanks,
Kuai

> 
> Thanks.
> 

Re: [PATCH v2 1/5] blk-cgroup: add a new helper blkg_print_dev_name()

[Tejun Heo]

Hello,

On Thu, Oct 31, 2024 at 04:04:00PM +0800, Yu Kuai wrote:
> > On Mon, Sep 30, 2024 at 04:52:58PM +0800, Yu Kuai wrote:
> > > +static inline bool blkg_print_dev_name(struct seq_file *sf,
> > > +				       struct blkcg_gq *blkg)
> > > +{
> > > +	struct gendisk *disk = blkg->q->disk;
> > > +
> > > +	if (!disk)
> > > +		return false;
> > > +
> > > +	seq_printf(sf, "%u:%u", disk->major, disk->first_minor);
> > > +	return true;
> > > +}
> > > +
> > 
> > I wonder whether we just should add a name field to disk.
> 
> And suggestions on this set now? I guess add a name filed is not
> appropriate. :(

Yeah, I don't know. I've always struggled a bit with block device names. We
use MAJ:MIN in all the input interfaces and mix the disk names and MAJ:MIN
when outputting and there are (or is it used to be now?) request_queues
without disk attached, so sometimes names are just not available.

Jens, do you any preference here? The proposed patch can be fine but e.g. it
can race against disk_release() if the caller isn't careful and it also
sucks not knowing the name in destruction path.

Thanks.

-- 
tejun

[PATCH 2/5] blk-iocost: use new helper blkg_print_dev_name()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

To avoid abuse of bdi_dev_name().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 5a6098a3db57..eede926937ac 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3067,11 +3067,14 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
 static u64 ioc_weight_prfill(struct seq_file *sf, struct blkg_policy_data *pd,
 			     int off)
 {
-	const char *dname = blkg_dev_name(pd->blkg);
 	struct ioc_gq *iocg = pd_to_iocg(pd);
 
-	if (dname && iocg->cfg_weight)
-		seq_printf(sf, "%s %u\n", dname, iocg->cfg_weight / WEIGHT_ONE);
+	if (!iocg->cfg_weight)
+		return 0;
+
+	if (blkg_print_dev_name(sf, pd->blkg))
+		seq_printf(sf, " %u\n", iocg->cfg_weight / WEIGHT_ONE);
+
 	return 0;
 }
 
@@ -3160,15 +3163,14 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd,
 			  int off)
 {
-	const char *dname = blkg_dev_name(pd->blkg);
 	struct ioc *ioc = pd_to_iocg(pd)->ioc;
 
-	if (!dname)
+	if (!blkg_print_dev_name(sf, pd->blkg))
 		return 0;
 
 	spin_lock_irq(&ioc->lock);
-	seq_printf(sf, "%s enable=%d ctrl=%s rpct=%u.%02u rlat=%u wpct=%u.%02u wlat=%u min=%u.%02u max=%u.%02u\n",
-		   dname, ioc->enabled, ioc->user_qos_params ? "user" : "auto",
+	seq_printf(sf, " enable=%d ctrl=%s rpct=%u.%02u rlat=%u wpct=%u.%02u wlat=%u min=%u.%02u max=%u.%02u\n",
+		   ioc->enabled, ioc->user_qos_params ? "user" : "auto",
 		   ioc->params.qos[QOS_RPPM] / 10000,
 		   ioc->params.qos[QOS_RPPM] % 10000 / 100,
 		   ioc->params.qos[QOS_RLAT],
@@ -3359,18 +3361,17 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 static u64 ioc_cost_model_prfill(struct seq_file *sf,
 				 struct blkg_policy_data *pd, int off)
 {
-	const char *dname = blkg_dev_name(pd->blkg);
 	struct ioc *ioc = pd_to_iocg(pd)->ioc;
 	u64 *u = ioc->params.i_lcoefs;
 
-	if (!dname)
+	if (!blkg_print_dev_name(sf, pd->blkg))
 		return 0;
 
 	spin_lock_irq(&ioc->lock);
-	seq_printf(sf, "%s ctrl=%s model=linear "
+	seq_printf(sf, " ctrl=%s model=linear "
 		   "rbps=%llu rseqiops=%llu rrandiops=%llu "
 		   "wbps=%llu wseqiops=%llu wrandiops=%llu\n",
-		   dname, ioc->user_cost_model ? "user" : "auto",
+		   ioc->user_cost_model ? "user" : "auto",
 		   u[I_LCOEF_RBPS], u[I_LCOEF_RSEQIOPS], u[I_LCOEF_RRANDIOPS],
 		   u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS]);
 	spin_unlock_irq(&ioc->lock);
-- 
2.39.2

[PATCH 3/5] blk-throttle: use new helper blkg_print_dev_name()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

To avoid abuse of bdi_dev_name().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6943ec720f39..35d86d7570c5 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1381,15 +1381,8 @@ static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd,
 			 int off)
 {
 	struct throtl_grp *tg = pd_to_tg(pd);
-	const char *dname = blkg_dev_name(pd->blkg);
-	u64 bps_dft;
-	unsigned int iops_dft;
-
-	if (!dname)
-		return 0;
-
-	bps_dft = U64_MAX;
-	iops_dft = UINT_MAX;
+	u64 bps_dft = U64_MAX;
+	unsigned int iops_dft = UINT_MAX;
 
 	if (tg->bps[READ] == bps_dft &&
 	    tg->bps[WRITE] == bps_dft &&
@@ -1397,7 +1390,9 @@ static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd,
 	    tg->iops[WRITE] == iops_dft)
 		return 0;
 
-	seq_printf(sf, "%s", dname);
+	if (!blkg_print_dev_name(sf, pd->blkg))
+		return 0;
+
 	if (tg->bps[READ] == U64_MAX)
 		seq_printf(sf, " rbps=max");
 	else
-- 
2.39.2

[PATCH 4/5] blk-iolatency: use new helper blkg_print_dev_name()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

To avoid abuse of bdi_dev_name().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iolatency.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index ebb522788d97..9c9ab4a73e0b 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -898,12 +898,15 @@ static u64 iolatency_prfill_limit(struct seq_file *sf,
 				  struct blkg_policy_data *pd, int off)
 {
 	struct iolatency_grp *iolat = pd_to_lat(pd);
-	const char *dname = blkg_dev_name(pd->blkg);
 
-	if (!dname || !iolat->min_lat_nsec)
+	if (!iolat->min_lat_nsec)
 		return 0;
-	seq_printf(sf, "%s target=%llu\n",
-		   dname, div_u64(iolat->min_lat_nsec, NSEC_PER_USEC));
+
+	if (!blkg_print_dev_name(sf, pd->blkg))
+		return 0;
+
+	seq_printf(sf, " target=%llu\n",
+		   div_u64(iolat->min_lat_nsec, NSEC_PER_USEC));
 	return 0;
 }
 
-- 
2.39.2

[PATCH 5/5] blk-cgroup: use new helper blkg_print_dev_name()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

To avoid abuse of bdi_dev_name(), and remove blkg_dev_name() since it's
not used anymore.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup-rwstat.c | 13 +++++++------
 block/blk-cgroup.c        | 19 ++++---------------
 block/blk-cgroup.h        |  1 -
 3 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/block/blk-cgroup-rwstat.c b/block/blk-cgroup-rwstat.c
index a55fb0c53558..a0a1694572da 100644
--- a/block/blk-cgroup-rwstat.c
+++ b/block/blk-cgroup-rwstat.c
@@ -43,21 +43,22 @@ u64 __blkg_prfill_rwstat(struct seq_file *sf, struct blkg_policy_data *pd,
 		[BLKG_RWSTAT_ASYNC]	= "Async",
 		[BLKG_RWSTAT_DISCARD]	= "Discard",
 	};
-	const char *dname = blkg_dev_name(pd->blkg);
 	u64 v;
 	int i;
 
-	if (!dname)
+	if (!pd->blkg->q->disk)
 		return 0;
 
-	for (i = 0; i < BLKG_RWSTAT_NR; i++)
-		seq_printf(sf, "%s %s %llu\n", dname, rwstr[i],
-			   rwstat->cnt[i]);
+	for (i = 0; i < BLKG_RWSTAT_NR; i++) {
+		blkg_print_dev_name(sf, pd->blkg);
+		seq_printf(sf, " %s %llu\n", rwstr[i], rwstat->cnt[i]);
+	}
 
 	v = rwstat->cnt[BLKG_RWSTAT_READ] +
 		rwstat->cnt[BLKG_RWSTAT_WRITE] +
 		rwstat->cnt[BLKG_RWSTAT_DISCARD];
-	seq_printf(sf, "%s Total %llu\n", dname, v);
+	blkg_print_dev_name(sf, pd->blkg);
+	seq_printf(sf, " Total %llu\n", v);
 	return v;
 }
 EXPORT_SYMBOL_GPL(__blkg_prfill_rwstat);
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index e68c725cf8d9..475fb826d92b 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -682,13 +682,6 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css,
 	return 0;
 }
 
-const char *blkg_dev_name(struct blkcg_gq *blkg)
-{
-	if (!blkg->q->disk)
-		return NULL;
-	return bdi_dev_name(blkg->q->disk->bdi);
-}
-
 /**
  * blkcg_print_blkgs - helper for printing per-blkg data
  * @sf: seq_file to print to
@@ -740,12 +733,10 @@ EXPORT_SYMBOL_GPL(blkcg_print_blkgs);
  */
 u64 __blkg_prfill_u64(struct seq_file *sf, struct blkg_policy_data *pd, u64 v)
 {
-	const char *dname = blkg_dev_name(pd->blkg);
-
-	if (!dname)
+	if (!blkg_print_dev_name(sf, pd->blkg))
 		return 0;
 
-	seq_printf(sf, "%s %llu\n", dname, (unsigned long long)v);
+	seq_printf(sf, " %llu\n", (unsigned long long)v);
 	return v;
 }
 EXPORT_SYMBOL_GPL(__blkg_prfill_u64);
@@ -1144,18 +1135,16 @@ static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s)
 {
 	struct blkg_iostat_set *bis = &blkg->iostat;
 	u64 rbytes, wbytes, rios, wios, dbytes, dios;
-	const char *dname;
 	unsigned seq;
 	int i;
 
 	if (!blkg->online)
 		return;
 
-	dname = blkg_dev_name(blkg);
-	if (!dname)
+	if (!blkg_print_dev_name(s, blkg))
 		return;
 
-	seq_printf(s, "%s ", dname);
+	seq_putc(s, ' ');
 
 	do {
 		seq = u64_stats_fetch_begin(&bis->sync);
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index d62bcc2bae14..ab9365bc23ef 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -202,7 +202,6 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol);
 void blkcg_deactivate_policy(struct gendisk *disk,
 			     const struct blkcg_policy *pol);
 
-const char *blkg_dev_name(struct blkcg_gq *blkg);
 void blkcg_print_blkgs(struct seq_file *sf, struct blkcg *blkcg,
 		       u64 (*prfill)(struct seq_file *,
 				     struct blkg_policy_data *, int),
-- 
2.39.2