add a partscan sysfs attribute

add a partscan sysfs attribute

[Christoph Hellwig]

Hi all,

this series adds a patscan sysfs attribute so that userspace 
(e.g. systemd) can check if partition scanning is enabled for a given
gendisk.

Diffstat:
 Documentation/ABI/stable/sysfs-block |   10 ++++++++++
 block/genhd.c                        |   15 ++++++++++-----
 block/partitions/core.c              |    5 +----
 include/linux/blkdev.h               |   13 +++++++++++++
 4 files changed, 34 insertions(+), 9 deletions(-)

[PATCH 1/2] block: add a disk_has_partscan helper

[Christoph Hellwig]

Add a helper to check if partition scanning is enabled instead of
open coding the check in a few places.  This now always checks for
the hidden flag even if all but one of the callers are never reachable
for hidden gendisks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c           |  7 ++-----
 block/partitions/core.c |  5 +----
 include/linux/blkdev.h  | 13 +++++++++++++
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index eb893df56d510e..4b85963d09dbb4 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -345,9 +345,7 @@ int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode)
 	struct file *file;
 	int ret = 0;
 
-	if (disk->flags & (GENHD_FL_NO_PART | GENHD_FL_HIDDEN))
-		return -EINVAL;
-	if (test_bit(GD_SUPPRESS_PART_SCAN, &disk->state))
+	if (!disk_has_partscan(disk))
 		return -EINVAL;
 	if (disk->open_partitions)
 		return -EBUSY;
@@ -503,8 +501,7 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 			goto out_unregister_bdi;
 
 		/* Make sure the first partition scan will be proceed */
-		if (get_capacity(disk) && !(disk->flags & GENHD_FL_NO_PART) &&
-		    !test_bit(GD_SUPPRESS_PART_SCAN, &disk->state))
+		if (get_capacity(disk) && disk_has_partscan(disk))
 			set_bit(GD_NEED_PART_SCAN, &disk->state);
 
 		bdev_add(disk->part0, ddev->devt);
diff --git a/block/partitions/core.c b/block/partitions/core.c
index b11e88c82c8cfa..37b5f92d07fec9 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -573,10 +573,7 @@ static int blk_add_partitions(struct gendisk *disk)
 	struct parsed_partitions *state;
 	int ret = -EAGAIN, p;
 
-	if (disk->flags & GENHD_FL_NO_PART)
-		return 0;
-
-	if (test_bit(GD_SUPPRESS_PART_SCAN, &disk->state))
+	if (!disk_has_partscan(disk))
 		return 0;
 
 	state = check_partition(disk);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 040a22e0eda0ec..3b18a40a1fc109 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -230,6 +230,19 @@ static inline unsigned int disk_openers(struct gendisk *disk)
 	return atomic_read(&disk->part0->bd_openers);
 }
 
