[PATCH] nbd: nbd sysfs framework

[PATCH] nbd: nbd sysfs framework

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

Description: The patch creates a new sysfs entry framework for nbd. The
single existing sysfs entry for nbd (pid) now uses the framework.

From: Paul Clements <paul.clements@steeleye.com>
Signed-off-by: Paul Clements <paul.clements@steeleye.com>

[-- Attachment #2: nbd-sysfs-framework.diff --]
[-- Type: text/x-patch, Size: 4183 bytes --]

Description: The patch creates a new sysfs entry framework for nbd. The
single existing sysfs entry for nbd (pid) now uses the framework.

From: Paul Clements <paul.clements@steeleye.com>
Signed-off-by: Paul Clements <paul.clements@steeleye.com>
---

 drivers/block/nbd.c |   87 ++++++++++++++++++++++++++++++++++++++--------------
 include/linux/nbd.h |    1 
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e6fc716..23f3619 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -383,39 +383,17 @@ harderror:
 	return NULL;
 }
 
-static ssize_t pid_show(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct gendisk *disk = dev_to_disk(dev);
-
-	return sprintf(buf, "%ld\n",
-		(long) ((struct nbd_device *)disk->private_data)->pid);
-}
-
-static struct device_attribute pid_attr = {
-	.attr = { .name = "pid", .mode = S_IRUGO},
-	.show = pid_show,
-};
-
 static int nbd_do_it(struct nbd_device *lo)
 {
 	struct request *req;
-	int ret;
 
 	BUG_ON(lo->magic != LO_MAGIC);
 
 	lo->pid = current->pid;
-	ret = sysfs_create_file(&disk_to_dev(lo->disk)->kobj, &pid_attr.attr);
-	if (ret) {
-		printk(KERN_ERR "nbd: sysfs_create_file failed!");
-		lo->pid = 0;
-		return ret;
-	}
 
 	while ((req = nbd_read_stat(lo)) != NULL)
 		nbd_end_request(req);
 
-	sysfs_remove_file(&disk_to_dev(lo->disk)->kobj, &pid_attr.attr);
 	lo->pid = 0;
 	return 0;
 }
@@ -735,6 +713,67 @@ static const struct block_device_operations nbd_fops =
  *  (Just smiley confuses emacs :-)
  */
 
+struct nbd_sysfs_entry {
+	struct attribute attr;
+	ssize_t (*show)(struct nbd_device *, char *);
+	ssize_t (*store)(struct nbd_device *, const char *, size_t);
+};
+
+static ssize_t pid_show(struct nbd_device *lo, char *page)
+{
+	return sprintf(page, "%ld\n", (long)lo->pid);
+}
+
+static struct nbd_sysfs_entry nbd_pid =
+__ATTR(pid, S_IRUGO, pid_show, NULL);
+
+static struct attribute *nbd_default_attrs[] = {
+	&nbd_pid.attr,
+	NULL,
+};
+
+static ssize_t
+nbd_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
+{
+	struct nbd_sysfs_entry *entry = container_of(attr, struct nbd_sysfs_entry, attr);
+	struct nbd_device *lo = container_of(kobj, struct nbd_device, kobj);
+	ssize_t rv;
+
+	if (!entry->show)
+		return -EIO;
+	rv = entry->show(lo, page);
+	return rv;
+}
+
+static ssize_t
+nbd_attr_store(struct kobject *kobj, struct attribute *attr,
+	      const char *page, size_t length)
+{
+	struct nbd_sysfs_entry *entry = container_of(attr, struct nbd_sysfs_entry, attr);
+	struct nbd_device *lo = container_of(kobj, struct nbd_device, kobj);
+	ssize_t rv;
+
+	if (!entry->store)
+		return -EIO;
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	mutex_lock(&lo->tx_lock);
+	rv = entry->store(lo, page, length);
+	mutex_unlock(&lo->tx_lock);
+
+	return rv;
+}
+
+static const struct sysfs_ops nbd_sysfs_ops = {
+	.show   = nbd_attr_show,
+	.store  = nbd_attr_store,
+};
+
+static struct kobj_type nbd_ktype = {
+	.sysfs_ops      = &nbd_sysfs_ops,
+	.default_attrs  = nbd_default_attrs,
+};
+
 static int __init nbd_init(void)
 {
 	int err = -ENOMEM;
@@ -786,6 +825,7 @@ static int __init nbd_init(void)
 	dprintk(DBG_INIT, "nbd: debugflags=0x%x\n", debugflags);
 
 	for (i = 0; i < nbds_max; i++) {
+		int error;
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].file = NULL;
 		nbd_dev[i].magic = LO_MAGIC;
@@ -805,6 +845,8 @@ static int __init nbd_init(void)
 		sprintf(disk->disk_name, "nbd%d", i);
 		set_capacity(disk, 0);
 		add_disk(disk);
+		error = kobject_init_and_add(&nbd_dev[i].kobj, &nbd_ktype,
+				&disk_to_dev(disk)->kobj, "%s", "nbd");
 	}
 
 	return 0;
