| 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 |
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.
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
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 --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); +}
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