checkpoints: fix performance drop on big volume restores #1345

alaaeddineelamri · 2023-01-01T12:12:26Z

Thank you for contributing to the Bareos Project!

Description

In this PR, jobmedia table entries are not updated anymore, and only new entries can be created. This change fixes a performance issue on restores of big volumes.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

PR name is meaningful
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Is the PR title usable as CHANGELOG entry?
~~Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.~~

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR
bareos-check-sources --since-merge does not report any problems

arogge

The code looks fine. I have some comments, but it is mostly about naming and using const.

arogge · 2023-02-13T10:46:46Z

core/src/stored/append.cc

-  auto now
-      = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
-  time_t next_checkpoint_time = now + me->checkpoint_interval;
+  bool checkpoints_enabled = me->checkpoint_interval > 0 ? true : false;


Suggested change

bool checkpoints_enabled = me->checkpoint_interval > 0 ? true : false;

const bool checkpoints_enabled = me->checkpoint_interval > 0;

arogge · 2023-02-13T10:56:21Z

core/src/stored/append.cc

+      bool block_changed
+          = current_block_number != jcr->sd_impl->dcr->block->BlockNumber;
+      bool volume_changed
+          = jcr->sd_impl->dcr->VolMediaId != current_volumeid && block_changed;


Suggested change

bool block_changed

= current_block_number != jcr->sd_impl->dcr->block->BlockNumber;

bool volume_changed

= jcr->sd_impl->dcr->VolMediaId != current_volumeid && block_changed;

const bool block_changed

= current_block_number != jcr->sd_impl->dcr->block->BlockNumber;

const bool volume_changed

= jcr->sd_impl->dcr->VolMediaId != current_volumeid && block_changed;

arogge · 2023-02-13T11:02:58Z

core/src/stored/checkpointhandler.h

+  void DoBackupCheckpoint(JobControlRecord* jcr);
+  void DoTimedCheckpoint(JobControlRecord* jcr);
+  void DoVolumeChangeBackupCheckpoint(JobControlRecord* jcr);
+  void SetReadyForCheckpoint(bool ready) { ready_for_checkpoint_ = ready; }


the rest of the codebase uses SetXYZ() / ClearXYZ(), so maybe we want to apply that pattern here, too.
Also ClearReadyForCheckpoint() could even be made private.

arogge · 2023-02-13T11:05:04Z

core/src/stored/checkpointhandler.h

+  void DoTimedCheckpoint(JobControlRecord* jcr);
+  void DoVolumeChangeBackupCheckpoint(JobControlRecord* jcr);
+  void SetReadyForCheckpoint(bool ready) { ready_for_checkpoint_ = ready; }
+  bool ReadyForCheckpoint() const { return ready_for_checkpoint_; }


While this looks completely reasonable here in context, ReadyForCheckpoint() could also mean "I am ready for a checkpoint now" instead of "Am I ready for a checkpoint?"
Mark at least as [[nodiscard]] or rename to IsXYZ().

arogge · 2023-02-13T11:14:06Z

core/src/stored/checkpointhandler.cc

+
+namespace storagedaemon {
+
+CheckpointHandler::CheckpointHandler(time_t interval)


When the class' name is CheckpointHandler I would have named the files checkpoint_handler.cc and checkpoint_handler.h.

arogge

the include guards needs to be changed after renaming the file, also bareos-check-sources is unhappy with the headers in append.cc and append.h.

Also, some of the commit headlines exceed the limit that pr-tool is willing to accept.

arogge · 2023-02-13T12:33:19Z

core/src/stored/checkpoint_handler.h

+#ifndef BAREOS_STORED_CHECKPOINTHANDLER_H_
+#define BAREOS_STORED_CHECKPOINTHANDLER_H_


include guard needs to be changed on rename, too.

arogge · 2023-02-13T12:33:24Z

core/src/stored/checkpoint_handler.h

+};
+
+}  // namespace storagedaemon
+#endif  // BAREOS_STORED_CHECKPOINTHANDLER_H_


include guard needs to be changed on rename, too.

remove unused `PossibleIncompleteJob` function

Previously, when updating backup checkpoints, DIR would try to update the jobmedia table entries or create a new one in case an update is not possible which in consequence made the jobmedia table entries kind of merged. The change lowered the number of entries in the jobmedia table but had the unwanted consequence of slowing restores in certain situations (restores on big volumes for example). With this change, jobmedia entries will only get created, and checkpoints will add an entry into the jobmedia table instead of updating existing entries.

