diff mbox series

[v11,23/26] block/rnbd: server: sysfs interface functions

Message ID 20200320121657.1165-24-jinpu.wang@cloud.ionos.com (mailing list archive)
State Changes Requested
Headers show
Series RTRS (former IBTRS) RDMA Transport Library and RNBD (former IBNBD) RDMA Network Block Device | expand

Commit Message

Jinpu Wang March 20, 2020, 12:16 p.m. UTC
This is the sysfs interface to rnbd mapped devices on server side:

  /sys/devices/virtual/rnbd-server/ctl/devices/<device_name>/
    |- block_dev
    |  *** link pointing to the corresponding block device sysfs entry
    |
    |- sessions/<session-name>/
    |  *** sessions directory
       |
       |- read_only
       |  *** is devices mapped as read only
       |
       |- mapping_path
          *** relative device path provided by the client during mapping

Signed-off-by: Danil Kipnis <danil.kipnis@cloud.ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/block/rnbd/rnbd-srv-sysfs.c | 211 ++++++++++++++++++++++++++++
 1 file changed, 211 insertions(+)
 create mode 100644 drivers/block/rnbd/rnbd-srv-sysfs.c

Comments

Bart Van Assche March 28, 2020, 7:31 p.m. UTC | #1
On 2020-03-20 05:16, Jack Wang wrote:
> This is the sysfs interface to rnbd mapped devices on server side:
> 
>   /sys/devices/virtual/rnbd-server/ctl/devices/<device_name>/
>     |- block_dev
>     |  *** link pointing to the corresponding block device sysfs entry
>     |
>     |- sessions/<session-name>/
>     |  *** sessions directory
>        |
>        |- read_only
>        |  *** is devices mapped as read only
>        |
>        |- mapping_path
>           *** relative device path provided by the client during mapping
> 

> +static struct kobj_type ktype = {
> +	.sysfs_ops	= &kobj_sysfs_ops,
> +};

From Documentation/kobject.txt: "One important point cannot be
overstated: every kobject must have a release() method." I think this is
something that Greg KH feels very strongly about. Please fix this.

> +int rnbd_srv_create_dev_sysfs(struct rnbd_srv_dev *dev,
> +			       struct block_device *bdev,
> +			       const char *dir_name)
> +{
> +	struct kobject *bdev_kobj;
> +	int ret;
> +
> +	ret = kobject_init_and_add(&dev->dev_kobj, &ktype,
> +				   rnbd_devs_kobj, dir_name);
> +	if (ret)
> +		return ret;
> +
> +	ret = kobject_init_and_add(&dev->dev_sessions_kobj,
> +				   &ktype,
> +				   &dev->dev_kobj, "sessions");
> +	if (ret)
> +		goto err;
> +
> +	bdev_kobj = &disk_to_dev(bdev->bd_disk)->kobj;
> +	ret = sysfs_create_link(&dev->dev_kobj, bdev_kobj, "block_dev");
> +	if (ret)
> +		goto err2;
> +
> +	return 0;
> +
> +err2:
> +	kobject_put(&dev->dev_sessions_kobj);
> +err:
> +	kobject_put(&dev->dev_kobj);
> +	return ret;
> +}

Please choose more descriptive names for the goto labels, e.g.
put_sess_kobj and put_dev_kobj.

> +static ssize_t read_only_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			      char *page)
> +{
> +	struct rnbd_srv_sess_dev *sess_dev;
> +
> +	sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
> +
> +	return scnprintf(page, PAGE_SIZE, "%s\n",
> +			 (sess_dev->open_flags & FMODE_WRITE) ? "0" : "1");
> +}

The scnprintf() statement looks overcomplicated. How about the following?

return scnprintf(page, PAGE_SIZE, "%d\n",
                 (sess_dev->open_flags & FMODE_WRITE) != 0);

