On Sat, Mar 28, 2020 at 8:31 PM Bart Van Assche [off-list ref] wrote:

On 2020-03-20 05:16, Jack Wang wrote:

quoted

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

quoted

+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.

quoted

+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

quoted

+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.

quoted

+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.

quoted

+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.