[PATCH] loop: add some basic read-only sysfs attributes

[PATCH] loop: add some basic read-only sysfs attributes

[Milan Broz]

Create /sys/block/loopX/loop directory and provide these attributes:
 - backing_file
 - autoclear
 - offset
 - sizelimit

To be used in util-linux-ng (and possibly elsewhere like udev rules)
where code need to get loop attributes from kernel (and not store
duplicate info in userspace).

Moreover loop ioctls are not even able to provide full backing
file info because of buffer limits.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 drivers/block/loop.c |  110 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/loop.h |    2 +
 2 files changed, 111 insertions(+), 1 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6120922..573fd5a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -73,6 +73,7 @@
 #include <linux/highmem.h>
 #include <linux/kthread.h>
 #include <linux/splice.h>
+#include <linux/sysfs.h>
 
 #include <asm/uaccess.h>
 
@@ -1542,8 +1543,112 @@ out:
 	return NULL;
 }
 
+/* loop sysfs attributes */
+
+struct loop_sysfs_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct loop_device *, char *);
+	ssize_t (*store)(struct loop_device *, char *);
+};
+
+#define LOOP_ATTR_RO(_name) \
+struct loop_sysfs_attr loop_attr_##_name = \
+	__ATTR(_name, S_IRUGO, loop_attr_##_name##_show, NULL)
+
+static ssize_t loop_attr_show(struct kobject *kobj,
+			      struct attribute *attr,
+			      char *page)
+{
+	struct loop_device *lo;
+	struct loop_sysfs_attr *loop_attr;
+
+	lo = container_of(kobj, struct loop_device, lo_kobj);
+	loop_attr = container_of(attr, struct loop_sysfs_attr, attr);
+
+	if (!loop_attr->show)
+		return -EIO;
+
+	return loop_attr->show(lo, page);
+}
+
+static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
+{
+	ssize_t ret;
+	char *p;
+
+	if (lo->lo_state != Lo_bound)
+		return 0;
+
+	mutex_lock(&lo->lo_ctl_mutex);
+	p = d_path(&lo->lo_backing_file->f_path, buf, PAGE_SIZE - 1);
+	mutex_unlock(&lo->lo_ctl_mutex);
+
+	if (IS_ERR(p))
+		ret = PTR_ERR(p);
+	else {
+		ret = strlen(p);
+		memmove(buf, p, ret);
+		buf[ret++] = '\n';
+		buf[ret] = 0;
+	}
+
+	return ret;
+}
+
+static ssize_t loop_attr_offset_show(struct loop_device *lo, char *buf)
+{
+	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_offset);
+}
+
+static ssize_t loop_attr_sizelimit_show(struct loop_device *lo, char *buf)
+{
+	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_sizelimit);
+}
+
+static ssize_t loop_attr_autoclear_show(struct loop_device *lo, char *buf)
+{
+	int autoclear = (lo->lo_flags & LO_FLAGS_AUTOCLEAR);
+
+	return sprintf(buf, "%s\n", autoclear ? "1" : "0");
+}
+
+static LOOP_ATTR_RO(backing_file);
+static LOOP_ATTR_RO(offset);
+static LOOP_ATTR_RO(sizelimit);
+static LOOP_ATTR_RO(autoclear);
+
+static struct attribute *loop_attrs[] = {
+	&loop_attr_backing_file.attr,
+	&loop_attr_offset.attr,
+	&loop_attr_sizelimit.attr,
+	&loop_attr_autoclear.attr,
+	NULL,
+};
+
+static const struct sysfs_ops loop_sysfs_ops = {
+	.show	= loop_attr_show,
+};
+
+static struct kobj_type loop_ktype = {
+	.sysfs_ops	= &loop_sysfs_ops,
+	.default_attrs	= loop_attrs,
+};
+
+static int  loop_sysfs_init(struct loop_device *lo)
+{
+	return kobject_init_and_add(&lo->lo_kobj, &loop_ktype,
+				    &disk_to_dev(lo->lo_disk)->kobj,
+				    "%s", "loop");
+}
+
+static void loop_sysfs_exit(struct loop_device *lo)
+{
+	kobject_put(&lo->lo_kobj);
+}
+
 static void loop_free(struct loop_device *lo)
 {
+	loop_sysfs_exit(lo);
 	blk_cleanup_queue(lo->lo_queue);
 	put_disk(lo->lo_disk);
 	list_del(&lo->lo_list);
@@ -1562,6 +1667,7 @@ static struct loop_device *loop_init_one(int i)
 	lo = loop_alloc(i);
 	if (lo) {
 		add_disk(lo->lo_disk);
+		loop_sysfs_init(lo);
 		list_add_tail(&lo->lo_list, &loop_devices);
 	}
 	return lo;
@@ -1635,8 +1741,10 @@ static int __init loop_init(void)
 
 	/* point of no return */
 
-	list_for_each_entry(lo, &loop_devices, lo_list)
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		add_disk(lo->lo_disk);
+		loop_sysfs_init(lo);
+	}
 
 	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
 				  THIS_MODULE, loop_probe, NULL, NULL);
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 66c194e..0b26385 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -65,6 +65,8 @@ struct loop_device {
 	struct request_queue	*lo_queue;
 	struct gendisk		*lo_disk;
 	struct list_head	lo_list;
+
+	struct kobject		lo_kobj;
 };
 
 #endif /* __KERNEL__ */