@@ -828,6 +870,7 @@ static void __exit nbd_cleanup(void)
 			blk_cleanup_queue(disk->queue);
 			put_disk(disk);
 		}
+		kobject_del(&nbd_dev[i].kobj);
 	}
 	unregister_blkdev(NBD_MAJOR, "nbd");
 	kfree(nbd_dev);
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index d146ca1..c795e28 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -68,6 +68,7 @@ struct nbd_device {
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
+	struct kobject kobj;
 };
 
 #endif

Re: [PATCH] nbd: nbd sysfs framework

[Greg KH]

On Wed, Aug 03, 2011 at 07:15:51PM -0400, Paul Clements wrote:
> Description: The patch creates a new sysfs entry framework for nbd.

Why?  What does this buy us except an increase in code size for no added
benifit?  You seem to be stripping out the driver layer here and using
"raw" kobjects, why?  That's generally never a good idea (hint, I think
you just caused a race and userspace bug with your change as well
because of this...)

confused,

greg k-h

Re: [PATCH] nbd: nbd sysfs framework

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Wed, Aug 3, 2011 at 7:34 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, Aug 03, 2011 at 07:15:51PM -0400, Paul Clements wrote:
>> Description: The patch creates a new sysfs entry framework for nbd.
>
> Why?  What does this buy us except an increase in code size for no added
> benifit?  You seem to be stripping out the driver layer here and using
> "raw" kobjects, why?

Not sure what I was thinking...just in a hurry, I think.

Is the following a little better?

Thanks for your feedback.

--
Paul

[-- Attachment #2: tmp.diff --]
[-- Type: text/x-patch, Size: 3725 bytes --]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e6fc716..a1e7615 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -383,39 +383,17 @@ harderror:
 	return NULL;
 }
 
-static ssize_t pid_show(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct gendisk *disk = dev_to_disk(dev);
-
-	return sprintf(buf, "%ld\n",
-		(long) ((struct nbd_device *)disk->private_data)->pid);
-}
-
-static struct device_attribute pid_attr = {
-	.attr = { .name = "pid", .mode = S_IRUGO},
-	.show = pid_show,
-};
-
 static int nbd_do_it(struct nbd_device *lo)
 {
 	struct request *req;
-	int ret;
 
 	BUG_ON(lo->magic != LO_MAGIC);
 
 	lo->pid = current->pid;
-	ret = sysfs_create_file(&disk_to_dev(lo->disk)->kobj, &pid_attr.attr);
-	if (ret) {
-		printk(KERN_ERR "nbd: sysfs_create_file failed!");
-		lo->pid = 0;
-		return ret;
-	}
 
 	while ((req = nbd_read_stat(lo)) != NULL)
 		nbd_end_request(req);
 
-	sysfs_remove_file(&disk_to_dev(lo->disk)->kobj, &pid_attr.attr);
 	lo->pid = 0;
 	return 0;
 }
@@ -735,6 +713,75 @@ static const struct block_device_operations nbd_fops =
  *  (Just smiley confuses emacs :-)
  */
 