> +void rnbd_srv_destroy_dev_session_sysfs(struct rnbd_srv_sess_dev *sess_dev)
> +{
> +	DECLARE_COMPLETION_ONSTACK(sysfs_compl);
> +
> +	sysfs_remove_group(&sess_dev->kobj,
> +			   &rnbd_srv_default_dev_session_attr_group);
> +
> +	sess_dev->sysfs_release_compl = &sysfs_compl;
> +	kobject_del(&sess_dev->kobj);
> +	kobject_put(&sess_dev->kobj);
> +	wait_for_completion(&sysfs_compl);
> +}

Why is there a wait_for_completion() call in the above function? I think
Greg KH strongly disagrees with such calls in functions that remove
sysfs attributes.

> +int rnbd_srv_create_sysfs_files(void)
> +{
> +	int err;
> +
> +	rnbd_dev_class = class_create(THIS_MODULE, "rnbd-server");
> +	if (IS_ERR(rnbd_dev_class))
> +		return PTR_ERR(rnbd_dev_class);
> +
> +	rnbd_dev = device_create(rnbd_dev_class, NULL,
> +				  MKDEV(0, 0), NULL, "ctl");
> +	if (IS_ERR(rnbd_dev)) {
> +		err = PTR_ERR(rnbd_dev);
> +		goto cls_destroy;
> +	}
> +	rnbd_devs_kobj = kobject_create_and_add("devices", &rnbd_dev->kobj);
> +	if (!rnbd_devs_kobj) {
> +		err = -ENOMEM;
> +		goto dev_destroy;
> +	}
> +
> +	return 0;
> +
> +dev_destroy:
> +	device_destroy(rnbd_dev_class, MKDEV(0, 0));
> +cls_destroy:
> +	class_destroy(rnbd_dev_class);
> +
> +	return err;
> +}

Please mention the device class in the description of this patch.

Thanks,

Bart.
Jason Gunthorpe March 28, 2020, 11:06 p.m. UTC | #2
On Sat, Mar 28, 2020 at 12:31:08PM -0700, Bart Van Assche wrote:
> On 2020-03-20 05:16, Jack Wang wrote:
> > This is the sysfs interface to rnbd mapped devices on server side:
> > 
> >   /sys/devices/virtual/rnbd-server/ctl/devices/<device_name>/
> >     |- block_dev
> >     |  *** link pointing to the corresponding block device sysfs entry
> >     |
> >     |- sessions/<session-name>/
> >     |  *** sessions directory
> >        |
> >        |- read_only
> >        |  *** is devices mapped as read only
> >        |
> >        |- mapping_path
> >           *** relative device path provided by the client during mapping
> > 
> 
> > +static struct kobj_type ktype = {
> > +	.sysfs_ops	= &kobj_sysfs_ops,
> > +};
> 
> From Documentation/kobject.txt: "One important point cannot be
> overstated: every kobject must have a release() method." I think this is
> something that Greg KH feels very strongly about. Please fix this.

more importantly you can't implement kobjects correctly without a
release so there is some bug in here.

Jason
Danil Kipnis March 30, 2020, 1:14 p.m. UTC | #3
On Sat, Mar 28, 2020 at 8:31 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2020-03-20 05:16, Jack Wang wrote:
> > This is the sysfs interface to rnbd mapped devices on server side:
> >
> >   /sys/devices/virtual/rnbd-server/ctl/devices/<device_name>/
> >     |- block_dev
> >     |  *** link pointing to the corresponding block device sysfs entry
> >     |
> >     |- sessions/<session-name>/
> >     |  *** sessions directory
> >        |
> >        |- read_only
> >        |  *** is devices mapped as read only
> >        |
> >        |- mapping_path
> >           *** relative device path provided by the client during mapping
> >
>
> > +static struct kobj_type ktype = {
> > +     .sysfs_ops      = &kobj_sysfs_ops,
> > +};
>
> From Documentation/kobject.txt: "One important point cannot be
> overstated: every kobject must have a release() method." I think this is
> something that Greg KH feels very strongly about. Please fix this.

OK.


