gavinchou commented on PR #67536: URL: https://github.com/apache/doris/pull/67536#issuecomment-6117936002
### Device discovery should not depend on name suffixes The current fix is too specific to `dm-N`. A trailing number is not a generic partition syntax: - `sda2` is a partition, but `nvme0n1`, `md127`, `mmcblk0`, `loop0`, and `dm-0` are complete devices. - `nvme0n1p1` should resolve to `nvme0n1`, while the new unit test currently expects `nvme0n1p`, which codifies an incorrect result. - `dm-*` is a general device-mapper name, not necessarily an LVM volume. Making the exception list configurable would only move this naming heuristic from code to configuration. New device types would still require new exceptions. The usual approach is to treat the kernel device number as identity and the name only as a label: - `stat(storage_path).st_dev`, or the `major:minor` field from `/proc/self/mountinfo`, identifies the filesystem device. - `/sys/dev/block/<major>:<minor>` resolves that identity to the exact sysfs block device. - sysfs tells us whether it is a partition, its parent device, and any device-mapper backing devices. - `/proc/diskstats` can then be joined by `major:minor`, without parsing the device name. This is also closer to ClickHouse: it enumerates devices from `/sys/block`, reads each device's `/sys/block/<device>/stat`, and periodically refreshes the device list instead of classifying devices by suffix ([source](https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/AsynchronousMetrics.cpp)). cAdvisor and node_exporter similarly use major/minor plus sysfs for filesystem/device association and device-mapper metadata. For Doris, a practical design would be: 1. Enumerate top-level devices from `/sys/block`, following the ClickHouse model. 2. For every configured storage path, use `stat(path).st_dev` rather than `stat(dev_path).st_rdev`; the mount source may be an alias or may not exist inside a container. 3. Resolve `/sys/dev/block/<major>:<minor>`. If it is a partition, find its parent through sysfs; otherwise keep it as-is. This naturally preserves `dm-0`, `md127`, and `nvme0n1`. 4. Use `major:minor` as the internal key. Keep the kernel name, DM name, or DM UUID only as metric labels. 5. Refresh the mapping periodically or when storage paths change. Configuration is still useful for policy, for example `logical | physical | both`, include/exclude filters, or choosing `dm_name` versus the kernel name as the label. It should not define prefix-based partition rules. Please replace the `dm-` special case with sysfs/major-minor-based discovery and add tests for at least `sda1`, `nvme0n1p1`, `mmcblk0p1`, `md127`, and `dm-0`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