-- 
1.7.1

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
> Create /sys/block/loopX/loop directory and provide these attributes:
>  - backing_file
>  - autoclear
>  - offset
>  - sizelimit
>
> To be used in util-linux-ng (and possibly elsewhere like udev rules)
> where code need to get loop attributes from kernel (and not store
> duplicate info in userspace).

Isn't it that the loop attributes are created _after_ the loopdev is
registered? That would make it hard to use these attributes from udev,
as the event is already running while they are created.

Kay

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Milan Broz]

On 07/29/2010 03:47 PM, Kay Sievers wrote:
> On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
>> Create /sys/block/loopX/loop directory and provide these attributes:
>>  - backing_file
>>  - autoclear
>>  - offset
>>  - sizelimit
>>
>> To be used in util-linux-ng (and possibly elsewhere like udev rules)
>> where code need to get loop attributes from kernel (and not store
>> duplicate info in userspace).
> 
> Isn't it that the loop attributes are created _after_ the loopdev is
> registered? That would make it hard to use these attributes from udev,
> as the event is already running while they are created.

First 8 loop devices are registered always (without backing file),
so you have wait for change event initiated from fd set ioctl anyway...
(backing file attribute is empty in that case)

Milan

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Thu, Jul 29, 2010 at 16:06, Milan Broz <mbroz@redhat.com> wrote:
> On 07/29/2010 03:47 PM, Kay Sievers wrote:
>> On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
>>> Create /sys/block/loopX/loop directory and provide these attributes:
>>>  - backing_file
>>>  - autoclear
>>>  - offset
>>>  - sizelimit
>>>
>>> To be used in util-linux-ng (and possibly elsewhere like udev rules)
>>> where code need to get loop attributes from kernel (and not store
>>> duplicate info in userspace).
>>
>> Isn't it that the loop attributes are created _after_ the loopdev is
>> registered? That would make it hard to use these attributes from udev,
>> as the event is already running while they are created.
>
> First 8 loop devices are registered always (without backing file),
> so you have wait for change event initiated from fd set ioctl anyway...
> (backing file attribute is empty in that case)

Ah, so we are sure, we always get a 'change' event, and before that,
none of these values are ever useful to read? I mean, there will not
be attributes that are interesting during an 'add' event?

Kay

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Milan Broz]

On 07/29/2010 04:22 PM, Kay Sievers wrote:
>> First 8 loop devices are registered always (without backing file),
>> so you have wait for change event initiated from fd set ioctl anyway...
>> (backing file attribute is empty in that case)
> 
> Ah, so we are sure, we always get a 'change' event, and before that,
> none of these values are ever useful to read? I mean, there will not
> be attributes that are interesting during an 'add' event?

I think it was already that way for all loop block devices...

See loop block devices registered during module init
(up to max_loop - which is 8 by default) and later configured by losetup.

Milan

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Karel Zak]

On Thu, Jul 29, 2010 at 04:22:50PM +0200, Kay Sievers wrote:
> On Thu, Jul 29, 2010 at 16:06, Milan Broz <mbroz@redhat.com> wrote:
> > On 07/29/2010 03:47 PM, Kay Sievers wrote:
> >> On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
> >>> Create /sys/block/loopX/loop directory and provide these attributes:
> >>>  - backing_file
> >>>  - autoclear
> >>>  - offset
> >>>  - sizelimit
> >>>
> >>> To be used in util-linux-ng (and possibly elsewhere like udev rules)
> >>> where code need to get loop attributes from kernel (and not store
> >>> duplicate info in userspace).
> >>
> >> Isn't it that the loop attributes are created _after_ the loopdev is
> >> registered? That would make it hard to use these attributes from udev,
> >> as the event is already running while they are created.
> >
> > First 8 loop devices are registered always (without backing file),
> > so you have wait for change event initiated from fd set ioctl anyway...
> > (backing file attribute is empty in that case)
> 
> Ah, so we are sure, we always get a 'change' event, and before that,
> none of these values are ever useful to read? I mean, there will not
> be attributes that are interesting during an 'add' event?

I think the patch does not change the current behavior. It exports
details about loopdevs to userspace by /sys. This is the primary goal
of the patch.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Thu, Jul 29, 2010 at 16:58, Karel Zak <kzak@redhat.com> wrote:
> On Thu, Jul 29, 2010 at 04:22:50PM +0200, Kay Sievers wrote:
>> On Thu, Jul 29, 2010 at 16:06, Milan Broz <mbroz@redhat.com> wrote:
>> > On 07/29/2010 03:47 PM, Kay Sievers wrote:
>> >> On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
>> >>> Create /sys/block/loopX/loop directory and provide these attributes:
>> >>>  - backing_file
>> >>>  - autoclear
>> >>>  - offset
>> >>>  - sizelimit
>> >>>
>> >>> To be used in util-linux-ng (and possibly elsewhere like udev rules)
>> >>> where code need to get loop attributes from kernel (and not store
>> >>> duplicate info in userspace).
>> >>
>> >> Isn't it that the loop attributes are created _after_ the loopdev is
>> >> registered? That would make it hard to use these attributes from udev,
>> >> as the event is already running while they are created.
>> >
>> > First 8 loop devices are registered always (without backing file),
>> > so you have wait for change event initiated from fd set ioctl anyway...
>> > (backing file attribute is empty in that case)
>>
>> Ah, so we are sure, we always get a 'change' event, and before that,
>> none of these values are ever useful to read? I mean, there will not
>> be attributes that are interesting during an 'add' event?
>
> I think the patch does not change the current behavior. It exports
> details about loopdevs to userspace by /sys. This is the primary goal
> of the patch.

Sure it does. Sysfs attributes need to be created _before_ uevents are
sent out. The current behavior is that all blockdev attributes are
safely created before the event is sent. These loop attributes are
created _after_ the event is sent.

The question is if we can rely on the fact, that 'add' events never
want to look at any of these attributes, and all can be deferred to
later 'change' events. If we can't be fully certain about this, this
stuff must be changed to happen before the event for the blockdev is
sent out.

Kay

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Karel Zak]

