glusterfs icon indicating copy to clipboard operation
glusterfs copied to clipboard

Multiple files: reduce redirection for measure_latency variable

Open mykaul opened this issue 2 years ago • 3 comments

As far as I can see, we have the ctx variable in the call_stack_t structure only to be able to access the measure_latency boolean. Example from create_frame():

if (frame->root->ctx->measure_latency) {
    timespec_now(&stack->tv);
    memcpy(&frame->begin, &stack->tv, sizeof(stack->tv));
}

I did not see any other use for the ctx variable apart from that boolean. We can instead just copy that boolean and reduce one level of indirection (which seem to be happening in create_frame(), FRAME_DESTROY() and STACK_UNWIND_STRICT macro)

Updates: #1874 Signed-off-by: Yaniv Kaul [email protected]

mykaul avatar Jun 27 '22 18:06 mykaul

/run regression

mykaul avatar Jun 27 '22 21:06 mykaul

0 test(s) failed

1 test(s) generated core ./tests/bugs/glusterd/mgmt-handshake-and-volume-sync-post-glusterd-restart.t

7 test(s) needed retry ./tests/000-flaky/basic_afr_split-brain-favorite-child-policy.t ./tests/000-flaky/basic_mount-nfs-auth.t ./tests/000-flaky/bugs_nfs_bug-1116503.t ./tests/basic/afr/ta-shd.t ./tests/bugs/bitrot/bug-1245981.t ./tests/bugs/changelog/bug-1208470.t ./tests/bugs/glusterd/bug-1696046.t

2 flaky test(s) marked as success even though they failed ./tests/000-flaky/basic_mount-nfs-auth.t ./tests/000-flaky/bugs_nfs_bug-1116503.t https://build.gluster.org/job/gh_centos7-regression/2593/

gluster-ant avatar Jun 28 '22 00:06 gluster-ant

/run regression

mykaul avatar Jul 10 '22 12:07 mykaul

I think this patch has a problem: by default measure_latency is true for all xlators, but global_xl_reconfigure() only modifies the value for the global xlator. So I don't see how the option can be disabled.

Perhaps I need to change glusterfs_graph_insert() and glusterfs_graph_prepare() to inherit the value instead of just setting it to true.

mykaul avatar Sep 29 '22 10:09 mykaul

I think this patch has a problem: by default measure_latency is true for all xlators, but global_xl_reconfigure() only modifies the value for the global xlator. So I don't see how the option can be disabled.

Perhaps I need to change glusterfs_graph_insert() and glusterfs_graph_prepare() to inherit the value instead of just setting it to true.

The problem with this is that the value won't be modifiable at runtime. This option is commonly used to enable profiling temporarily to debug some issue. If we don't allow a dynamic change, a volume restart would be needed to enable profiling.

An alternative is to only keep measure_latency in the stack, and set it from the ctx->measure_latency at stack creation. Then all STACK_* macros can use it instead of accessing ctx. I think this covers most of its use cases, at least the most important ones.

xhernandez avatar Sep 29 '22 11:09 xhernandez

Thank you for your contributions. Noticed that this issue is not having any activity in last ~6 months! We are marking this issue as stale because it has not had recent activity. It will be closed in 2 weeks if no one responds with a comment here.

stale[bot] avatar May 20 '23 15:05 stale[bot]

Closing this issue as there was no update since my last update on issue. If this is an issue which is still valid, feel free to open it.

stale[bot] avatar Jun 10 '23 03:06 stale[bot]