| Message ID | 20240502130033.1958492-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] block: add a disk_has_partscan helper | expand |
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?
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.
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.
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.
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.
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
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(+)