search

SUSE / lrbd

GNU Lesser General Public License v2.1
24 stars 19 forks source link

performance optimizations #25

Closed ddiss closed 5 years ago

ddiss commented 5 years ago

This patchset significantly improves lrbd start times by dispatching multiple commands via a single targetcli invocation.

ddiss commented 5 years ago

Thanks for the review feedback Eric...

Okay, effectively batching all of the same type of commands together. These still print to stdout so the user can investigate/rerun/debug as necessary. I think this looks good.

Yes, I've tried to keep output and error propagation as-is, via the flush_close(). I had to be a little careful that we weren't batching non-targetcli commands (e.g. rbd map) in the td.queue_cmd(cmd) loops, and that the parameters were actual commands, not --version, etc.

ddiss commented 5 years ago

This new version includes the following changes:

ddiss commented 5 years ago

FWIW, I've hit problems with stdin command feed-in against ancient targetcli-2.1-17.1.x86_64 (SLE12-SP3). This shouldn't be backported until I've fixed the ui_command_exit() behaviour there.

ddiss commented 5 years ago

New version with a one-line change to help with backports:

ddiss commented 5 years ago

One thing I noticed while backporting for SES5 is that backstore creation includes a retry loop (69e8f8ba59bf02098414adfac45b3c6fe7256efa). This functionality doesn't appear to be present in master. @swiftgist do you remember why that was added? The commit msg doesn't include a reason or bsc#.

ddiss commented 5 years ago

FWIW, I've pushed my backport to https://github.com/ddiss/lrbd-1/tree/bsc1137518_perf_no_dupes_cmd_dispatch_ses5

swiftgist commented 5 years ago

One thing I noticed while backporting for SES5 is that backstore creation includes a retry loop (69e8f8b). This functionality doesn't appear to be present in master. @swiftgist do you remember why that was added? The commit msg doesn't include a reason or bsc#.

IIRC, the initial setup from boot on QA hardware would fail intermittently. Rerunning lrbd would always work. While the problem did not lie with anything in lrbd nor targetcli, switching to retry was an easy accommodation. I do not recall a bsc#, though.

ddiss commented 5 years ago

IIRC, the initial setup from boot on QA hardware would fail intermittently. Rerunning lrbd would always work. While the problem did not lie with anything in lrbd nor targetcli, switching to retry was an easy accommodation. I do not recall a bsc#, though.

Got it, thanks. I ended up leaving the synchronous retry loop in place for ses5 backstore creation. We could make the retry apply to the entire TargetcliDispatcher() batch in future if seeking further optimization.