On Thu, Jul 29, 2010 at 06:07:31PM +0200, Kay Sievers wrote:
> On Thu, Jul 29, 2010 at 16:58, Karel Zak <kzak@redhat.com> wrote:
> > On Thu, Jul 29, 2010 at 04:22:50PM +0200, Kay Sievers wrote:
> >> On Thu, Jul 29, 2010 at 16:06, Milan Broz <mbroz@redhat.com> wrote:
> >> > On 07/29/2010 03:47 PM, Kay Sievers wrote:
> >> >> On Thu, Jul 29, 2010 at 15:33, Milan Broz <mbroz@redhat.com> wrote:
> >> >>> Create /sys/block/loopX/loop directory and provide these attributes:
> >> >>>  - backing_file
> >> >>>  - autoclear
> >> >>>  - offset
> >> >>>  - sizelimit
> >> >>>
> >> >>> To be used in util-linux-ng (and possibly elsewhere like udev rules)
> >> >>> where code need to get loop attributes from kernel (and not store
> >> >>> duplicate info in userspace).
> >> >>
> >> >> Isn't it that the loop attributes are created _after_ the loopdev is
> >> >> registered? That would make it hard to use these attributes from udev,
> >> >> as the event is already running while they are created.
> >> >
> >> > First 8 loop devices are registered always (without backing file),
> >> > so you have wait for change event initiated from fd set ioctl anyway...
> >> > (backing file attribute is empty in that case)
> >>
> >> Ah, so we are sure, we always get a 'change' event, and before that,
> >> none of these values are ever useful to read? I mean, there will not
> >> be attributes that are interesting during an 'add' event?
> >
> > I think the patch does not change the current behavior. It exports
> > details about loopdevs to userspace by /sys. This is the primary goal
> > of the patch.
> 
> Sure it does. Sysfs attributes need to be created _before_ uevents are
> sent out. The current behavior is that all blockdev attributes are
> safely created before the event is sent. These loop attributes are
> created _after_ the event is sent.
> 
> The question is if we can rely on the fact, that 'add' events never
> want to look at any of these attributes, and all can be deferred to

 I think we can rely on this fact, 'add' does not mean that the device
 is ready. BTW, why there is /sys/block/loopN at all if the device
 is not associated with any backing file?

 Note that /sys/block/loopN/ro is there for years and depends on
 backing file as well (so it's useless after "add").

> later 'change' events. If we can't be fully certain about this, this
> stuff must be changed to happen before the event for the blockdev is
> sent out.

 The primary goal is to use the attributes in mount(8) and losetup(8). I 
 have doubts that we will use it udev rules.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Karel Zak]

On Thu, Jul 29, 2010 at 09:56:16PM +0200, Karel Zak wrote:
> BTW, why there is /sys/block/loopN at all if the device
> is not associated with any backing file?

 ignore this question ;-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Milan Broz]

On 07/29/2010 09:56 PM, Karel Zak wrote:
>> Sure it does. Sysfs attributes need to be created _before_ uevents are
>> sent out. The current behavior is that all blockdev attributes are
>> safely created before the event is sent. These loop attributes are
>> created _after_ the event is sent.
>>
>> The question is if we can rely on the fact, that 'add' events never
>> want to look at any of these attributes, and all can be deferred to

The problem is that add_disk() initializes kobject and announces device.
How can I add some new attributes (subdir) with the current code
before it happens?

And why it is problem at all? After configuration there is always change
event and at this time attributes are there.

Milan

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Thu, Jul 29, 2010 at 22:24, Milan Broz <mbroz@redhat.com> wrote:
> On 07/29/2010 09:56 PM, Karel Zak wrote:
>>> Sure it does. Sysfs attributes need to be created _before_ uevents are
>>> sent out. The current behavior is that all blockdev attributes are
>>> safely created before the event is sent. These loop attributes are
>>> created _after_ the event is sent.
>>>
>>> The question is if we can rely on the fact, that 'add' events never
>>> want to look at any of these attributes, and all can be deferred to
>
> The problem is that add_disk() initializes kobject and announces device.
> How can I add some new attributes (subdir) with the current code
> before it happens?

Use attribute groups, and assign them to the class, or in this case to
the device before it is registered. All this stuff is created by the
core before the event is sent out. It also takes care of the subdir,
instead rolling your own kobject stuff, does proper error handling,
and cleans-up things in the proper order, without any further custom
code.

Alternatively, these attributes could be created and removed/created
with the ioctl, and before the 'change' event, only if there is an
active backing file, but I would expect the attribute group at the
device to work just fine.

> And why it is problem at all? After configuration there is always change
> event and at this time attributes are there.

As said, if we can rely on this fact, and there will never be any
attribute now, or added in the future, to this block of attributes,
this might be just fine, that's why I ask ...

We spent a lot of time to get the sysfs timing right, fix all the
subsystems, and make udev work with all sorts of attributes which have
been created like this. To add out-of-band attribute creation timing
today, there needs to be a very good reason to do so.