+/**
+ * disk_has_partscan - return %true if partition scanning is enabled on a disk
+ * @disk: disk to check
+ *
+ * Returns %true if partitions scanning is enabled for @disk, or %false if
+ * partition scanning is disabled either permanently or temporarily.
+ */
+static inline bool disk_has_partscan(struct gendisk *disk)
+{
+	return !(disk->flags & (GENHD_FL_NO_PART | GENHD_FL_HIDDEN)) &&
+		!test_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
+}
+
 /*
  * The gendisk is refcounted by the part0 block_device, and the bd_device
  * therein is also used for device model presentation in sysfs.
-- 
2.39.2

[PATCH 2/2] block: add a partscan sysfs attribute for disks

[Christoph Hellwig]

This attribute reports if partition scanning is enabled for a given disk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/ABI/stable/sysfs-block | 10 ++++++++++
 block/genhd.c                        |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
index 1fe9a553c37b71..db58fdcdff31b0 100644
--- a/Documentation/ABI/stable/sysfs-block
+++ b/Documentation/ABI/stable/sysfs-block
@@ -101,6 +101,16 @@ Description:
 		devices that support receiving integrity metadata.
 
 
+What:		/sys/block/<disk>/partscan
+Date:		Atorl 2024
+Contact:	Christoph Hellwig <hch@lst.de>
+Description:
+		The /sys/block/<disk>/partscan files reports if partition
+		scanning is enabled for the disk.  It returns "1" if partition
+		scanning is enabled, or "0" if not.  The value type is a 32-bit
+		unsigned integer, but only "0" and "1" are valid values.
+
+
 What:		/sys/block/<disk>/<partition>/alignment_offset
 Date:		April 2009
 Contact:	Martin K. Petersen <martin.petersen@oracle.com>
diff --git a/block/genhd.c b/block/genhd.c
index 4b85963d09dbb4..dec2ee338fb44a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1044,6 +1044,12 @@ static ssize_t diskseq_show(struct device *dev,
 	return sprintf(buf, "%llu\n", disk->diskseq);
 }
 
+static ssize_t partscan_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%u\n", disk_has_partscan(dev_to_disk(dev)));
+}
+
 static DEVICE_ATTR(range, 0444, disk_range_show, NULL);
 static DEVICE_ATTR(ext_range, 0444, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, 0444, disk_removable_show, NULL);
@@ -1057,6 +1063,7 @@ static DEVICE_ATTR(stat, 0444, part_stat_show, NULL);
 static DEVICE_ATTR(inflight, 0444, part_inflight_show, NULL);
 static DEVICE_ATTR(badblocks, 0644, disk_badblocks_show, disk_badblocks_store);
 static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
+static DEVICE_ATTR(partscan, 0444, partscan_show, NULL);
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 ssize_t part_fail_show(struct device *dev,
@@ -1103,6 +1110,7 @@ static struct attribute *disk_attrs[] = {
 	&dev_attr_events_async.attr,
 	&dev_attr_events_poll_msecs.attr,
 	&dev_attr_diskseq.attr,
+	&dev_attr_partscan.attr,
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	&dev_attr_fail.attr,
 #endif
-- 
2.39.2

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[John Garry]

On 29/04/2024 18:49, Christoph Hellwig wrote:
> +Date:		Atorl 2024

I suppose that this needs to be fixed.. or become May.

Re: add a partscan sysfs attribute

[Lennart Poettering]

On Mo, 29.04.24 19:48, Christoph Hellwig (hch@lst.de) wrote:

> Hi all,
>
> this series adds a patscan sysfs attribute so that userspace
> (e.g. systemd) can check if partition scanning is enabled for a given
> gendisk.

thanks!

lgtm for our uses!

Lennart

--
Lennart Poettering, Berlin

add a partscan sysfs attribute v2

[Christoph Hellwig]

Hi all,

this series adds a patscan sysfs attribute so that userspace 
(e.g. systemd) can check if partition scanning is enabled for a given
gendisk.

Changes since v1:
 - update the data from the non-existing month of Atorl to May

Diffstat:
 Documentation/ABI/stable/sysfs-block |   10 ++++++++++
 block/genhd.c                        |   15 ++++++++++-----
 block/partitions/core.c              |    5 +----
 include/linux/blkdev.h               |   13 +++++++++++++
 4 files changed, 34 insertions(+), 9 deletions(-)

[PATCH 2/2] block: add a partscan sysfs attribute for disks

[Christoph Hellwig]

This attribute reports if partition scanning is enabled for a given disk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/ABI/stable/sysfs-block | 10 ++++++++++
 block/genhd.c                        |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
index 1fe9a553c37b71..f0025d1c3d5acd 100644
--- a/Documentation/ABI/stable/sysfs-block
+++ b/Documentation/ABI/stable/sysfs-block
@@ -101,6 +101,16 @@ Description:
 		devices that support receiving integrity metadata.
 
 
+What:		/sys/block/<disk>/partscan
+Date:		May 2024
+Contact:	Christoph Hellwig <hch@lst.de>
+Description:
+		The /sys/block/<disk>/partscan files reports if partition
+		scanning is enabled for the disk.  It returns "1" if partition
+		scanning is enabled, or "0" if not.  The value type is a 32-bit
+		unsigned integer, but only "0" and "1" are valid values.
+
+
 What:		/sys/block/<disk>/<partition>/alignment_offset
 Date:		April 2009
 Contact:	Martin K. Petersen <martin.petersen@oracle.com>
diff --git a/block/genhd.c b/block/genhd.c
index 4b85963d09dbb4..dec2ee338fb44a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1044,6 +1044,12 @@ static ssize_t diskseq_show(struct device *dev,
 	return sprintf(buf, "%llu\n", disk->diskseq);
 }
 
+static ssize_t partscan_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%u\n", disk_has_partscan(dev_to_disk(dev)));
+}
+
 static DEVICE_ATTR(range, 0444, disk_range_show, NULL);
 static DEVICE_ATTR(ext_range, 0444, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, 0444, disk_removable_show, NULL);
@@ -1057,6 +1063,7 @@ static DEVICE_ATTR(stat, 0444, part_stat_show, NULL);
 static DEVICE_ATTR(inflight, 0444, part_inflight_show, NULL);
 static DEVICE_ATTR(badblocks, 0644, disk_badblocks_show, disk_badblocks_store);
 static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
+static DEVICE_ATTR(partscan, 0444, partscan_show, NULL);
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 ssize_t part_fail_show(struct device *dev,
@@ -1103,6 +1110,7 @@ static struct attribute *disk_attrs[] = {
 	&dev_attr_events_async.attr,
 	&dev_attr_events_poll_msecs.attr,
 	&dev_attr_diskseq.attr,
+	&dev_attr_partscan.attr,
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	&dev_attr_fail.attr,
 #endif
-- 
2.39.2

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[Jens Axboe]

On 5/2/24 7:00 AM, Christoph Hellwig wrote:
> This attribute reports if partition scanning is enabled for a given disk.

This should, at least, have a reference to Lennart's posting, and
honestly a much better commit message as well. There's no reasoning
given here at all.

Maybe even a fixes tag and stable notation?

-- 
Jens Axboe

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[Christoph Hellwig]

On Thu, May 02, 2024 at 11:05:54AM -0600, Jens Axboe wrote:
> On 5/2/24 7:00 AM, Christoph Hellwig wrote:
> > This attribute reports if partition scanning is enabled for a given disk.
> 
> This should, at least, have a reference to Lennart's posting, and
> honestly a much better commit message as well. There's no reasoning
> given here at all.

I'm not sure I can come up with something much better, feel free to
throw in what you prefer.

> Maybe even a fixes tag and stable notation?

This is definitively not a Fixes as nothing it doesn't actually fix
any code.  It provides a proper interfaces for what was an abuse
of leaking internal bits out.

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[Keith Busch]

On Fri, May 03, 2024 at 10:16:12AM +0200, Christoph Hellwig wrote:
> On Thu, May 02, 2024 at 11:05:54AM -0600, Jens Axboe wrote:
> > On 5/2/24 7:00 AM, Christoph Hellwig wrote:
> > > This attribute reports if partition scanning is enabled for a given disk.
> > 
> > This should, at least, have a reference to Lennart's posting, and
> > honestly a much better commit message as well. There's no reasoning
> > given here at all.
> 
> I'm not sure I can come up with something much better, feel free to
> throw in what you prefer.

I think just explaining the "why" would be usesful for the git history.
How about this:

  Userspace had been unknowingly relying on a non-stable interface of
  kernel internals to determine if partition scanning is enabled for a
  given disk. Provide a stable interface for this purpose instead.

  Link: https://lore.kernel.org/linux-block/ZhQJf8mzq_wipkBH@gardel-login/
 
> > Maybe even a fixes tag and stable notation?
> 
> This is definitively not a Fixes as nothing it doesn't actually fix
> any code.  It provides a proper interfaces for what was an abuse
> of leaking internal bits out.

I kind of agree it's not a "Fixes:" in the traditional sense, but at a
"Cc: <stable>" sounds appropriate given the fallout.

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[Jens Axboe]

On 5/3/24 3:12 AM, Keith Busch wrote:
> On Fri, May 03, 2024 at 10:16:12AM +0200, Christoph Hellwig wrote:
>> On Thu, May 02, 2024 at 11:05:54AM -0600, Jens Axboe wrote:
>>> On 5/2/24 7:00 AM, Christoph Hellwig wrote:
>>>> This attribute reports if partition scanning is enabled for a given disk.
>>>
>>> This should, at least, have a reference to Lennart's posting, and
>>> honestly a much better commit message as well. There's no reasoning
>>> given here at all.
>>
>> I'm not sure I can come up with something much better, feel free to
>> throw in what you prefer.
> 
> I think just explaining the "why" would be usesful for the git history.
> How about this:
> 
>   Userspace had been unknowingly relying on a non-stable interface of
>   kernel internals to determine if partition scanning is enabled for a
>   given disk. Provide a stable interface for this purpose instead.
> 
>   Link: https://lore.kernel.org/linux-block/ZhQJf8mzq_wipkBH@gardel-login/

Yep this looks good, I can grab that.

>>> Maybe even a fixes tag and stable notation?
>>
>> This is definitively not a Fixes as nothing it doesn't actually fix
>> any code.  It provides a proper interfaces for what was an abuse
>> of leaking internal bits out.
> 
> I kind of agree it's not a "Fixes:" in the traditional sense, but at a
> "Cc: <stable>" sounds appropriate given the fallout.

It's definitely not a traditional kind of fixes, even a made-up tag
might be fine for this.

But probably just a Cc: stable@vger.kernel.org # version

and the added link would be good enough.

-- 
Jens Axboe

Re: [PATCH 2/2] block: add a partscan sysfs attribute for disks

[Jens Axboe]

On 5/3/24 2:16 AM, Christoph Hellwig wrote:
> On Thu, May 02, 2024 at 11:05:54AM -0600, Jens Axboe wrote:
>> On 5/2/24 7:00 AM, Christoph Hellwig wrote:
>>> This attribute reports if partition scanning is enabled for a given disk.
>>
>> This should, at least, have a reference to Lennart's posting, and
>> honestly a much better commit message as well. There's no reasoning
>> given here at all.
> 
> I'm not sure I can come up with something much better, feel free to
> throw in what you prefer.

Really? You have literally nothing in there! It's good practice to
include reasoning for why the change is being done, particularly in this
case where there's quite strong reasonings for the addition.

>> Maybe even a fixes tag and stable notation?
> 
> This is definitively not a Fixes as nothing it doesn't actually fix
> any code.  It provides a proper interfaces for what was an abuse
> of leaking internal bits out.

I'm looking to bridge the gap between when we yanked the old garbage
interface and now added this one. So yeah it's not a pure fixes, but I
think we should still tie them together somehow so we can do a proper
stable backport of this.

-- 
Jens Axboe