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

Commit Message

Yu Kuai Sept. 30, 2024, 8:52 a.m. UTC

From: Yu Kuai <[e-mail]>

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 <[e-mail]>
---
 block/blk-cgroup.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Tejun Heo Sept. 30, 2024, 5:11 p.m. UTC | #1

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.

Yu Kuai Oct. 8, 2024, 1:39 a.m. UTC | #2

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

Christoph Hellwig Oct. 8, 2024, 5:01 a.m. UTC | #3

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.

Yu Kuai Oct. 8, 2024, 6:24 a.m. UTC | #4

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

> 
> .
>

Christoph Hellwig Oct. 8, 2024, 6:29 a.m. UTC | #5

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.

Yu Kuai Oct. 31, 2024, 8:04 a.m. UTC | #6

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

Tejun Heo Oct. 31, 2024, 4:55 p.m. UTC | #7

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.

Patch

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