If you need any help with a try to a switch to the attribute group,
please let me know.

Kay

[PATCH v2] loop: add some basic read-only sysfs attributes

[Milan Broz]

On 07/30/2010 06:36 AM, Kay Sievers wrote:

> Alternatively, these attributes could be created and removed/created
> with the ioctl, and before the 'change' event, only if there is an
> active backing file, but I would expect the attribute group at the
> device to work just fine.
I have no idea how you can add attribute group before add_disk() which
initializes kobj (it ends with BUG_ON in internal_create_group
- because !kobj->sd). Perhaps I missed something?

Anyway, second approach works - now is loop attributes available only
when loop is configured and before CHANGE uevent is sent.

Ok with that?

Milan
-

loop: add some basic read-only sysfs attributes

Create /sys/block/loopX/loop directory and provide these attributes:
 - backing_file
 - autoclear
 - offset
 - sizelimit

This loop directory is present only if loop device is configured.

To be used in util-linux-ng (and possibly elsewhere like udev rules)
where code need to get loop attributes from kernel (and not store
duplicate info in userspace).

Moreover loop ioctls are not even able to provide full backing
file info because of buffer limits.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 drivers/block/loop.c |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6120922..fea84d1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -73,6 +73,7 @@
 #include <linux/highmem.h>
 #include <linux/kthread.h>
 #include <linux/splice.h>
+#include <linux/sysfs.h>
 
 #include <asm/uaccess.h>
 
@@ -736,6 +737,103 @@ static inline int is_loop_device(struct file *file)
 	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
 }
 
+/* loop sysfs attributes */
+
+static ssize_t loop_attr_show(struct device *dev, char *page,
+			      ssize_t (*callback)(struct loop_device *, char *))
+{
+	struct loop_device *l, *lo = NULL;
+
+	mutex_lock(&loop_devices_mutex);
+	list_for_each_entry(l, &loop_devices, lo_list)
+		if (disk_to_dev(l->lo_disk) == dev) {
+			lo = l;
+			break;
+		}
+	mutex_unlock(&loop_devices_mutex);
+
+	return lo ? callback(lo, page) : -EIO;
+}
+
+#define LOOP_ATTR_RO(_name)						\
+static ssize_t loop_attr_##_name##_show(struct loop_device *, char *);	\
+static ssize_t loop_attr_do_show_##_name(struct device *d,		\
+				struct device_attribute *attr, char *b)	\
+{									\
+	return loop_attr_show(d, b, loop_attr_##_name##_show);		\
+}									\
+static struct device_attribute loop_attr_##_name =			\
+	__ATTR(_name, S_IRUGO, loop_attr_do_show_##_name, NULL);
+
+static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
+{
+	ssize_t ret;
+	char *p = NULL;
+
+	mutex_lock(&lo->lo_ctl_mutex);
+	if (lo->lo_backing_file)
+		p = d_path(&lo->lo_backing_file->f_path, buf, PAGE_SIZE - 1);
+	mutex_unlock(&lo->lo_ctl_mutex);
+
+	if (IS_ERR_OR_NULL(p))
+		ret = PTR_ERR(p);
+	else {
+		ret = strlen(p);
+		memmove(buf, p, ret);
+		buf[ret++] = '\n';
+		buf[ret] = 0;
+	}
+
+	return ret;
+}
+
+static ssize_t loop_attr_offset_show(struct loop_device *lo, char *buf)
+{
+	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_offset);
+}
+
+static ssize_t loop_attr_sizelimit_show(struct loop_device *lo, char *buf)
+{
+	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_sizelimit);
+}
+
+static ssize_t loop_attr_autoclear_show(struct loop_device *lo, char *buf)
+{
+	int autoclear = (lo->lo_flags & LO_FLAGS_AUTOCLEAR);
+
+	return sprintf(buf, "%s\n", autoclear ? "1" : "0");
+}
+
+LOOP_ATTR_RO(backing_file);
+LOOP_ATTR_RO(offset);
+LOOP_ATTR_RO(sizelimit);
+LOOP_ATTR_RO(autoclear);
+
+static struct attribute *loop_attrs[] = {
+	&loop_attr_backing_file.attr,
+	&loop_attr_offset.attr,
+	&loop_attr_sizelimit.attr,
+	&loop_attr_autoclear.attr,
+	NULL,
+};
+
+static struct attribute_group loop_attribute_group = {
+	.name = "loop",
+	.attrs= loop_attrs,
+};
+
+static int loop_sysfs_init(struct loop_device *lo)
+{
+	return sysfs_create_group(&disk_to_dev(lo->lo_disk)->kobj,
+				  &loop_attribute_group);
+}
+
+static void loop_sysfs_exit(struct loop_device *lo)
+{
+	sysfs_remove_group(&disk_to_dev(lo->lo_disk)->kobj,
+			   &loop_attribute_group);
+}
+
 static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 		       struct block_device *bdev, unsigned int arg)
 {
@@ -835,6 +933,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 
 	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);
