Re: [PATCH] rbd: replace the rbd sysfs interface

[Yehuda Sadeh Weinraub]

On Mon, Nov 22, 2010 at 4:14 PM, Greg KH <greg@xxxxxxxxx> wrote:
> On Fri, Nov 19, 2010 at 12:42:51PM -0800, Yehuda Sadeh Weinraub wrote:
>> On Thu, Nov 18, 2010 at 6:08 PM, Greg KH <greg@xxxxxxxxx> wrote:
>> > On Thu, Nov 18, 2010 at 02:53:35PM -0800, Yehuda Sadeh Weinraub wrote:
>> >> Yes, pretty much. One problem that I do see is that if we define the
>> >> snaps/ as a device (and not just as a kobj) as you suggested before,
>> >> it'll automatically create a 'uevent' entry under it which can be a
>> >> real issue in the case we have a snapshot named like that. Shouldn't
>> >> we just create it as a kobj in that case?
>> >
>> > No.  Just use the subdirectory option of an attribute group to handle
>> > that and you will not need to create any device or kobject with that
>> > name, the driver core will handle it all automatically for you.
>> >
>>
>> One issue with using the groups name, is that it's not nested (unless
>> I'm missing something), so we can't have it done for the entire
>> planned hierarchy without holding a kobject on the way. Just a
>> reminder, the device-specific hierarchy would look like this:
>>
>> 1. /sys/bus/rbd/devices/<id>/
>> 2. /sys/bus/rbd/devices/<id>/<device_attrs>
>> 3. /sys/bus/rbd/devices/<id>/snaps/
>> 4. /sys/bus/rbd/devices/<id>/snaps/<snap_name>/
>> 5. /sys/bus/rbd/devices/<id>/snaps/<snap_name>/<snap_attrs>
>>
>> One solution would be to create kobjects for (3) and for (4), without
>> using a group name.
>
> Ick, no.
>
>> Another way, we can create groups for (2), and (3)
>> under (1), but that's about it,
>
> attribute group for 2 is fine.
>
>> you can't create the snap specific directory this way without
>> resorting to some internal sysfs directory creation, which will be
>> horribly wrong. At that point we don't have anything for 'snaps', and
>> we don't really need to do any operations under that directory, we
>> just need it to exist so that it contains the snapshot-specific
>> directories.
>
> But you need to do something with those snapshots, right?  So, why even
> have "snaps" be a subdir?  Why not just make <snap_name> a struct device
> with <id> being the parent, and it living on the same bus_type, but
> being a different device_type (like partitions and block devices are),

The reason we keep snapshots in a separate subdirectory is that they
can have arbitrary name. So either we prefix them and put them in a
common namespace with the devices, or we put them down the hierarchy.
In any case we don't do any operations on them, we just have them for
informational use and we put them there so that we don't have one big
file that lists them all.

>
>> Another way would be to create a group for (2) under (1) and create a
>> kobject for (3), for which you can create group per snapshot.
>>
>> Am I missing something? We already have the first solution (kobjects
>> only) implemented, is there some real benefit for using the third
>> method? We'll have to manually add remove groups anyway, as snapshots
>> can be removed and new snapshots can be added.
>
> Never add kobjects to a struct device, that is showing you that
> something is wrong, and that userspace really will want to get that
> create/destroy event of the sub child.
>

But they're there as information device attributes, it's nothing like
partitions in block devices. So we want to just be able to list them
and their attributes easily (and nicely), without having to put them
in one big file.

Yehuda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/