rbd: set the 'device' link in sysfs
diff mbox series

Message ID 20200123124433.121939-1-hare@suse.de
State New
Headers show
Series
  • rbd: set the 'device' link in sysfs
Related show

Commit Message

Hannes Reinecke Jan. 23, 2020, 12:44 p.m. UTC
The rbd driver already provides additional information in sysfs
under /sys/bus/rbd, so we should set the 'device' link in the block
device to reference this information.

Cc: David Disseldorp <ddiss@suse.com>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/block/rbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ilya Dryomov Jan. 23, 2020, 6:45 p.m. UTC | #1
On Thu, Jan 23, 2020 at 1:44 PM Hannes Reinecke <hare@suse.de> wrote:
>
> The rbd driver already provides additional information in sysfs
> under /sys/bus/rbd, so we should set the 'device' link in the block
> device to reference this information.
>
> Cc: David Disseldorp <ddiss@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/block/rbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 9f1f8689e316..3240b7744aeb 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -6938,7 +6938,7 @@ static ssize_t do_rbd_add(struct bus_type *bus,
>         if (rc)
>                 goto err_out_image_lock;
>
> -       add_disk(rbd_dev->disk);
> +       device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL);
>         /* see rbd_init_disk() */
>         blk_put_queue(rbd_dev->disk->queue);

Hi Hannes,

I looked at this a while ago and didn't go through with the patch
because I wasn't sure whether this symlink can point to something
arbitrary.  IIRC it usually points a couple of levels up, to some
parent.  In the rbd case, this would be a completely different tree:
/sys/devices/virtual -> /sys/bus/rbd.

Do you know if there is precedent for this in some other driver?
Are you sure it's not going to break any assumptions?

How did this came up?  I'm curious because rbd(8) is only utility
that I know of that (somewhat) cares.

Thanks,

                Ilya
Hannes Reinecke Jan. 24, 2020, 7:03 a.m. UTC | #2
On 1/23/20 7:45 PM, Ilya Dryomov wrote:
> On Thu, Jan 23, 2020 at 1:44 PM Hannes Reinecke <hare@suse.de> wrote:
>>
>> The rbd driver already provides additional information in sysfs
>> under /sys/bus/rbd, so we should set the 'device' link in the block
>> device to reference this information.
>>
>> Cc: David Disseldorp <ddiss@suse.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/block/rbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 9f1f8689e316..3240b7744aeb 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -6938,7 +6938,7 @@ static ssize_t do_rbd_add(struct bus_type *bus,
>>         if (rc)
>>                 goto err_out_image_lock;
>>
>> -       add_disk(rbd_dev->disk);
>> +       device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL);
>>         /* see rbd_init_disk() */
>>         blk_put_queue(rbd_dev->disk->queue);
> 
> Hi Hannes,
> 
> I looked at this a while ago and didn't go through with the patch
> because I wasn't sure whether this symlink can point to something
> arbitrary.  IIRC it usually points a couple of levels up, to some
> parent.  In the rbd case, this would be a completely different tree:
> /sys/devices/virtual -> /sys/bus/rbd.
> 
Yes, this is expected.
The 'device' link will _always_ point into a different tree; the
accessor via /sys/block or /sys/bus are assumed to be virtual entries,
with the 'device' link pointing to the underlying device.
In our case the underlying device is also a virtual entity, but that's okay.

> Do you know if there is precedent for this in some other driver?
> Are you sure it's not going to break any assumptions?
> 
Things like iscsi do the very same thing.
And no, it doesn't break assumptions; quite the contrary.

> How did this came up?  I'm curious because rbd(8) is only utility
> that I know of that (somewhat) cares.
> 
That's me digging through the rbd driver and see if I could hook up
nvme-over-fabrics as a frontend to ceph/rbd.
As such I needed to find some information about the rbd device, and
found that while the driver provides additional information under
/sys/devices/rbd/X, this information is not linked to /sys/block/rbdX.
And this patch just links these to bits of information together, so that
now you can get the driver specific informations by following the
'device' link like all the other drivers do, too.

Cheers,

Hannes
Ilya Dryomov Jan. 27, 2020, 10:29 a.m. UTC | #3
On Fri, Jan 24, 2020 at 8:03 AM Hannes Reinecke <hare@suse.de> wrote:
>
> On 1/23/20 7:45 PM, Ilya Dryomov wrote:
> > On Thu, Jan 23, 2020 at 1:44 PM Hannes Reinecke <hare@suse.de> wrote:
> >>
> >> The rbd driver already provides additional information in sysfs
> >> under /sys/bus/rbd, so we should set the 'device' link in the block
> >> device to reference this information.
> >>
> >> Cc: David Disseldorp <ddiss@suse.com>
> >> Signed-off-by: Hannes Reinecke <hare@suse.com>
> >> ---
> >>  drivers/block/rbd.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> >> index 9f1f8689e316..3240b7744aeb 100644
> >> --- a/drivers/block/rbd.c
> >> +++ b/drivers/block/rbd.c
> >> @@ -6938,7 +6938,7 @@ static ssize_t do_rbd_add(struct bus_type *bus,
> >>         if (rc)
> >>                 goto err_out_image_lock;
> >>
> >> -       add_disk(rbd_dev->disk);
> >> +       device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL);
> >>         /* see rbd_init_disk() */
> >>         blk_put_queue(rbd_dev->disk->queue);
> >
> > Hi Hannes,
> >
> > I looked at this a while ago and didn't go through with the patch
> > because I wasn't sure whether this symlink can point to something
> > arbitrary.  IIRC it usually points a couple of levels up, to some
> > parent.  In the rbd case, this would be a completely different tree:
> > /sys/devices/virtual -> /sys/bus/rbd.
> >
> Yes, this is expected.
> The 'device' link will _always_ point into a different tree; the
> accessor via /sys/block or /sys/bus are assumed to be virtual entries,
> with the 'device' link pointing to the underlying device.
> In our case the underlying device is also a virtual entity, but that's okay.
>
> > Do you know if there is precedent for this in some other driver?
> > Are you sure it's not going to break any assumptions?
> >
> Things like iscsi do the very same thing.
> And no, it doesn't break assumptions; quite the contrary.

I see, thanks for the explanation.

Queued up for 5.6.

Thanks,

                Ilya

Patch
diff mbox series

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9f1f8689e316..3240b7744aeb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -6938,7 +6938,7 @@  static ssize_t do_rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_image_lock;
 
-	add_disk(rbd_dev->disk);
+	device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL);
 	/* see rbd_init_disk() */
 	blk_put_queue(rbd_dev->disk->queue);