+	loop_sysfs_init(lo);
 	/* let user-space know about the new size */
 	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 
@@ -853,6 +952,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	return 0;
 
 out_clr:
+	loop_sysfs_exit(lo);
 	lo->lo_thread = NULL;
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
@@ -949,6 +1049,7 @@ static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
 	set_capacity(lo->lo_disk, 0);
 	if (bdev) {
 		bd_set_size(bdev, 0);
+		loop_sysfs_exit(lo);
 		/* let user-space know about this change */
 		kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 	}

Re: [PATCH v2] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Fri, Jul 30, 2010 at 16:22, Milan Broz <mbroz@redhat.com> wrote:
> On 07/30/2010 06:36 AM, Kay Sievers wrote:
>
>> Alternatively, these attributes could be created and removed/created
>> with the ioctl, and before the 'change' event, only if there is an
>> active backing file, but I would expect the attribute group at the
>> device to work just fine.
> I have no idea how you can add attribute group before add_disk() which
> initializes kobj (it ends with BUG_ON in internal_create_group
> - because !kobj->sd). Perhaps I missed something?

Attribute groups handle the creation of a kobject (subdir) for you,
you only supply a name to the group. Without a name, they will put all
the attributes in the root of the device.

The 'struct device' has a member **groups, and that can have a list of
attribute groups assigned. You assign them before you register the
device, and the core will take care of everything.

> Anyway, second approach works - now is loop attributes available only
> when loop is configured and before CHANGE uevent is sent.
>
> Ok with that?

Sounds good, nothing to complain from a sysfs timing perspective.

Now you have do decide if you prefer that over the attribute group
approach. :) The code with attribute groups, instead of custom kobject
stuff, might be a bit easier to understand.

Thanks,
Kay

Re: [PATCH v2] loop: add some basic read-only sysfs attributes

[Karel Zak]

On Fri, Jul 30, 2010 at 04:34:43PM +0200, Kay Sievers wrote:
> On Fri, Jul 30, 2010 at 16:22, Milan Broz <mbroz@redhat.com> wrote:
> > On 07/30/2010 06:36 AM, Kay Sievers wrote:
> >
> >> Alternatively, these attributes could be created and removed/created
> >> with the ioctl, and before the 'change' event, only if there is an
> >> active backing file, but I would expect the attribute group at the
> >> device to work just fine.
> > I have no idea how you can add attribute group before add_disk() which
> > initializes kobj (it ends with BUG_ON in internal_create_group
> > - because !kobj->sd). Perhaps I missed something?
> 
> Attribute groups handle the creation of a kobject (subdir) for you,
> you only supply a name to the group. Without a name, they will put all
> the attributes in the root of the device.
> 
> The 'struct device' has a member **groups, and that can have a list of
> attribute groups assigned. You assign them before you register the
> device, and the core will take care of everything.
> 
> > Anyway, second approach works - now is loop attributes available only
> > when loop is configured and before CHANGE uevent is sent.
> >
> > Ok with that?
> 
> Sounds good, nothing to complain from a sysfs timing perspective.

Jens, ping... it would be really really nice to have this feature in 
kernel. 

