resource-agents icon indicating copy to clipboard operation
resource-agents copied to clipboard

storage_mon: Use the O_DIRECT flag in open() to eliminate cache effects

Open inouekazu opened this issue 2 years ago • 4 comments

Use of O_DIRECT in open() eliminates the possibility of false negatives resulting from buffer cache reads.

please refer to https://lists.clusterlabs.org/pipermail/users/2022-August/030459.html for background.

inouekazu avatar Aug 10 '22 08:08 inouekazu

Can one of the admins verify this patch?

knet-ci-bot avatar Aug 10 '22 08:08 knet-ci-bot

test this please

fabbione avatar Aug 10 '22 09:08 fabbione

I agree with Ken (in the above thread) that there should be a fallback if O_DIRECT fails. There are filesystems and options even on Linux that don't always support O_DIRECT and they shouldn't be ecxluded or errored

chrissie-c avatar Aug 10 '22 11:08 chrissie-c

  • https://github.com/ClusterLabs/resource-agents/pull/1797/commits/ce4e632f29ed6b86b82a959eac5844655baed153
    • According to https://ci.kronosnet.org/job/resource-agents-build-all-voting/1530/, build targets are FreeBSD and RHEL etc., so I deleted #if defined(__linux__) || defined(__FreeBSD__).
    • Fixed FreeBSD build failure.
  • The open() manual says it returns EINVAL for filesystems that do not support O_DIRECT. Are there any issues that the implementation below does not address?
    • https://github.com/ClusterLabs/resource-agents/pull/1797/files#diff-91e236cc5ccce26f5a934e98aef5f1082251a8676e999b5364add5603538483dR44

inouekazu avatar Aug 16 '22 12:08 inouekazu

Also, I don't understand why the error checking for the ioctl is done on the sec_size value rather than the ioctl return. If that's fine then a comment saying why would be helpful. Maybe we need both?

Some kind of shortcut for 'ioctl has to succeed and not return bogus'. But some comment would of course make sense. Basically I guess the code should be safe though I don't know if there are actually cases where ioctl would succeed without returning a proper block-size. Same code-pattern btw. has been in sbd since ever.

wenningerk avatar Aug 22 '22 09:08 wenningerk

  • https://github.com/ClusterLabs/resource-agents/pull/1797/commits/7a0aaa0dfdebeab3fae9fe9ddc412c3d1f610273
    • reflected the https://github.com/ClusterLabs/resource-agents/pull/1797#pullrequestreview-1080084034.

inouekazu avatar Aug 24 '22 08:08 inouekazu

retest this please

chrissie-c avatar Aug 24 '22 09:08 chrissie-c

retest this please

chrissie-c avatar Aug 24 '22 09:08 chrissie-c

Thanks.

oalbrigt avatar Aug 31 '22 10:08 oalbrigt