* changed when checkpoints are triggered relative to attribute saving * add extra checks for when to trigger checkpoints

after jobmedia entry creation

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch from f183406 to 52e56d7 Compare January 4, 2023 12:33

bruno-at-bareos added nobuild onhold labels Jan 12, 2023

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch from 52e56d7 to 8882856 Compare February 6, 2023 07:53

alaaeddineelamri removed onhold nobuild labels Feb 6, 2023

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch 3 times, most recently from f838940 to b6c219c Compare February 7, 2023 09:00

alaaeddineelamri marked this pull request as ready for review February 7, 2023 09:56

alaaeddineelamri changed the title ~~checkpoints: update jobmedia table updates with checkpoints~~ checkpoints: update how jobmedia table entries are added Feb 7, 2023

alaaeddineelamri changed the title ~~checkpoints: update how jobmedia table entries are added~~ checkpoints: fix performance drop on big volume restores Feb 7, 2023

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch from 12325b2 to 8c0b01a Compare February 8, 2023 17:45

bruno-at-bareos added bugfix requires backport to 22 labels Feb 9, 2023

bruno-at-bareos requested a review from arogge February 9, 2023 10:58

bruno-at-bareos assigned arogge Feb 9, 2023

arogge approved these changes Feb 13, 2023

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch from 812e8a4 to 878d413 Compare February 13, 2023 11:43

arogge requested changes Feb 13, 2023

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch 2 times, most recently from 4b49f99 to 6f55b4b Compare February 13, 2023 12:59

alaaeddineelamri requested a review from arogge February 13, 2023 13:00

arogge approved these changes Feb 13, 2023

View reviewed changes

arogge self-requested a review February 23, 2023 13:50

alaaeddineelamri added 5 commits February 23, 2023 14:51

refactor: append.cc: remove unused function

03c838f

remove unused `PossibleIncompleteJob` function

refactor: move checkpoints handling to its own class

80f2c52

refactor: rename function name to be more accurate

46e9632

stored: adapt checkpoints to multiple jobmedia table entries

2297d5a

* changed when checkpoints are triggered relative to attribute saving * add extra checks for when to trigger checkpoints

alaaeddineelamri added 6 commits February 23, 2023 14:51

stored: update startfile and startblock ...

f622ade

after jobmedia entry creation

checkpoints: create jobmedia record after volume change

bef9631

systemtest: update tests to backup bigger amount of files

66aac31

stored: rename checkpointhandler file

cc1e0c7

stored: update setting and getting readyforcheckpoint

0448d17

apply bareos-check-sources

b270c90

arogge force-pushed the dev/alaaeddineelamri/master/change-jobmedia-update branch from 6f55b4b to b270c90 Compare February 23, 2023 13:51

Update CHANGELOG.md

7114637

arogge merged commit 1e5cdb4 into bareos:master Feb 24, 2023

alaaeddineelamri mentioned this pull request Feb 24, 2023

checkpoints: fix performance drop on big volume restores #1393

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkpoints: fix performance drop on big volume restores #1345

checkpoints: fix performance drop on big volume restores #1345

alaaeddineelamri commented Jan 1, 2023 •

edited by arogge

Loading

arogge left a comment

arogge Feb 13, 2023

arogge Feb 13, 2023

arogge Feb 13, 2023

arogge Feb 13, 2023

arogge Feb 13, 2023

arogge left a comment

arogge Feb 13, 2023

arogge Feb 13, 2023

	bool checkpoints_enabled = me->checkpoint_interval > 0 ? true : false;
	const bool checkpoints_enabled = me->checkpoint_interval > 0;


		namespace storagedaemon {

		CheckpointHandler::CheckpointHandler(time_t interval)

		#ifndef BAREOS_STORED_CHECKPOINTHANDLER_H_
		#define BAREOS_STORED_CHECKPOINTHANDLER_H_

checkpoints: fix performance drop on big volume restores #1345

checkpoints: fix performance drop on big volume restores #1345

Conversation

alaaeddineelamri commented Jan 1, 2023 • edited by arogge Loading

Thank you for contributing to the Bareos Project!

Description

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

arogge left a comment

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

arogge Feb 13, 2023

Choose a reason for hiding this comment

alaaeddineelamri commented Jan 1, 2023 •

edited by arogge

Loading