The ioctls are useless and I'd like to minimize number of situations
where mount(8) behaviour depends on /etc/mtab.

Thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH v2] loop: add some basic read-only sysfs attributes

[Jens Axboe]

On 2010-08-23 14:29, Karel Zak wrote:
> On Fri, Jul 30, 2010 at 04:34:43PM +0200, Kay Sievers wrote:
>> On Fri, Jul 30, 2010 at 16:22, Milan Broz <mbroz@redhat.com> wrote:
>>> On 07/30/2010 06:36 AM, Kay Sievers wrote:
>>>
>>>> Alternatively, these attributes could be created and removed/created
>>>> with the ioctl, and before the 'change' event, only if there is an
>>>> active backing file, but I would expect the attribute group at the
>>>> device to work just fine.
>>> I have no idea how you can add attribute group before add_disk() which
>>> initializes kobj (it ends with BUG_ON in internal_create_group
>>> - because !kobj->sd). Perhaps I missed something?
>>
>> Attribute groups handle the creation of a kobject (subdir) for you,
>> you only supply a name to the group. Without a name, they will put all
>> the attributes in the root of the device.
>>
>> The 'struct device' has a member **groups, and that can have a list of
>> attribute groups assigned. You assign them before you register the
>> device, and the core will take care of everything.
>>
>>> Anyway, second approach works - now is loop attributes available only
>>> when loop is configured and before CHANGE uevent is sent.
>>>
>>> Ok with that?
>>
>> Sounds good, nothing to complain from a sysfs timing perspective.
> 
> Jens, ping... it would be really really nice to have this feature in 
> kernel. 
> 
> The ioctls are useless and I'd like to minimize number of situations
> where mount(8) behaviour depends on /etc/mtab.

Looks good to me as well and agree on the ioctls. Care to resend
a fresh patch and I will queue it up for 2.6.37.

-- 
Jens Axboe

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Karel Zak]

On Thu, Jul 29, 2010 at 06:07:31PM +0200, Kay Sievers wrote:
> Sure it does. Sysfs attributes need to be created _before_ uevents are
> sent out. The current behavior is that all blockdev attributes are
> safely created before the event is sent.

 Hmm... I see that in add_disk(), the "queue" subdirectory is created
 after disk registration (the register_disk() calls kobject_uevent()).
 Is it true?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Fri, Jul 30, 2010 at 09:37, Karel Zak <kzak@redhat.com> wrote:
> On Thu, Jul 29, 2010 at 06:07:31PM +0200, Kay Sievers wrote:
>> Sure it does. Sysfs attributes need to be created _before_ uevents are
>> sent out. The current behavior is that all blockdev attributes are
>> safely created before the event is sent.
>
>  Hmm... I see that in add_disk(), the "queue" subdirectory is created
>  after disk registration (the register_disk() calls kobject_uevent()).
>  Is it true?

Might be. Then seems nobody has tried using this stuff from udev. :)

blk_register_queue() probably needs to move to register_disk(), where
the uevent is delayed until all stuff is properly created.

Kay

Re: [PATCH] loop: add some basic read-only sysfs attributes

[Kay Sievers]

On Fri, Jul 30, 2010 at 09:43, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Fri, Jul 30, 2010 at 09:37, Karel Zak <kzak@redhat.com> wrote:
>> On Thu, Jul 29, 2010 at 06:07:31PM +0200, Kay Sievers wrote:
>>> Sure it does. Sysfs attributes need to be created _before_ uevents are
>>> sent out. The current behavior is that all blockdev attributes are
>>> safely created before the event is sent.
>>
>>  Hmm... I see that in add_disk(), the "queue" subdirectory is created
>>  after disk registration (the register_disk() calls kobject_uevent()).
>>  Is it true?
>
> Might be. Then seems nobody has tried using this stuff from udev. :)
>
> blk_register_queue() probably needs to move to register_disk(), where
> the uevent is delayed until all stuff is properly created.

Seems, someone should finally move register_disk() from
fs/partition/check.c to block/genhd.c, where it belongs, and merge the
code into add_disk(), where the control over the event timing is
easily possible.

Kay