Skip to content

Conversation

@lingkai0110
Copy link
Contributor

This patch is a follow up of (#2559).

It adds the stats support for EC2 local storage on top of EBS.

For local stroage, it supports a --details / -d option. In this mode it will print out detail io histograms for different block size ranges.

@igaw
Copy link
Collaborator

igaw commented Aug 22, 2025

Could you tweak the commit message slightly. Use a prefix for the commit title, e.g.

plugins/amzn: ...

Also the link to github in the first sentence should be removed. The simple reason is that we don't want to tight in this platform, so every commit message should be complete in itself. Pointer to external sources is sometimes okay, but usually those links have a limited life time. I think you can just remove the first sentence.

@igaw
Copy link
Collaborator

igaw commented Aug 22, 2025

 /usr/bin/arm-linux-gnueabihf-gcc -Invme.p -I. -I.. -Iccan -I../ccan -Isubprojects/libnvme/src -I../subprojects/libnvme/src -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=gnu99 -O3 -fomit-frame-pointer -D_GNU_SOURCE -include config.h -MD -MQ nvme.p/plugins_amzn_amzn-nvme.c.o -MF nvme.p/plugins_amzn_amzn-nvme.c.o.d -o nvme.p/plugins_amzn_amzn-nvme.c.o -c ../plugins/amzn/amzn-nvme.c
../plugins/amzn/amzn-nvme.c: In function ‘get_stats’:
../plugins/amzn/amzn-nvme.c:538:51: error: macro "amzn_print_json_stats" passed 2 arguments, but takes just 1
  538 |                 amzn_print_json_stats(&log, detail);
      |                                                   ^
../plugins/amzn/amzn-nvme.c:455: note: macro "amzn_print_json_stats" defined here
  455 | #define amzn_print_json_stats(log)
      | 
../plugins/amzn/amzn-nvme.c:538:17: error: ‘amzn_print_json_stats’ undeclared (first use in this function); did you mean ‘amzn_print_io_stats’?
  538 |                 amzn_print_json_stats(&log, detail);
      |                 ^~~~~~~~~~~~~~~~~~~~~
      |                 amzn_print_io_stats

@lingkai0110
Copy link
Contributor Author

Thanks. Will look into those suggestions.

@lingkai0110 lingkai0110 force-pushed the amzn-nvme-log-page-extension branch from 2c3420f to 9e86225 Compare August 22, 2025 22:31
@lingkai0110
Copy link
Contributor Author

lingkai0110 commented Aug 25, 2025

@igaw Thanks for the review. I’ve addressed your feedback in 9e86225:

• Commit title now uses the plugins/amzn: prefix and I removed the external link from the message body.
• Fixed the amzn_print_json_stats call/signature mismatch and cleaned up JSON/plain output paths.
• Fixed the format checking
• Fresh build is clean on GCC and Clang:
meson setup .build && meson compile -C .build

@lingkai0110
Copy link
Contributor Author

@igaw Thanks for the review. I’ve addressed your feedback in 9e86225:

• Commit title now uses the plugins/amzn: prefix and I removed the external link from the message body.
• Fixed the amzn_print_json_stats call/signature mismatch and cleaned up JSON/plain output paths.
• Fixed the format checking
• Fresh build is clean on GCC and Clang:
meson setup .build && meson compile -C .build

Please have a look. Thanks again.

This patch adds the stats support for EC2 local storage on top of EBS.

For local storage, it supports a `--details / -d` option. In this mode
it will print out detail io histograms for different block size ranges.

Signed-off-by: Lingkai Zuo <[email protected]>
@igaw igaw force-pushed the amzn-nvme-log-page-extension branch from 9e86225 to 9efae72 Compare August 27, 2025 14:42
@igaw
Copy link
Collaborator

igaw commented Aug 27, 2025

I've fixed a few of the checkpatch warnings, e.g. overlong lines. If a line gets too long because of the intention, it usually a sign, that the function should be splitted into smaller blocks.

Thanks!

@igaw igaw merged commit ffb1b3e into linux-nvme:master Aug 27, 2025
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.

2 participants