>
> > +int rnbd_srv_create_dev_sysfs(struct rnbd_srv_dev *dev,
> > +                            struct block_device *bdev,
> > +                            const char *dir_name)
> > +{
> > +     struct kobject *bdev_kobj;
> > +     int ret;
> > +
> > +     ret = kobject_init_and_add(&dev->dev_kobj, &ktype,
> > +                                rnbd_devs_kobj, dir_name);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = kobject_init_and_add(&dev->dev_sessions_kobj,
> > +                                &ktype,
> > +                                &dev->dev_kobj, "sessions");
> > +     if (ret)
> > +             goto err;
> > +
> > +     bdev_kobj = &disk_to_dev(bdev->bd_disk)->kobj;
> > +     ret = sysfs_create_link(&dev->dev_kobj, bdev_kobj, "block_dev");
> > +     if (ret)
> > +             goto err2;
> > +
> > +     return 0;
> > +
> > +err2:
> > +     kobject_put(&dev->dev_sessions_kobj);
> > +err:
> > +     kobject_put(&dev->dev_kobj);
> > +     return ret;
> > +}
>
> Please choose more descriptive names for the goto labels, e.g.
> put_sess_kobj and put_dev_kobj.

OK


>
> > +static ssize_t read_only_show(struct kobject *kobj, struct kobj_attribute *attr,
> > +                           char *page)
> > +{
> > +     struct rnbd_srv_sess_dev *sess_dev;
> > +
> > +     sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
> > +
> > +     return scnprintf(page, PAGE_SIZE, "%s\n",
> > +                      (sess_dev->open_flags & FMODE_WRITE) ? "0" : "1");
> > +}
>
> The scnprintf() statement looks overcomplicated. How about the following?
>
> return scnprintf(page, PAGE_SIZE, "%d\n",
>                  (sess_dev->open_flags & FMODE_WRITE) != 0);

Looks better, thanks.


> > +void rnbd_srv_destroy_dev_session_sysfs(struct rnbd_srv_sess_dev *sess_dev)
> > +{
> > +     DECLARE_COMPLETION_ONSTACK(sysfs_compl);
> > +
> > +     sysfs_remove_group(&sess_dev->kobj,
> > +                        &rnbd_srv_default_dev_session_attr_group);
> > +
> > +     sess_dev->sysfs_release_compl = &sysfs_compl;
> > +     kobject_del(&sess_dev->kobj);
> > +     kobject_put(&sess_dev->kobj);
> > +     wait_for_completion(&sysfs_compl);
> > +}
>
> Why is there a wait_for_completion() call in the above function? I think
> Greg KH strongly disagrees with such calls in functions that remove
> sysfs attributes.

This just makes the function wait until the sysfs release function is
called, so that sess_dev can be freed afterwards. Will try to get rid
of the completion and call the rnbd_destroy_sess_dev() which frees the
struct directly from the release function.


>
> > +int rnbd_srv_create_sysfs_files(void)
> > +{
> > +     int err;
> > +
> > +     rnbd_dev_class = class_create(THIS_MODULE, "rnbd-server");
> > +     if (IS_ERR(rnbd_dev_class))
> > +             return PTR_ERR(rnbd_dev_class);
> > +
> > +     rnbd_dev = device_create(rnbd_dev_class, NULL,
> > +                               MKDEV(0, 0), NULL, "ctl");
> > +     if (IS_ERR(rnbd_dev)) {
> > +             err = PTR_ERR(rnbd_dev);
> > +             goto cls_destroy;
> > +     }
> > +     rnbd_devs_kobj = kobject_create_and_add("devices", &rnbd_dev->kobj);
> > +     if (!rnbd_devs_kobj) {
> > +             err = -ENOMEM;
> > +             goto dev_destroy;
> > +     }
> > +
> > +     return 0;
> > +
> > +dev_destroy:
> > +     device_destroy(rnbd_dev_class, MKDEV(0, 0));
> > +cls_destroy:
> > +     class_destroy(rnbd_dev_class);
> > +
> > +     return err;
> > +}
>
> Please mention the device class in the description of this patch.

OK,

Thanks,

Danil
>
> Thanks,
>
> Bart.
diff mbox series

Patch

diff --git a/drivers/block/rnbd/rnbd-srv-sysfs.c b/drivers/block/rnbd/rnbd-srv-sysfs.c
new file mode 100644
index 000000000000..15dff4ede37e
--- /dev/null
+++ b/drivers/block/rnbd/rnbd-srv-sysfs.c
@@ -0,0 +1,211 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * RDMA Network Block Driver
+ *
+ * Copyright (c) 2014 - 2018 ProfitBricks GmbH. All rights reserved.
+ * Copyright (c) 2018 - 2019 1&1 IONOS Cloud GmbH. All rights reserved.
+ * Copyright (c) 2019 - 2020 1&1 IONOS SE. All rights reserved.
+ */
+#undef pr_fmt
+#define pr_fmt(fmt) KBUILD_MODNAME " L" __stringify(__LINE__) ": " fmt
+
+#include <uapi/linux/limits.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/stat.h>
+#include <linux/genhd.h>
+#include <linux/list.h>
+#include <linux/moduleparam.h>
+#include <linux/device.h>
+
+#include "rnbd-srv.h"
+
+static struct device *rnbd_dev;
+static struct class *rnbd_dev_class;
+static struct kobject *rnbd_devs_kobj;
+
+static struct kobj_type ktype = {
+	.sysfs_ops	= &kobj_sysfs_ops,
+};
+
+int rnbd_srv_create_dev_sysfs(struct rnbd_srv_dev *dev,
+			       struct block_device *bdev,
+			       const char *dir_name)
+{
+	struct kobject *bdev_kobj;
+	int ret;
+
+	ret = kobject_init_and_add(&dev->dev_kobj, &ktype,
+				   rnbd_devs_kobj, dir_name);
+	if (ret)
+		return ret;
+
+	ret = kobject_init_and_add(&dev->dev_sessions_kobj,
+				   &ktype,
+				   &dev->dev_kobj, "sessions");
+	if (ret)
+		goto err;
+
+	bdev_kobj = &disk_to_dev(bdev->bd_disk)->kobj;
+	ret = sysfs_create_link(&dev->dev_kobj, bdev_kobj, "block_dev");
+	if (ret)
+		goto err2;
+
+	return 0;
+
+err2:
+	kobject_put(&dev->dev_sessions_kobj);
+err:
+	kobject_put(&dev->dev_kobj);
+	return ret;
+}
+
+void rnbd_srv_destroy_dev_sysfs(struct rnbd_srv_dev *dev)
+{
+	sysfs_remove_link(&dev->dev_kobj, "block_dev");
+	kobject_del(&dev->dev_sessions_kobj);
+	kobject_put(&dev->dev_sessions_kobj);
+	kobject_del(&dev->dev_kobj);
+	kobject_put(&dev->dev_kobj);
+}
+
+static ssize_t read_only_show(struct kobject *kobj, struct kobj_attribute *attr,
+			      char *page)
+{
+	struct rnbd_srv_sess_dev *sess_dev;
+
+	sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
+
+	return scnprintf(page, PAGE_SIZE, "%s\n",
+			 (sess_dev->open_flags & FMODE_WRITE) ? "0" : "1");
+}
+
+static struct kobj_attribute rnbd_srv_dev_session_ro_attr =
+	__ATTR_RO(read_only);
+
+static ssize_t access_mode_show(struct kobject *kobj,
+				struct kobj_attribute *attr,
+				char *page)
+{
+	struct rnbd_srv_sess_dev *sess_dev;
+
+	sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
+
+	return scnprintf(page, PAGE_SIZE, "%s\n",
+			 rnbd_access_mode_str(sess_dev->access_mode));
+}
+
+static struct kobj_attribute rnbd_srv_dev_session_access_mode_attr =
+	__ATTR_RO(access_mode);
+
+static ssize_t mapping_path_show(struct kobject *kobj,
+				 struct kobj_attribute *attr, char *page)
+{
+	struct rnbd_srv_sess_dev *sess_dev;
+
+	sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
+
+	return scnprintf(page, PAGE_SIZE, "%s\n", sess_dev->pathname);
+}
+
+static struct kobj_attribute rnbd_srv_dev_session_mapping_path_attr =
+	__ATTR_RO(mapping_path);
+
+static struct attribute *rnbd_srv_default_dev_sessions_attrs[] = {
+	&rnbd_srv_dev_session_access_mode_attr.attr,
+	&rnbd_srv_dev_session_ro_attr.attr,
+	&rnbd_srv_dev_session_mapping_path_attr.attr,
+	NULL,
+};
+
+static struct attribute_group rnbd_srv_default_dev_session_attr_group = {
+	.attrs = rnbd_srv_default_dev_sessions_attrs,
+};
+
+void rnbd_srv_destroy_dev_session_sysfs(struct rnbd_srv_sess_dev *sess_dev)
+{
+	DECLARE_COMPLETION_ONSTACK(sysfs_compl);
+
+	sysfs_remove_group(&sess_dev->kobj,
+			   &rnbd_srv_default_dev_session_attr_group);
+
+	sess_dev->sysfs_release_compl = &sysfs_compl;
+	kobject_del(&sess_dev->kobj);
+	kobject_put(&sess_dev->kobj);
+	wait_for_completion(&sysfs_compl);
+}
+
+static void rnbd_srv_sess_dev_release(struct kobject *kobj)
+{
+	struct rnbd_srv_sess_dev *sess_dev;
+
+	sess_dev = container_of(kobj, struct rnbd_srv_sess_dev, kobj);
+	if (sess_dev->sysfs_release_compl)
+		complete_all(sess_dev->sysfs_release_compl);
+}
+
+static struct kobj_type rnbd_srv_sess_dev_ktype = {
+	.sysfs_ops	= &kobj_sysfs_ops,
+	.release	= rnbd_srv_sess_dev_release,
+};
+
+int rnbd_srv_create_dev_session_sysfs(struct rnbd_srv_sess_dev *sess_dev)
+{
+	int ret;
+
+	ret = kobject_init_and_add(&sess_dev->kobj, &rnbd_srv_sess_dev_ktype,
+				   &sess_dev->dev->dev_sessions_kobj, "%s",
+				   sess_dev->sess->sessname);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&sess_dev->kobj,
+				 &rnbd_srv_default_dev_session_attr_group);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	kobject_put(&sess_dev->kobj);
+
+	return ret;
+}
+
+int rnbd_srv_create_sysfs_files(void)
+{
+	int err;
+
+	rnbd_dev_class = class_create(THIS_MODULE, "rnbd-server");
+	if (IS_ERR(rnbd_dev_class))
+		return PTR_ERR(rnbd_dev_class);
+
+	rnbd_dev = device_create(rnbd_dev_class, NULL,
+				  MKDEV(0, 0), NULL, "ctl");
+	if (IS_ERR(rnbd_dev)) {
+		err = PTR_ERR(rnbd_dev);
+		goto cls_destroy;
+	}
+	rnbd_devs_kobj = kobject_create_and_add("devices", &rnbd_dev->kobj);
+	if (!rnbd_devs_kobj) {
+		err = -ENOMEM;
+		goto dev_destroy;
+	}
+
+	return 0;
+
+dev_destroy:
+	device_destroy(rnbd_dev_class, MKDEV(0, 0));
+cls_destroy:
+	class_destroy(rnbd_dev_class);
+
+	return err;
+}
+
+void rnbd_srv_destroy_sysfs_files(void)
+{
+	kobject_del(rnbd_devs_kobj);
+	kobject_put(rnbd_devs_kobj);
+	device_destroy(rnbd_dev_class, MKDEV(0, 0));
+	class_destroy(rnbd_dev_class);
+}