| Message ID | 517A6B55.1010009@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Josh Durgin <josh.durgin@inktank.com> On 04/26/2013 04:56 AM, Alex Elder wrote: > When an rbd image gets mapped a device entry gets created for it > under /sys/bus/rbd/devices/<id>/. Inside that directory there are > sysfs files that contain information about the image: its size, > feature bits, major device number, and so on. > > Additionally, if that image has any snapshots, a device entry gets > created for each of those as a "child" of the mapped device. Each > of these is a subdirectory of the mapped device, and each directory > contains a few files with information about the snapshot (its > snapshot id, size, and feature mask). > > There is no clear benefit to having those device entries for the > snapshots. The information provided via sysfs of of little real > value--and all of it is available via rbd CLI commands. If we > still wanted to see the kernel's view of this information it could > be done much more simply by including it in a single sysfs file for > the mapped image. > > But there *is* a clear cost to supporting them. Every time a snapshot > context changes, these entries need to be updated (deleted snapshots > removed, new snapshots created). The rbd driver is notified of > changes to the snapshot context via callbacks from an osd, and care > must be taken to coordinate removal of snapshot data structures > with the possibility of one these notifications occurring. > > Things would be considerably simpler if we just didn't have to > maintain device entries for the snapshots. > > So get rid of them. > > The ability to map a snapshot of an rbd image will remain; the only > thing lost will be the ability to query these sysfs directories for > information about snapshots of mapped images. > > This resolves: > http://tracker.ceph.com/issues/4796 > > Signed-off-by: Alex Elder <elder@inktank.com> > --- > Documentation/ABI/testing/sysfs-bus-rbd | 20 ----- > drivers/block/rbd.c | 137 > +------------------------------ > 2 files changed, 4 insertions(+), 153 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-rbd > b/Documentation/ABI/testing/sysfs-bus-rbd > index cd9213c..0a30647 100644 > --- a/Documentation/ABI/testing/sysfs-bus-rbd > +++ b/Documentation/ABI/testing/sysfs-bus-rbd > @@ -66,27 +66,7 @@ current_snap > > The current snapshot for which the device is mapped. > > -snap_* > - > - A directory per each snapshot > - > parent > > Information identifying the pool, image, and snapshot id for > the parent image in a layered rbd image (format 2 only). > - > -Entries under /sys/bus/rbd/devices/<dev-id>/snap_<snap-name> > -------------------------------------------------------------- > - > -snap_id > - > - The rados internal snapshot id assigned for this snapshot > - > -snap_size > - > - The size of the image when this snapshot was taken. > - > -snap_features > - > - A hexadecimal encoding of the feature bits for this snapshot. > - > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 4d99d40..515fbf9 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -272,7 +272,6 @@ struct rbd_img_request { > list_for_each_entry_safe_reverse(oreq, n, &(ireq)->obj_requests, links) > > struct rbd_snap { > - struct device dev; > const char *name; > u64 size; > struct list_head node; > @@ -358,7 +357,6 @@ static DEFINE_SPINLOCK(rbd_client_list_lock); > static int rbd_img_request_submit(struct rbd_img_request *img_request); > > static int rbd_dev_snaps_update(struct rbd_device *rbd_dev); > -static int rbd_dev_snaps_register(struct rbd_device *rbd_dev); > > static void rbd_dev_release(struct device *dev); > static void rbd_remove_snap_dev(struct rbd_snap *snap); > @@ -3069,8 +3067,6 @@ static int rbd_dev_v1_refresh(struct rbd_device > *rbd_dev, u64 *hver) > kfree(h.object_prefix); > > ret = rbd_dev_snaps_update(rbd_dev); > - if (!ret) > - ret = rbd_dev_snaps_register(rbd_dev); > > up_write(&rbd_dev->header_rwsem); > > @@ -3344,71 +3340,6 @@ static struct device_type rbd_device_type = { > .release = rbd_sysfs_dev_release, > }; > > - > -/* > - sysfs - snapshots > -*/ > - > -static ssize_t rbd_snap_size_show(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); > - > - return sprintf(buf, "%llu\n", (unsigned long long)snap->size); > -} > - > -static ssize_t rbd_snap_id_show(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); > - > - return sprintf(buf, "%llu\n", (unsigned long long)snap->id); > -} > - > -static ssize_t rbd_snap_features_show(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); > - > - return sprintf(buf, "0x%016llx\n", > - (unsigned long long) snap->features); > -} > - > -static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL); > -static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL); > -static DEVICE_ATTR(snap_features, S_IRUGO, rbd_snap_features_show, NULL); > - > -static struct attribute *rbd_snap_attrs[] = { > - &dev_attr_snap_size.attr, > - &dev_attr_snap_id.attr, > - &dev_attr_snap_features.attr, > - NULL, > -}; > - > -static struct attribute_group rbd_snap_attr_group = { > - .attrs = rbd_snap_attrs, > -}; > - > -static void rbd_snap_dev_release(struct device *dev) > -{ > - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); > - kfree(snap->name); > - kfree(snap); > -} > - > -static const struct attribute_group *rbd_snap_attr_groups[] = { > - &rbd_snap_attr_group, > - NULL > -}; > - > -static struct device_type rbd_snap_device_type = { > - .groups = rbd_snap_attr_groups, > - .release = rbd_snap_dev_release, > -}; > - > static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) > { > kref_get(&spec->kref); > @@ -3483,38 +3414,11 @@ static void rbd_dev_destroy(struct rbd_device > *rbd_dev) > kfree(rbd_dev); > } > > -static bool rbd_snap_registered(struct rbd_snap *snap) > -{ > - bool ret = snap->dev.type == &rbd_snap_device_type; > - bool reg = device_is_registered(&snap->dev); > - > - rbd_assert(!ret ^ reg); > - > - return ret; > -} > - > static void rbd_remove_snap_dev(struct rbd_snap *snap) > { > list_del(&snap->node); > - if (device_is_registered(&snap->dev)) > - device_unregister(&snap->dev); > -} > - > -static int rbd_register_snap_dev(struct rbd_snap *snap, > - struct device *parent) > -{ > - struct device *dev = &snap->dev; > - int ret; > - > - dev->type = &rbd_snap_device_type; > - dev->parent = parent; > - dev->release = rbd_snap_dev_release; > - dev_set_name(dev, "%s%s", RBD_SNAP_DEV_NAME_PREFIX, snap->name); > - dout("%s: registering device for snapshot %s\n", __func__, snap->name); > - > - ret = device_register(dev); > - > - return ret; > + kfree(snap->name); > + kfree(snap); > } > > static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev, > @@ -4089,8 +3993,6 @@ static int rbd_dev_v2_refresh(struct rbd_device > *rbd_dev, u64 *hver) > dout("rbd_dev_snaps_update returned %d\n", ret); > if (ret) > goto out; > - ret = rbd_dev_snaps_register(rbd_dev); > - dout("rbd_dev_snaps_register returned %d\n", ret); > out: > up_write(&rbd_dev->header_rwsem); > > @@ -4145,11 +4047,11 @@ static int rbd_dev_snaps_update(struct > rbd_device *rbd_dev) > */ > if (rbd_dev->spec->snap_id == snap->id) > clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags); > - rbd_remove_snap_dev(snap); > - dout("%ssnap id %llu has been removed\n", > + dout("removing %ssnap id %llu\n", > rbd_dev->spec->snap_id == snap->id ? > "mapped " : "", > (unsigned long long) snap->id); > + rbd_remove_snap_dev(snap); > > /* Done with this list entry; advance */ > > @@ -4209,31 +4111,6 @@ static int rbd_dev_snaps_update(struct rbd_device > *rbd_dev) > return 0; > } > > -/* > - * Scan the list of snapshots and register the devices for any that > - * have not already been registered. > - */ > -static int rbd_dev_snaps_register(struct rbd_device *rbd_dev) > -{ > - struct rbd_snap *snap; > - int ret = 0; > - > - dout("%s:\n", __func__); > - if (WARN_ON(!device_is_registered(&rbd_dev->dev))) > - return -EIO; > - > - list_for_each_entry(snap, &rbd_dev->snaps, node) { > - if (!rbd_snap_registered(snap)) { > - ret = rbd_register_snap_dev(snap, &rbd_dev->dev); > - if (ret < 0) > - break; > - } > - } > - dout("%s: returning %d\n", __func__, ret); > - > - return ret; > -} > - > static int rbd_bus_add_dev(struct rbd_device *rbd_dev) > { > struct device *dev; > @@ -4840,12 +4717,6 @@ static int rbd_dev_probe_finish(struct rbd_device > *rbd_dev) > rbd_dev->parent = parent; > } > > - down_write(&rbd_dev->header_rwsem); > - ret = rbd_dev_snaps_register(rbd_dev); > - up_write(&rbd_dev->header_rwsem); > - if (ret) > - goto err_out_bus; > - > ret = rbd_dev_header_watch_sync(rbd_dev, 1); > if (ret) > goto err_out_bus; > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/ABI/testing/sysfs-bus-rbd b/Documentation/ABI/testing/sysfs-bus-rbd index cd9213c..0a30647 100644 --- a/Documentation/ABI/testing/sysfs-bus-rbd +++ b/Documentation/ABI/testing/sysfs-bus-rbd @@ -66,27 +66,7 @@ current_snap The current snapshot for which the device is mapped. -snap_* - - A directory per each snapshot - parent Information identifying the pool, image, and snapshot id for the parent image in a layered rbd image (format 2 only). - -Entries under /sys/bus/rbd/devices/<dev-id>/snap_<snap-name> -------------------------------------------------------------- - -snap_id - - The rados internal snapshot id assigned for this snapshot - -snap_size - - The size of the image when this snapshot was taken. - -snap_features - - A hexadecimal encoding of the feature bits for this snapshot. - diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 4d99d40..515fbf9 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -272,7 +272,6 @@ struct rbd_img_request { list_for_each_entry_safe_reverse(oreq, n, &(ireq)->obj_requests, links) struct rbd_snap { - struct device dev; const char *name; u64 size; struct list_head node; @@ -358,7 +357,6 @@ static DEFINE_SPINLOCK(rbd_client_list_lock); static int rbd_img_request_submit(struct rbd_img_request *img_request); static int rbd_dev_snaps_update(struct rbd_device *rbd_dev); -static int rbd_dev_snaps_register(struct rbd_device *rbd_dev); static void rbd_dev_release(struct device *dev); static void rbd_remove_snap_dev(struct rbd_snap *snap); @@ -3069,8 +3067,6 @@ static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev, u64 *hver) kfree(h.object_prefix); ret = rbd_dev_snaps_update(rbd_dev); - if (!ret) - ret = rbd_dev_snaps_register(rbd_dev); up_write(&rbd_dev->header_rwsem);
When an rbd image gets mapped a device entry gets created for it under /sys/bus/rbd/devices/<id>/. Inside that directory there are sysfs files that contain information about the image: its size, feature bits, major device number, and so on. Additionally, if that image has any snapshots, a device entry gets created for each of those as a "child" of the mapped device. Each of these is a subdirectory of the mapped device, and each directory contains a few files with information about the snapshot (its snapshot id, size, and feature mask). There is no clear benefit to having those device entries for the snapshots. The information provided via sysfs of of little real value--and all of it is available via rbd CLI commands. If we still wanted to see the kernel's view of this information it could be done much more simply by including it in a single sysfs file for the mapped image. But there *is* a clear cost to supporting them. Every time a snapshot context changes, these entries need to be updated (deleted snapshots removed, new snapshots created). The rbd driver is notified of changes to the snapshot context via callbacks from an osd, and care must be taken to coordinate removal of snapshot data structures with the possibility of one these notifications occurring. Things would be considerably simpler if we just didn't have to maintain device entries for the snapshots. So get rid of them. The ability to map a snapshot of an rbd image will remain; the only thing lost will be the ability to query these sysfs directories for information about snapshots of mapped images. This resolves: http://tracker.ceph.com/issues/4796 Signed-off-by: Alex Elder <elder@inktank.com> --- Documentation/ABI/testing/sysfs-bus-rbd | 20 ----- drivers/block/rbd.c | 137 +------------------------------ 2 files changed, 4 insertions(+), 153 deletions(-) @@ -3344,71 +3340,6 @@ static struct device_type rbd_device_type = { .release = rbd_sysfs_dev_release, }; - -/* - sysfs - snapshots -*/ - -static ssize_t rbd_snap_size_show(struct device *dev, - struct device_attribute *attr, - char *buf) -{ - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); - - return sprintf(buf, "%llu\n", (unsigned long long)snap->size); -} - -static ssize_t rbd_snap_id_show(struct device *dev, - struct device_attribute *attr, - char *buf) -{ - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); - - return sprintf(buf, "%llu\n", (unsigned long long)snap->id); -} - -static ssize_t rbd_snap_features_show(struct device *dev, - struct device_attribute *attr, - char *buf) -{ - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); - - return sprintf(buf, "0x%016llx\n", - (unsigned long long) snap->features); -} - -static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL); -static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL); -static DEVICE_ATTR(snap_features, S_IRUGO, rbd_snap_features_show, NULL); - -static struct attribute *rbd_snap_attrs[] = { - &dev_attr_snap_size.attr, - &dev_attr_snap_id.attr, - &dev_attr_snap_features.attr, - NULL, -}; - -static struct attribute_group rbd_snap_attr_group = { - .attrs = rbd_snap_attrs, -}; - -static void rbd_snap_dev_release(struct device *dev) -{ - struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev); - kfree(snap->name); - kfree(snap); -} - -static const struct attribute_group *rbd_snap_attr_groups[] = { - &rbd_snap_attr_group, - NULL -}; - -static struct device_type rbd_snap_device_type = { - .groups = rbd_snap_attr_groups, - .release = rbd_snap_dev_release, -}; - static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) { kref_get(&spec->kref); @@ -3483,38 +3414,11 @@ static void rbd_dev_destroy(struct rbd_device *rbd_dev) kfree(rbd_dev); } -static bool rbd_snap_registered(struct rbd_snap *snap) -{ - bool ret = snap->dev.type == &rbd_snap_device_type; - bool reg = device_is_registered(&snap->dev); - - rbd_assert(!ret ^ reg); - - return ret; -} - static void rbd_remove_snap_dev(struct rbd_snap *snap) { list_del(&snap->node); - if (device_is_registered(&snap->dev)) - device_unregister(&snap->dev); -} - -static int rbd_register_snap_dev(struct rbd_snap *snap, - struct device *parent) -{ - struct device *dev = &snap->dev; - int ret; - - dev->type = &rbd_snap_device_type; - dev->parent = parent; - dev->release = rbd_snap_dev_release; - dev_set_name(dev, "%s%s", RBD_SNAP_DEV_NAME_PREFIX, snap->name); - dout("%s: registering device for snapshot %s\n", __func__, snap->name); - - ret = device_register(dev); - - return ret; + kfree(snap->name); + kfree(snap); } static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev, @@ -4089,8 +3993,6 @@ static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver) dout("rbd_dev_snaps_update returned %d\n", ret); if (ret) goto out; - ret = rbd_dev_snaps_register(rbd_dev); - dout("rbd_dev_snaps_register returned %d\n", ret); out: up_write(&rbd_dev->header_rwsem); @@ -4145,11 +4047,11 @@ static int rbd_dev_snaps_update(struct rbd_device *rbd_dev) */ if (rbd_dev->spec->snap_id == snap->id) clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags); - rbd_remove_snap_dev(snap); - dout("%ssnap id %llu has been removed\n", + dout("removing %ssnap id %llu\n", rbd_dev->spec->snap_id == snap->id ? "mapped " : "", (unsigned long long) snap->id); + rbd_remove_snap_dev(snap); /* Done with this list entry; advance */ @@ -4209,31 +4111,6 @@ static int rbd_dev_snaps_update(struct rbd_device *rbd_dev) return 0; } -/* - * Scan the list of snapshots and register the devices for any that - * have not already been registered. - */ -static int rbd_dev_snaps_register(struct rbd_device *rbd_dev) -{ - struct rbd_snap *snap; - int ret = 0; - - dout("%s:\n", __func__); - if (WARN_ON(!device_is_registered(&rbd_dev->dev))) - return -EIO; - - list_for_each_entry(snap, &rbd_dev->snaps, node) { - if (!rbd_snap_registered(snap)) { - ret = rbd_register_snap_dev(snap, &rbd_dev->dev); - if (ret < 0) - break; - } - } - dout("%s: returning %d\n", __func__, ret); - - return ret; -} - static int rbd_bus_add_dev(struct rbd_device *rbd_dev) { struct device *dev; @@ -4840,12 +4717,6 @@ static int rbd_dev_probe_finish(struct rbd_device *rbd_dev) rbd_dev->parent = parent; } - down_write(&rbd_dev->header_rwsem); - ret = rbd_dev_snaps_register(rbd_dev); - up_write(&rbd_dev->header_rwsem); - if (ret) - goto err_out_bus; - ret = rbd_dev_header_watch_sync(rbd_dev, 1); if (ret) goto err_out_bus;