Skip to content

Conversation

@ArneTR
Copy link
Member

@ArneTR ArneTR commented Jan 8, 2026

Greptile Overview

Greptile Summary

Enhanced disk device detection by adding support for NVMe devices (major 259) and additional device mapper ranges (251-254), plus added lspci and lsblk commands to hardware info collection for better debugging.

  • Added documentation for NVMe (259) and network block devices (43) major numbers
  • Expanded device mapper exclusion from just 254 to range 251-254
  • Updated comments to clarify that major 8 is SCSI/SATA, not NVMe
  • Added comprehensive guideline comments explaining disk detection philosophy
  • Added lspci and lsblk to hardware info for better system visibility

Issues found:

  • Multiple spelling errors in comments ('devive', 'Partion', 'phyiscal')
  • NVMe partition detection logic (minor_number % 16) won't work correctly for major 259 devices, which use a different partition numbering scheme

@ArneTR
Copy link
Member Author

ArneTR commented Jan 8, 2026

@greptileai

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Required keyword not found in PR title or description.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile

// => If this code runs into trouble in the future we might need to migrate to a better detection mechanism
// However lsblk -o NAME,MAJ:MIN,TYPE is not too helpful, as the type is not useful to use
// (resolves to sysfs virtual / physical classification)
// but especially the dynamic devive block mapper might contain unknown disk we want to track or exclude
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'devive' is misspelled

Suggested change
// but especially the dynamic devive block mapper might contain unknown disk we want to track or exclude
// but especially the dynamic device block mapper might contain unknown disk we want to track or exclude

// => If this code runs into trouble in the future we might need to migrate to a better detection mechanism
// However lsblk -o NAME,MAJ:MIN,TYPE is not too helpful, as the type is not useful to use
// (resolves to sysfs virtual / physical classification)
// but especially the dynamic devive block mapper might contain unknown disk we want to track or exclude
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'devive' is misspelled

Suggested change
// but especially the dynamic devive block mapper might contain unknown disk we want to track or exclude
// but especially the dynamic device block mapper might contain unknown disk we want to track or exclude

}

if (minor_number % 16 != 0) {
fprintf(stderr, "Partion inside a docker container found. This should not happen: %u:%u rbytes=%llu wbytes=%llu\n", major_number, minor_number, rbytes, wbytes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Partion' is misspelled

Suggested change
fprintf(stderr, "Partion inside a docker container found. This should not happen: %u:%u rbytes=%llu wbytes=%llu\n", major_number, minor_number, rbytes, wbytes);
fprintf(stderr, "Partition inside a docker container found. This should not happen: %u:%u rbytes=%llu wbytes=%llu\n", major_number, minor_number, rbytes, wbytes);


///////////////////// Guideline ///////////////////////////
// This code should only detect non-partitions and only the main disk, as this is where data is effectively stored
// This includes network storage as well (as in the end a phyiscal disk is somewhere) - Thus the physical / virtual
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'phyiscal' is misspelled

Suggested change
// This includes network storage as well (as in the end a phyiscal disk is somewhere) - Thus the physical / virtual
// This includes network storage as well (as in the end a physical disk is somewhere) - Thus the physical / virtual


///////////////////// Guideline ///////////////////////////
// This code should only detect non-partitions and only the main disk, as this is where data is effectively stored
// This includes network storage as well (as in the end a phyiscal disk is somewhere) - Thus the physical / virtual
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'phyiscal' is misspelled

Suggested change
// This includes network storage as well (as in the end a phyiscal disk is somewhere) - Thus the physical / virtual
// This includes network storage as well (as in the end a physical disk is somewhere) - Thus the physical / virtual

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Additional Comments (2)

metric_providers/disk/io/cgroup/container/source.c
NVMe partition detection won't work with minor_number % 16. NVMe devices (major 259) use minor 0 for /dev/nvme0n1, minor 1-128 for partitions, minor 256 for /dev/nvme1n1, etc. Consider checking if major_number == 259 and using different logic like checking device name suffix or skipping partitions by checking if minor % 256 gives a value > 0 and < some threshold.


metric_providers/disk/io/procfs/system/source.c
NVMe partition detection won't work with minor_number % 16. NVMe devices (major 259) use minor 0 for /dev/nvme0n1, minor 1-128 for partitions, minor 256 for /dev/nvme1n1, etc. Consider adding NVMe-specific logic or using device name pattern matching.

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

Eco CI Output [RUN-ID: 20805776361]:

🌳 CO2 Data:
City: CONSTANT, Lat: , Lon:
IP:
CO₂ from energy is: 1.611978060 g
CO₂ from manufacturing (embodied carbon) is: 0.467649303 g
Carbon Intensity for this location: 231 gCO₂eq/kWh
SCI: 2.079627 gCO₂eq / pipeline run emitted


Total cost of whole PR so far:

Label🖥 avg. CPU utilization [%]🔋 Total Energy [Joules]🔌 avg. Power [Watts]Duration [Seconds]
Measurement #130.00786978.264.261639.07
Total Run30.016978.264.261639.07
Additional overhead from Eco CIN/A17.174.104.19

@flyaruu
Copy link

flyaruu commented Jan 8, 2026

tested on my machine, worked fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants