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

[Yehuda Sadeh Weinraub <yehuda@hq.newdream.net>]

On Mon, Nov 22, 2010 at 4:58 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Nov 22, 2010 at 04:48:54PM -0800, Yehuda Sadeh Weinraub wrote:
>> On Mon, Nov 22, 2010 at 4:14 PM, Greg KH <greg@kroah.com> 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@kroah.com> 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.
>
> Do either one.  I would suggest a prefix.
>
>> 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.
>
> But something cares about them, so treat them properly.
>
>> >> 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.
>
> Then use a prefix and put everything in the same subdirectory underneath
> the id and you should be fine, right?
>
Functional-wise it'll give what we need, albeit not as pretty. I guess
we could do that, but for the sake of completion, I'd like to fully
understand what's wrong with keeping the extra kobject under the
device like this:

struct rbd_snaps {
	struct kobject  kobj;
};

struct rbd_device {
  struct device dev;
  strict rbd_snaps *snaps;
};

where rbd->snaps.kobj is initialized to have rbd.dev.kobj as its parent.

Thanks,
Yehuda