+struct nbd_sysfs_entry {
+	struct device_attribute dev_attr;
+	ssize_t (*show)(struct nbd_device *, char *);
+	ssize_t (*store)(struct nbd_device *, const char *, size_t);
+};
+
+static ssize_t pid_show(struct nbd_device *lo, char *page)
+{
+	return sprintf(page, "%ld\n", (long)lo->pid);
+}
+
+#define NBD_ATTR(_var, _name, _perm, _show, _store) \
+	static struct nbd_sysfs_entry _var = { \
+	.dev_attr = __ATTR(_name, _perm, nbd_attr_show, nbd_attr_store), \
+	.show = _show, \
+	.store = _store \
+}
+
+// static struct nbd_sysfs_entry nbd_pid =
+// __ATTR(pid, S_IRUGO, pid_show, NULL);
+
+static ssize_t
+nbd_attr_show(struct device *dev, struct device_attribute *attr, char *page)
+{
+	struct nbd_sysfs_entry *entry = container_of(attr,
+			struct nbd_sysfs_entry, dev_attr);
+	struct nbd_device *lo =
+			(struct nbd_device *)dev_to_disk(dev)->private_data;
+	ssize_t rv;
+
+	if (!entry->show || !lo)
+		return -EIO;
+	rv = entry->show(lo, page);
+	return rv;
+}
+
+static ssize_t
+nbd_attr_store(struct device *dev, struct device_attribute *attr,
+	      const char *page, size_t length)
+{
+	struct nbd_sysfs_entry *entry = container_of(attr,
+			struct nbd_sysfs_entry, dev_attr);
+	struct nbd_device *lo =
+			(struct nbd_device *)dev_to_disk(dev)->private_data;
+	ssize_t rv;
+
+	if (!entry->store)
+		return -EIO;
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	mutex_lock(&lo->tx_lock);
+	rv = entry->store(lo, page, length);
+	mutex_unlock(&lo->tx_lock);
+
+	return rv;
+}
+
+NBD_ATTR(nbd_pid, pid, S_IRUGO, pid_show, NULL);
+
+static struct attribute *nbd_attrs[] = {
+	&nbd_pid.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group nbd_attr_group = {
+	.name = "nbd",
+	.attrs = nbd_attrs,
+};
+
 static int __init nbd_init(void)
 {
 	int err = -ENOMEM;
@@ -786,6 +833,7 @@ static int __init nbd_init(void)
 	dprintk(DBG_INIT, "nbd: debugflags=0x%x\n", debugflags);
 
 	for (i = 0; i < nbds_max; i++) {
+		int error;
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].file = NULL;
 		nbd_dev[i].magic = LO_MAGIC;
@@ -805,6 +853,8 @@ static int __init nbd_init(void)
 		sprintf(disk->disk_name, "nbd%d", i);
 		set_capacity(disk, 0);
 		add_disk(disk);
+		error = sysfs_create_group(&disk_to_dev(disk)->kobj,
+				&nbd_attr_group);
 	}
 
 	return 0;
@@ -824,6 +874,8 @@ static void __exit nbd_cleanup(void)
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].magic = 0;
 		if (disk) {
+			sysfs_remove_group(&disk_to_dev(disk)->kobj,
+					&nbd_attr_group);
 			del_gendisk(disk);
 			blk_cleanup_queue(disk->queue);
 			put_disk(disk);

Re: [PATCH] nbd: nbd sysfs framework

[Greg KH]

On Wed, Aug 03, 2011 at 10:17:04PM -0400, Paul Clements wrote:
> On Wed, Aug 3, 2011 at 7:34 PM, Greg KH <greg@kroah.com> wrote:
> > On Wed, Aug 03, 2011 at 07:15:51PM -0400, Paul Clements wrote:
> >> Description: The patch creates a new sysfs entry framework for nbd.
> >
> > Why?  What does this buy us except an increase in code size for no added
> > benifit?  You seem to be stripping out the driver layer here and using
> > "raw" kobjects, why?
> 
> Not sure what I was thinking...just in a hurry, I think.
> 
> Is the following a little better?

I don't know, I still fail to understand _why_ this patch is needed.

Why are you doing this?  What is the goal?  What is wrong with the
existing code that it doesn't work for you?

confused,

greg k-h

Re: [PATCH] nbd: nbd sysfs framework

[Paul Clements]

On Thu, Aug 4, 2011 at 1:22 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Aug 03, 2011 at 10:17:04PM -0400, Paul Clements wrote:
>> On Wed, Aug 3, 2011 at 7:34 PM, Greg KH <greg@kroah.com> wrote:
>> > On Wed, Aug 03, 2011 at 07:15:51PM -0400, Paul Clements wrote:
>> >> Description: The patch creates a new sysfs entry framework for nbd.
>> >
>> > Why?  What does this buy us except an increase in code size for no added
>> > benifit?  You seem to be stripping out the driver layer here and using
>> > "raw" kobjects, why?
>>
>> Not sure what I was thinking...just in a hurry, I think.
>>
>> Is the following a little better?
>
> I don't know, I still fail to understand _why_ this patch is needed.
>
> Why are you doing this?  What is the goal?  What is wrong with the
> existing code that it doesn't work for you?

So, first, the point of the framework, as I'm calling it, is (and
several other drivers do similar things...) to avoid the whole kobj to
device to gendisk to driver-specific struct walking in every single
sysfs handler routine. So I've created the meta-structure
nbd_sysfs_entry to allow me to have two routines that do the walking
(and any other common tasks) and then hand off (via function pointer)
to the actual handler for the particular sysfs entry. Consequently,
most of the sysfs entry handlers are just one line long.

The second goal is to avoid adding more ioctls to nbd, as it's already
got enough. Eventually, we may even reduce the number of ioctls in
nbd. Yeah, sysfs is not a panacea, but it tends to be easier to
describe and work with and avoids some of the cross-platform headaches
of ioctls.

Thirdly, support for FUA and FLUSH (and other features) can be added
once we've exposed the nbd device flags via sysfs. This is being
requested by nbd users and has been discussed on nbd-general.

Thanks again for your feedback. I'll add the documentation and other
bits that you asked about.

--
Paul