The embedded member integrity_kobj member of struct gendisk violates
the assumption of the driver core that only one struct kobject should be
embedded into another object and then manages its lifetime.
As the integrity_kobj is only used to hold a few sysfs attributes it can
be replaced by direct device_attributes and removed.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Changes in v3:
- Style cleanups
- Register attributes directly via disk_attr_groups
- Link to v2: https://lore.kernel.org/r/20230309-kobj_release-gendisk_integrity-v2-0-761a50d71900@weissschuh.net
Changes in v2:
- Get rid of integrity_kobj completely
- Migrate to sysfs_emit helper
- Link to v1: https://lore.kernel.org/r/20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net
---
Thomas Weißschuh (3):
blk-integrity: use sysfs_emit
blk-integrity: convert to struct device_attribute
blk-integrity: register sysfs attributes on struct device
block/blk-integrity.c | 175 +++++++++++++++++--------------------------------
block/blk.h | 10 +--
block/genhd.c | 12 ++--
include/linux/blkdev.h | 3 -
4 files changed, 66 insertions(+), 134 deletions(-)
---
base-commit: 478a351ce0d69cef2d2bf2a686a09b356b63a66c
change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa
Best regards,
--
Thomas Weißschuh <[email protected]>
The correct way to emit data into sysfs is via sysfs_emit(), use it.
Also perform some trivial syntactic cleanups.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 8f01d786f5cb..aca8c783d749 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -248,20 +248,19 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
static ssize_t integrity_format_show(struct blk_integrity *bi, char *page)
{
if (bi->profile && bi->profile->name)
- return sprintf(page, "%s\n", bi->profile->name);
- else
- return sprintf(page, "none\n");
+ return sysfs_emit(page, "%s\n", bi->profile->name);
+ return sysfs_emit(page, "none\n");
}
static ssize_t integrity_tag_size_show(struct blk_integrity *bi, char *page)
{
- return sprintf(page, "%u\n", bi->tag_size);
+ return sysfs_emit(page, "%u\n", bi->tag_size);
}
static ssize_t integrity_interval_show(struct blk_integrity *bi, char *page)
{
- return sprintf(page, "%u\n",
- bi->interval_exp ? 1 << bi->interval_exp : 0);
+ return sysfs_emit(page, "%u\n",
+ bi->interval_exp ? 1 << bi->interval_exp : 0);
}
static ssize_t integrity_verify_store(struct blk_integrity *bi,
@@ -280,7 +279,7 @@ static ssize_t integrity_verify_store(struct blk_integrity *bi,
static ssize_t integrity_verify_show(struct blk_integrity *bi, char *page)
{
- return sprintf(page, "%d\n", (bi->flags & BLK_INTEGRITY_VERIFY) != 0);
+ return sysfs_emit(page, "%d\n", !!(bi->flags & BLK_INTEGRITY_VERIFY));
}
static ssize_t integrity_generate_store(struct blk_integrity *bi,
@@ -299,13 +298,13 @@ static ssize_t integrity_generate_store(struct blk_integrity *bi,
static ssize_t integrity_generate_show(struct blk_integrity *bi, char *page)
{
- return sprintf(page, "%d\n", (bi->flags & BLK_INTEGRITY_GENERATE) != 0);
+ return sysfs_emit(page, "%d\n", !!(bi->flags & BLK_INTEGRITY_GENERATE));
}
static ssize_t integrity_device_show(struct blk_integrity *bi, char *page)
{
- return sprintf(page, "%u\n",
- (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE) != 0);
+ return sysfs_emit(page, "%u\n",
+ !!(bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE));
}
static struct integrity_sysfs_entry integrity_format_entry = {
--
2.40.0
An upcoming patch will register the integrity attributes directly with
the struct device kobject.
For this the attributes have to be implemented in terms of
struct device_attribute.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 127 ++++++++++++++++++++++++--------------------------
1 file changed, 62 insertions(+), 65 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index aca8c783d749..1cbfdea88c72 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -212,21 +212,15 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
return true;
}
-struct integrity_sysfs_entry {
- struct attribute attr;
- ssize_t (*show)(struct blk_integrity *, char *);
- ssize_t (*store)(struct blk_integrity *, const char *, size_t);
-};
-
static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
char *page)
{
struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct blk_integrity *bi = &disk->queue->integrity;
- struct integrity_sysfs_entry *entry =
- container_of(attr, struct integrity_sysfs_entry, attr);
+ struct device *dev = disk_to_dev(disk);
+ struct device_attribute *dev_attr =
+ container_of(attr, struct device_attribute, attr);
- return entry->show(bi, page);
+ return dev_attr->show(dev, dev_attr, page);
}
static ssize_t integrity_attr_store(struct kobject *kobj,
@@ -234,38 +228,53 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
size_t count)
{
struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct blk_integrity *bi = &disk->queue->integrity;
- struct integrity_sysfs_entry *entry =
- container_of(attr, struct integrity_sysfs_entry, attr);
- ssize_t ret = 0;
+ struct device *dev = disk_to_dev(disk);
+ struct device_attribute *dev_attr =
+ container_of(attr, struct device_attribute, attr);
- if (entry->store)
- ret = entry->store(bi, page, count);
+ if (!dev_attr->store)
+ return 0;
+ return dev_attr->store(dev, dev_attr, page, count);
+}
- return ret;
+static inline struct blk_integrity *dev_to_bi(struct device *dev)
+{
+ return &dev_to_disk(dev)->queue->integrity;
}
-static ssize_t integrity_format_show(struct blk_integrity *bi, char *page)
+static ssize_t format_show(struct device *dev, struct device_attribute *attr,
+ char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
if (bi->profile && bi->profile->name)
return sysfs_emit(page, "%s\n", bi->profile->name);
return sysfs_emit(page, "none\n");
}
-static ssize_t integrity_tag_size_show(struct blk_integrity *bi, char *page)
+static ssize_t tag_size_show(struct device *dev, struct device_attribute *attr,
+ char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n", bi->tag_size);
}
-static ssize_t integrity_interval_show(struct blk_integrity *bi, char *page)
+static ssize_t protection_interval_bytes_show(struct device *dev,
+ struct device_attribute *attr,
+ char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n",
bi->interval_exp ? 1 << bi->interval_exp : 0);
}
-static ssize_t integrity_verify_store(struct blk_integrity *bi,
- const char *page, size_t count)
+static ssize_t read_verify_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *page, size_t count)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
char *p = (char *) page;
unsigned long val = simple_strtoul(p, &p, 10);
@@ -277,14 +286,20 @@ static ssize_t integrity_verify_store(struct blk_integrity *bi,
return count;
}
-static ssize_t integrity_verify_show(struct blk_integrity *bi, char *page)
+static ssize_t read_verify_show(struct device *dev,
+ struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%d\n", !!(bi->flags & BLK_INTEGRITY_VERIFY));
}
-static ssize_t integrity_generate_store(struct blk_integrity *bi,
- const char *page, size_t count)
+static ssize_t write_generate_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *page, size_t count)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
char *p = (char *) page;
unsigned long val = simple_strtoul(p, &p, 10);
@@ -296,57 +311,39 @@ static ssize_t integrity_generate_store(struct blk_integrity *bi,
return count;
}
-static ssize_t integrity_generate_show(struct blk_integrity *bi, char *page)
+static ssize_t write_generate_show(struct device *dev,
+ struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%d\n", !!(bi->flags & BLK_INTEGRITY_GENERATE));
}
-static ssize_t integrity_device_show(struct blk_integrity *bi, char *page)
+static ssize_t device_is_integrity_capable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n",
!!(bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE));
}
-static struct integrity_sysfs_entry integrity_format_entry = {
- .attr = { .name = "format", .mode = 0444 },
- .show = integrity_format_show,
-};
-
-static struct integrity_sysfs_entry integrity_tag_size_entry = {
- .attr = { .name = "tag_size", .mode = 0444 },
- .show = integrity_tag_size_show,
-};
-
-static struct integrity_sysfs_entry integrity_interval_entry = {
- .attr = { .name = "protection_interval_bytes", .mode = 0444 },
- .show = integrity_interval_show,
-};
-
-static struct integrity_sysfs_entry integrity_verify_entry = {
- .attr = { .name = "read_verify", .mode = 0644 },
- .show = integrity_verify_show,
- .store = integrity_verify_store,
-};
-
-static struct integrity_sysfs_entry integrity_generate_entry = {
- .attr = { .name = "write_generate", .mode = 0644 },
- .show = integrity_generate_show,
- .store = integrity_generate_store,
-};
-
-static struct integrity_sysfs_entry integrity_device_entry = {
- .attr = { .name = "device_is_integrity_capable", .mode = 0444 },
- .show = integrity_device_show,
-};
+static DEVICE_ATTR_RO(format);
+static DEVICE_ATTR_RO(tag_size);
+static DEVICE_ATTR_RO(protection_interval_bytes);
+static DEVICE_ATTR_RW(read_verify);
+static DEVICE_ATTR_RW(write_generate);
+static DEVICE_ATTR_RO(device_is_integrity_capable);
static struct attribute *integrity_attrs[] = {
- &integrity_format_entry.attr,
- &integrity_tag_size_entry.attr,
- &integrity_interval_entry.attr,
- &integrity_verify_entry.attr,
- &integrity_generate_entry.attr,
- &integrity_device_entry.attr,
- NULL,
+ &dev_attr_format.attr,
+ &dev_attr_tag_size.attr,
+ &dev_attr_protection_interval_bytes.attr,
+ &dev_attr_read_verify.attr,
+ &dev_attr_write_generate.attr,
+ &dev_attr_device_is_integrity_capable.attr,
+ NULL
};
ATTRIBUTE_GROUPS(integrity);
--
2.40.0
The "integrity" kobject only acted as a holder for static sysfs entries.
It also was embedded into struct gendisk without managing it, violating
assumptions of the driver core.
Instead register the sysfs entries directly onto the struct device.
Also drop the now unused member integrity_kobj from struct gendisk.
Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 55 +++-----------------------------------------------
block/blk.h | 10 +--------
block/genhd.c | 12 ++++-------
include/linux/blkdev.h | 3 ---
4 files changed, 8 insertions(+), 72 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 1cbfdea88c72..d4e9b4556d14 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -212,31 +212,6 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
return true;
}
-static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
- char *page)
-{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct device *dev = disk_to_dev(disk);
- struct device_attribute *dev_attr =
- container_of(attr, struct device_attribute, attr);
-
- return dev_attr->show(dev, dev_attr, page);
-}
-
-static ssize_t integrity_attr_store(struct kobject *kobj,
- struct attribute *attr, const char *page,
- size_t count)
-{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct device *dev = disk_to_dev(disk);
- struct device_attribute *dev_attr =
- container_of(attr, struct device_attribute, attr);
-
- if (!dev_attr->store)
- return 0;
- return dev_attr->store(dev, dev_attr, page, count);
-}
-
static inline struct blk_integrity *dev_to_bi(struct device *dev)
{
return &dev_to_disk(dev)->queue->integrity;
@@ -345,16 +320,10 @@ static struct attribute *integrity_attrs[] = {
&dev_attr_device_is_integrity_capable.attr,
NULL
};
-ATTRIBUTE_GROUPS(integrity);
-static const struct sysfs_ops integrity_ops = {
- .show = &integrity_attr_show,
- .store = &integrity_attr_store,
-};
-
-static const struct kobj_type integrity_ktype = {
- .default_groups = integrity_groups,
- .sysfs_ops = &integrity_ops,
+const struct attribute_group blk_integrity_attr_group = {
+ .name = "integrity",
+ .attrs = integrity_attrs,
};
static blk_status_t blk_integrity_nop_fn(struct blk_integrity_iter *iter)
@@ -433,21 +402,3 @@ void blk_integrity_unregister(struct gendisk *disk)
memset(bi, 0, sizeof(*bi));
}
EXPORT_SYMBOL(blk_integrity_unregister);
-
-int blk_integrity_add(struct gendisk *disk)
-{
- int ret;
-
- ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
- &disk_to_dev(disk)->kobj, "%s", "integrity");
- if (!ret)
- kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
- return ret;
-}
-
-void blk_integrity_del(struct gendisk *disk)
-{
- kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
- kobject_del(&disk->integrity_kobj);
- kobject_put(&disk->integrity_kobj);
-}
diff --git a/block/blk.h b/block/blk.h
index cc4e8873dfde..4859290b04a2 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -214,8 +214,7 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
bip_next->bip_vec[0].bv_offset);
}
-int blk_integrity_add(struct gendisk *disk);
-void blk_integrity_del(struct gendisk *);
+extern const struct attribute_group blk_integrity_attr_group;
#else /* CONFIG_BLK_DEV_INTEGRITY */
static inline bool blk_integrity_merge_rq(struct request_queue *rq,
struct request *r1, struct request *r2)
@@ -248,13 +247,6 @@ static inline bool bio_integrity_endio(struct bio *bio)
static inline void bio_integrity_free(struct bio *bio)
{
}
-static inline int blk_integrity_add(struct gendisk *disk)
-{
- return 0;
-}
-static inline void blk_integrity_del(struct gendisk *disk)
-{
-}
#endif /* CONFIG_BLK_DEV_INTEGRITY */
unsigned long blk_rq_timeout(unsigned long timeout);
diff --git a/block/genhd.c b/block/genhd.c
index 02d9cfb9e077..7348c4408453 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -480,15 +480,11 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
*/
pm_runtime_set_memalloc_noio(ddev, true);
- ret = blk_integrity_add(disk);
- if (ret)
- goto out_del_block_link;
-
disk->part0->bd_holder_dir =
kobject_create_and_add("holders", &ddev->kobj);
if (!disk->part0->bd_holder_dir) {
ret = -ENOMEM;
- goto out_del_integrity;
+ goto out_del_block_link;
}
disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
if (!disk->slave_dir) {
@@ -551,8 +547,6 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
disk->slave_dir = NULL;
out_put_holder_dir:
kobject_put(disk->part0->bd_holder_dir);
-out_del_integrity:
- blk_integrity_del(disk);
out_del_block_link:
if (!sysfs_deprecated)
sysfs_remove_link(block_depr, dev_name(ddev));
@@ -615,7 +609,6 @@ void del_gendisk(struct gendisk *disk)
if (WARN_ON_ONCE(!disk_live(disk) && !(disk->flags & GENHD_FL_HIDDEN)))
return;
- blk_integrity_del(disk);
disk_del_events(disk);
mutex_lock(&disk->open_mutex);
@@ -1152,6 +1145,9 @@ static const struct attribute_group *disk_attr_groups[] = {
&disk_attr_group,
#ifdef CONFIG_BLK_DEV_IO_TRACE
&blk_trace_attr_group,
+#endif
+#ifdef CONFIG_BLK_DEV_INTEGRITY
+ &blk_integrity_attr_group,
#endif
NULL
};
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 941304f17492..cc2b9e110728 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -163,9 +163,6 @@ struct gendisk {
struct timer_rand_state *random;
atomic_t sync_io; /* RAID */
struct disk_events *ev;
-#ifdef CONFIG_BLK_DEV_INTEGRITY
- struct kobject integrity_kobj;
-#endif /* CONFIG_BLK_DEV_INTEGRITY */
#ifdef CONFIG_BLK_DEV_ZONED
/*
--
2.40.0
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Thomas,
> The embedded member integrity_kobj member of struct gendisk violates
> the assumption of the driver core that only one struct kobject should
> be embedded into another object and then manages its lifetime.
>
> As the integrity_kobj is only used to hold a few sysfs attributes it
> can be replaced by direct device_attributes and removed.
Looks good to me and passed a quick test on a couple of systems. Thanks
for cleaning this up!
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
Hi Martin, Christoph, Jens,
On 2023-03-20 07:56:58-0400, Martin K. Petersen wrote:
> > The embedded member integrity_kobj member of struct gendisk violates
> > the assumption of the driver core that only one struct kobject should
> > be embedded into another object and then manages its lifetime.
> >
> > As the integrity_kobj is only used to hold a few sysfs attributes it
> > can be replaced by direct device_attributes and removed.
>
> Looks good to me and passed a quick test on a couple of systems. Thanks
> for cleaning this up!
>
> Reviewed-by: Martin K. Petersen <[email protected]>
Am I getting some part of the process for block/ wrong?
It seems my patches for the block subsystem are having a hard time
getting merged.
* https://lore.kernel.org/all/[email protected]/
* this series
* https://lore.kernel.org/all/[email protected]/
Thanks for any pointers,
Thomas
On 4/26/23 5:12?PM, Thomas Wei?schuh wrote:
> Hi Martin, Christoph, Jens,
>
> On 2023-03-20 07:56:58-0400, Martin K. Petersen wrote:
>>> The embedded member integrity_kobj member of struct gendisk violates
>>> the assumption of the driver core that only one struct kobject should
>>> be embedded into another object and then manages its lifetime.
>>>
>>> As the integrity_kobj is only used to hold a few sysfs attributes it
>>> can be replaced by direct device_attributes and removed.
>>
>> Looks good to me and passed a quick test on a couple of systems. Thanks
>> for cleaning this up!
>>
>> Reviewed-by: Martin K. Petersen <[email protected]>
>
> Am I getting some part of the process for block/ wrong?
Sorry, I missed this series. I'll queue it up for 6.4.
> It seems my patches for the block subsystem are having a hard time
> getting merged.
>
> * https://lore.kernel.org/all/[email protected]/
This one is missing nbd review. It's unfortunately not uncommon to need
to re-ping on something like this, if you don't get a timely review.
This is not specific to this patch, just in general. Things get missed.
> * this series
> * https://lore.kernel.org/all/[email protected]/
This one is just a week old, and coming into the merge window. Generally
takes longer at that time, as it's late for that merge window, and folks
are busy with getting things ready. If nothing happens on this one, I'd
suggest resending past -rc1 when folks are more ready to review and
queue things up for the next release.
--
Jens Axboe
On Sat, 18 Mar 2023 17:36:22 +0000, Thomas Weißschuh wrote:
> The embedded member integrity_kobj member of struct gendisk violates
> the assumption of the driver core that only one struct kobject should be
> embedded into another object and then manages its lifetime.
>
> As the integrity_kobj is only used to hold a few sysfs attributes it can
> be replaced by direct device_attributes and removed.
>
> [...]
Applied, thanks!
[1/3] blk-integrity: use sysfs_emit
commit: 3315e169b446249c1b61ff988d157238f4b2c5a0
[2/3] blk-integrity: convert to struct device_attribute
commit: 76b8c319f02715e14abdbbbdd6508e83a1059bcc
[3/3] blk-integrity: register sysfs attributes on struct device
commit: ff53cd52d9bdbf4074d2bbe9b591729997780bd3
Best regards,
--
Jens Axboe
On 2023-04-26 18:26:11-0600, Jens Axboe wrote:
> On 4/26/23 5:12?PM, Thomas Wei?schuh wrote:
> > Hi Martin, Christoph, Jens,
> >
> > On 2023-03-20 07:56:58-0400, Martin K. Petersen wrote:
> >>> The embedded member integrity_kobj member of struct gendisk violates
> >>> the assumption of the driver core that only one struct kobject should
> >>> be embedded into another object and then manages its lifetime.
> >>>
> >>> As the integrity_kobj is only used to hold a few sysfs attributes it
> >>> can be replaced by direct device_attributes and removed.
> >>
> >> Looks good to me and passed a quick test on a couple of systems. Thanks
> >> for cleaning this up!
> >>
> >> Reviewed-by: Martin K. Petersen <[email protected]>
> >
> > Am I getting some part of the process for block/ wrong?
>
> Sorry, I missed this series. I'll queue it up for 6.4.
Thanks!
> > It seems my patches for the block subsystem are having a hard time
> > getting merged.
> >
> > * https://lore.kernel.org/all/[email protected]/
>
> This one is missing nbd review. It's unfortunately not uncommon to need
> to re-ping on something like this, if you don't get a timely review.
> This is not specific to this patch, just in general. Things get missed.
Will do.
> > * this series
> > * https://lore.kernel.org/all/[email protected]/
>
> This one is just a week old, and coming into the merge window. Generally
> takes longer at that time, as it's late for that merge window, and folks
> are busy with getting things ready. If nothing happens on this one, I'd
> suggest resending past -rc1 when folks are more ready to review and
> queue things up for the next release.
Indeed, it is only listed for completeness sake.
I assumed that because all three series were like this, that I maybe
missed some PATCH prefix for your filter, managed to end up in your
killfile or mails from my privately managed mailservers don't get
through.
This is why I also wrote to Martin and Christoph, fearing that you don't
see my mails.
Thanks for the clarification,
Thomas