core/rawdb: freezer index repair #29792

rjl493456442 · 2024-05-16T06:46:47Z

This pull request removes the fsync of index files in freezer.ModifyAncients function for
performance gain.

Originally, fsync is added after each freezer write operation to ensure the written data is truly
transferred into disk. Unfortunately, it turns out fsync could be relatively slow, especially on
macOS. see #28754 for more information.

In this pull request, fsync for index file is removed as it turns out index file can be recovered
even after a unclean shutdown. But fsync for data file is still kept, as we have no meaningful
way to validate the data correctness after unclean shutdown.

But why do we need the fsync in the first place?

As it's necessary for freezer to survive/recover after the machine crash (e.g. power failure).
In linux, whenever the file write is performed, the file metadata update and data update are
not necessarily performed at the same time. Typically, the metadata will be flushed/journalled
ahead of the file data. Therefore, we make the pessimistic assumption that the file is first
extended with invalid "garbage" data (normally zero bytes) and that afterwards the correct
data replaces the garbage.

We have observed that the index file of the freezer often contain garbage entry with zero value
(filenumber = 0, offset = 0) after a machine power failure. It proves that the index file is extended
without the data being flushed. And this corruption can destroy the whole freezer data eventually.

Performing fsync after each write operation can reduce the time window for data to be transferred
to the disk and ensure the correctness of the data in the disk to the greatest extent.

How can we maintain this guarantee without relying on fsync?

Because the items in the index file are strictly in order, we can leverage this characteristic to
detect the corruption and truncate them when freezer is opened. Specifically these validation
rules are performed for each index file:

For two consecutive index items:

If their file numbers are the same, then the offset of the latter one MUST not be less than that of the former.
If the file number of the latter one is equal to that of the former plus one, then the offset of the latter one MUST not be 0.
If their file numbers are not equal, and the latter's file number is not equal to the former plus 1, the latter one is valid

And also, for the first non-head item, it must refer to the earliest data file, or the next file if the
earliest file is not sufficient to place the first item(very special case, only theoretical possible
in tests)

With these validation rules, we can detect the invalid item in index file with greatest possibility.

But unfortunately, these scenarios are not covered and could still lead to a freezer corruption if it occurs:

All items in index file are in zero value

It's impossible to distinguish if they are truly zero (e.g. all the data entries maintained in freezer
are zero size) or just the garbage left by OS. In this case, these index items will be kept by truncating
the entire data file, namely the freezer is corrupted.

However, we can consider that the probability of this situation occurring is quite low, and even
if it occurs, the freezer can be considered to be close to an empty state. Rerun the state sync
should be acceptable.

Index file is integral while relative data file is corrupted

It might be possible the data file is corrupted whose file size is extended correctly with garbage
filled (e.g. zero bytes). In this case, it's impossible to detect the corruption by index validation.

We can either choose to fsync the data file, or blindly believe that if index file is integral then
the data file could be integral with very high chance. In this pull request, the first option is taken.

core/rawdb/freezer_batch.go

core/rawdb/freezer_table.go

core/rawdb/freezer_batch.go

core/rawdb/freezer_table.go

fjl · 2024-05-21T12:07:13Z

core/rawdb/freezer_table.go

+		}
+		// ensure two consecutive index items are in order
+		if err := t.checkIndexItems(prev, entry); err != nil {
+			return truncate(offset)


I think we should log the error here. Maybe pass the error into truncate to log it as part of the warning it prints.

rjl493456442 · 2024-05-22T06:36:22Z

Performance wise, it takes ~0.5 second to verify ~20m index items, namely it will introduce ~2.5s delay in geth launching, but i think it's acceptable.

INFO [05-22|06:35:09.596] Verified index file                      items=19,803,925 elapsed=529.361ms
INFO [05-22|06:35:10.173] Verified index file                      items=19,803,925 elapsed=576.231ms
INFO [05-22|06:35:10.653] Verified index file                      items=19,803,925 elapsed=480.599ms
INFO [05-22|06:35:11.172] Verified index file                      items=19,803,925 elapsed=516.957ms
INFO [05-22|06:35:11.621] Verified index file                      items=19,803,925 elapsed=447.692ms

rjl493456442 · 2024-05-24T02:01:25Z

After benchmarking the pull request for a while (full sync for more than 24 hours), this branch is slightly faster than master. Specifically:

The average time on state history construction is reduced from 2.32ms to 1.57ms, by getting rid of the index file fsync.

This performance difference is negligible on the benchmark machine with power ssd. However, for normal users, it can make a big difference as fsync on an ordinary SSD does take time. For instance, Datafile fsync takes ~18ms per block, namely we can save 18ms per block by getting rid of indexfile fsync

fjl · 2024-05-28T13:06:07Z

core/rawdb/freezer_table.go

+			}
+			log.Warn("Truncated index file", "offset", offset, "truncated", size-offset)
+			return nil
+		}


These functions could just be methods on freezerTable

fjl · 2024-07-02T11:40:17Z

core/rawdb/freezer_batch.go

@@ -194,15 +194,10 @@ func (batch *freezerTableBatch) commit() error {
 	dataSize := int64(len(batch.dataBuffer))


Could we remove the sync call above as well, if we extend/modify the repair?

Unfortunately, we have no mechanism to detect the whether the data file (batch.t.head) contains garbage or not after the power failure. It's the reason I leave the sync operation for data file.

However, I think we can do the background file sync for data file, with a specific time interval. For example, mongo uses this strategy by flushing the file every 100 milliseconds

MongoDB also uses a write-ahead logging via a separate journal. Each write operation is appended to this journal, which is then flushed to disk every 100 milliseconds, taking aspects of the group commit and async commit approaches. If you cannot tolerate losing up to 100 milliseconds of data, you can optionally force an early flush of the journal when issuing a write, increasing the commit latency.

core/rawdb/freezer_table.go

fjl · 2024-08-08T09:02:04Z

Results from triage discussion: we need to optimize the validation process to remove the startup delay. There were some ideas for optimization:

Validating in reverse is not possible: a small amount of random data can throw off the validation
We could parallelize the validation, but it would only really help on SSD, and the freezer is supposed to work on spinning disks as well.
@holiman voiced the idea of only checking the last 1M items. There are some safety concerns about this.
It would be safe to validate only the offsets pointing into the last data file. that's because we always flush all files when moving to a new data file. However, we would need a fast algorithm for locating the first index entry of the last data file to skip over most of the index.
We could store the last known good offset somewhere (in the KV-DB) and only verify from that offset onward. But note this offset would need to move back when we truncate the freezer.

holiman · 2024-08-26T11:35:36Z

Results from triage discussion: we need to optimize the validation process to remove the startup delay.

IIRC the reasoning was that if users are using an ancientdir on an HDD, it will take quite a long time to parse all index-files. However, we don't actually have any concrete numbers on that, do we?

I'm slightly leaning towards thinking that long startup-time isn't a big deal. If it takes a minute instead of 15 seconds, does it really matter? Whereas shutdown-lag is worse, since it can trigger kill-signals if not performed fast enough

holiman

I think this is a good start, even if we may fine-tune it later

fjl · 2024-08-27T15:08:06Z

High startup time is not acceptable if it occurs every time. It's not OK if Geth suddenly starts taking a minute to start on mainnet. We'd have to make it async or something for this to be acceptable.

holiman · 2024-08-28T11:56:41Z

High startup time is not acceptable if it occurs every time. It's not OK if Geth suddenly starts taking a minute to start on mainnet. We'd have to make it async or something for this to be acceptable.

To follow-up on this. Here's an index-repair performed on an HDD, after a power failure (so no hot os-caches):

INFO [08-27|19:12:54.926] Verified index file                      items=20,619,465 elapsed=623.795ms
INFO [08-27|19:12:55.561] Verified index file                      items=20,619,465 elapsed=580.057ms
INFO [08-27|19:12:56.229] Verified index file                      items=20,619,465 elapsed=647.133ms
INFO [08-27|19:12:56.922] Verified index file                      items=20,619,465 elapsed=619.618ms
INFO [08-27|19:12:57.515] Verified index file                      items=20,619,465 elapsed=569.665ms

It seems to total 2-3s additional overhead - but if it's a regular restart of geth, with some potentially hot os-caches, the numbers can be a lot faster, like

INFO [08-27|18:55:00.472] Verified index file                      items=20,618,031 elapsed=168.120ms
INFO [08-27|18:55:00.667] Verified index file                      items=20,618,031 elapsed=194.537ms
INFO [08-27|18:55:00.821] Verified index file                      items=20,618,031 elapsed=153.609ms
INFO [08-27|18:55:00.976] Verified index file                      items=20,618,031 elapsed=154.121ms
INFO [08-27|18:55:01.129] Verified index file                      items=20,618,031 elapsed=152.758ms

Totalling sub second. I don't claim credit for these numbers, which comes from @rjl493456442 . But, I'd consider this acceptable.

holiman · 2024-09-13T13:42:50Z

I think we should merge this. Let's discuss next triage

This pull request removes the `fsync` of index files in freezer.ModifyAncients function for performance gain. Originally, fsync is added after each freezer write operation to ensure the written data is truly transferred into disk. Unfortunately, it turns out `fsync` can be relatively slow, especially on macOS (see ethereum#28754 for more information). In this pull request, fsync for index file is removed as it turns out index file can be recovered even after a unclean shutdown. But fsync for data file is still kept, as we have no meaningful way to validate the data correctness after unclean shutdown. --- **But why do we need the `fsync` in the first place?** As it's necessary for freezer to survive/recover after the machine crash (e.g. power failure). In linux, whenever the file write is performed, the file metadata update and data update are not necessarily performed at the same time. Typically, the metadata will be flushed/journalled ahead of the file data. Therefore, we make the pessimistic assumption that the file is first extended with invalid "garbage" data (normally zero bytes) and that afterwards the correct data replaces the garbage. We have observed that the index file of the freezer often contain garbage entry with zero value (filenumber = 0, offset = 0) after a machine power failure. It proves that the index file is extended without the data being flushed. And this corruption can destroy the whole freezer data eventually. Performing fsync after each write operation can reduce the time window for data to be transferred to the disk and ensure the correctness of the data in the disk to the greatest extent. --- **How can we maintain this guarantee without relying on fsync?** Because the items in the index file are strictly in order, we can leverage this characteristic to detect the corruption and truncate them when freezer is opened. Specifically these validation rules are performed for each index file: For two consecutive index items: - If their file numbers are the same, then the offset of the latter one MUST not be less than that of the former. - If the file number of the latter one is equal to that of the former plus one, then the offset of the latter one MUST not be 0. - If their file numbers are not equal, and the latter's file number is not equal to the former plus 1, the latter one is valid And also, for the first non-head item, it must refer to the earliest data file, or the next file if the earliest file is not sufficient to place the first item(very special case, only theoretical possible in tests) With these validation rules, we can detect the invalid item in index file with greatest possibility. --- But unfortunately, these scenarios are not covered and could still lead to a freezer corruption if it occurs: **All items in index file are in zero value** It's impossible to distinguish if they are truly zero (e.g. all the data entries maintained in freezer are zero size) or just the garbage left by OS. In this case, these index items will be kept by truncating the entire data file, namely the freezer is corrupted. However, we can consider that the probability of this situation occurring is quite low, and even if it occurs, the freezer can be considered to be close to an empty state. Rerun the state sync should be acceptable. **Index file is integral while relative data file is corrupted** It might be possible the data file is corrupted whose file size is extended correctly with garbage filled (e.g. zero bytes). In this case, it's impossible to detect the corruption by index validation. We can either choose to `fsync` the data file, or blindly believe that if index file is integral then the data file could be integral with very high chance. In this pull request, the first option is taken.

This pull request removes the `fsync` of index files in freezer.ModifyAncients function for performance gain. Originally, fsync is added after each freezer write operation to ensure the written data is truly transferred into disk. Unfortunately, it turns out `fsync` can be relatively slow, especially on macOS (see #28754 for more information). In this pull request, fsync for index file is removed as it turns out index file can be recovered even after a unclean shutdown. But fsync for data file is still kept, as we have no meaningful way to validate the data correctness after unclean shutdown. --- **But why do we need the `fsync` in the first place?** As it's necessary for freezer to survive/recover after the machine crash (e.g. power failure). In linux, whenever the file write is performed, the file metadata update and data update are not necessarily performed at the same time. Typically, the metadata will be flushed/journalled ahead of the file data. Therefore, we make the pessimistic assumption that the file is first extended with invalid "garbage" data (normally zero bytes) and that afterwards the correct data replaces the garbage. We have observed that the index file of the freezer often contain garbage entry with zero value (filenumber = 0, offset = 0) after a machine power failure. It proves that the index file is extended without the data being flushed. And this corruption can destroy the whole freezer data eventually. Performing fsync after each write operation can reduce the time window for data to be transferred to the disk and ensure the correctness of the data in the disk to the greatest extent. --- **How can we maintain this guarantee without relying on fsync?** Because the items in the index file are strictly in order, we can leverage this characteristic to detect the corruption and truncate them when freezer is opened. Specifically these validation rules are performed for each index file: For two consecutive index items: - If their file numbers are the same, then the offset of the latter one MUST not be less than that of the former. - If the file number of the latter one is equal to that of the former plus one, then the offset of the latter one MUST not be 0. - If their file numbers are not equal, and the latter's file number is not equal to the former plus 1, the latter one is valid And also, for the first non-head item, it must refer to the earliest data file, or the next file if the earliest file is not sufficient to place the first item(very special case, only theoretical possible in tests) With these validation rules, we can detect the invalid item in index file with greatest possibility. --- But unfortunately, these scenarios are not covered and could still lead to a freezer corruption if it occurs: **All items in index file are in zero value** It's impossible to distinguish if they are truly zero (e.g. all the data entries maintained in freezer are zero size) or just the garbage left by OS. In this case, these index items will be kept by truncating the entire data file, namely the freezer is corrupted. However, we can consider that the probability of this situation occurring is quite low, and even if it occurs, the freezer can be considered to be close to an empty state. Rerun the state sync should be acceptable. **Index file is integral while relative data file is corrupted** It might be possible the data file is corrupted whose file size is extended correctly with garbage filled (e.g. zero bytes). In this case, it's impossible to detect the corruption by index validation. We can either choose to `fsync` the data file, or blindly believe that if index file is integral then the data file could be integral with very high chance. In this pull request, the first option is taken.

This pull request removes the `fsync` of index files in freezer.ModifyAncients function for performance gain. Originally, fsync is added after each freezer write operation to ensure the written data is truly transferred into disk. Unfortunately, it turns out `fsync` can be relatively slow, especially on macOS (see ethereum#28754 for more information). In this pull request, fsync for index file is removed as it turns out index file can be recovered even after a unclean shutdown. But fsync for data file is still kept, as we have no meaningful way to validate the data correctness after unclean shutdown. --- **But why do we need the `fsync` in the first place?** As it's necessary for freezer to survive/recover after the machine crash (e.g. power failure). In linux, whenever the file write is performed, the file metadata update and data update are not necessarily performed at the same time. Typically, the metadata will be flushed/journalled ahead of the file data. Therefore, we make the pessimistic assumption that the file is first extended with invalid "garbage" data (normally zero bytes) and that afterwards the correct data replaces the garbage. We have observed that the index file of the freezer often contain garbage entry with zero value (filenumber = 0, offset = 0) after a machine power failure. It proves that the index file is extended without the data being flushed. And this corruption can destroy the whole freezer data eventually. Performing fsync after each write operation can reduce the time window for data to be transferred to the disk and ensure the correctness of the data in the disk to the greatest extent. --- **How can we maintain this guarantee without relying on fsync?** Because the items in the index file are strictly in order, we can leverage this characteristic to detect the corruption and truncate them when freezer is opened. Specifically these validation rules are performed for each index file: For two consecutive index items: - If their file numbers are the same, then the offset of the latter one MUST not be less than that of the former. - If the file number of the latter one is equal to that of the former plus one, then the offset of the latter one MUST not be 0. - If their file numbers are not equal, and the latter's file number is not equal to the former plus 1, the latter one is valid And also, for the first non-head item, it must refer to the earliest data file, or the next file if the earliest file is not sufficient to place the first item(very special case, only theoretical possible in tests) With these validation rules, we can detect the invalid item in index file with greatest possibility. --- But unfortunately, these scenarios are not covered and could still lead to a freezer corruption if it occurs: **All items in index file are in zero value** It's impossible to distinguish if they are truly zero (e.g. all the data entries maintained in freezer are zero size) or just the garbage left by OS. In this case, these index items will be kept by truncating the entire data file, namely the freezer is corrupted. However, we can consider that the probability of this situation occurring is quite low, and even if it occurs, the freezer can be considered to be close to an empty state. Rerun the state sync should be acceptable. **Index file is integral while relative data file is corrupted** It might be possible the data file is corrupted whose file size is extended correctly with garbage filled (e.g. zero bytes). In this case, it's impossible to detect the corruption by index validation. We can either choose to `fsync` the data file, or blindly believe that if index file is integral then the data file could be integral with very high chance. In this pull request, the first option is taken.

rjl493456442 force-pushed the freezer-index-validation branch 4 times, most recently from a6ed90b to 1b10ff8 Compare May 20, 2024 07:52

holiman reviewed May 20, 2024

View reviewed changes

core/rawdb/freezer_batch.go Outdated Show resolved Hide resolved

core/rawdb/freezer_table.go Outdated Show resolved Hide resolved

core/rawdb/freezer_batch.go Outdated Show resolved Hide resolved

core/rawdb/freezer_table.go Outdated Show resolved Hide resolved

rjl493456442 force-pushed the freezer-index-validation branch 3 times, most recently from 9be55a0 to 660e460 Compare May 21, 2024 05:48

rjl493456442 marked this pull request as ready for review May 21, 2024 06:43

rjl493456442 requested a review from karalabe as a code owner May 21, 2024 06:43

fjl reviewed May 21, 2024

View reviewed changes

rjl493456442 force-pushed the freezer-index-validation branch from 63ffdbb to 8381d61 Compare May 28, 2024 12:59

fjl reviewed May 28, 2024

View reviewed changes

fjl reviewed Jul 2, 2024

View reviewed changes

fjl added the status:triage label Jul 2, 2024

holiman reviewed Aug 8, 2024

View reviewed changes

core/rawdb/freezer_table.go Show resolved Hide resolved

holiman removed the status:triage label Aug 20, 2024

fjl mentioned this pull request Aug 22, 2024

Failed to truncate extra state histories err="EOF #30333

Closed

rjl493456442 force-pushed the freezer-index-validation branch from 8381d61 to af6398c Compare August 27, 2024 10:48

holiman approved these changes Aug 27, 2024

View reviewed changes

rjl493456442 mentioned this pull request Sep 4, 2024

core/rawdb: introduce flush offset in freezer #30392

Open

rjl493456442 added 3 commits September 9, 2024 14:20

core/rawdb: introduce freezer index repair mechanism

7bf5d58

core/rawdb, triedb/pathdb: various fixes

af9d4bf

core/rawdb: fsync data file

65baef9

rjl493456442 added 3 commits September 9, 2024 14:20

core/rawdb: fix read error

e6ba338

core/rawdb: log the error message

8b8e5b9

core/rawdb: polish

0fe416d

rjl493456442 force-pushed the freezer-index-validation branch from af6398c to 0fe416d Compare September 9, 2024 06:21

holiman added the status:triage label Sep 13, 2024

holiman added this to the 1.14.12 milestone Oct 1, 2024

holiman removed the status:triage label Oct 1, 2024

holiman merged commit eff0bed into ethereum:master Oct 1, 2024
3 checks passed

BrewTestBot mentioned this pull request Nov 19, 2024

ethereum 1.14.12 Homebrew/homebrew-core#198233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/rawdb: freezer index repair #29792

core/rawdb: freezer index repair #29792

rjl493456442 commented May 16, 2024 •

edited

Loading

fjl May 21, 2024 •

edited

Loading

rjl493456442 commented May 22, 2024

rjl493456442 commented May 24, 2024

fjl May 28, 2024

fjl Jul 2, 2024

rjl493456442 Jul 2, 2024

fjl commented Aug 8, 2024

holiman commented Aug 26, 2024

holiman left a comment

fjl commented Aug 27, 2024

holiman commented Aug 28, 2024

holiman commented Sep 13, 2024

		@@ -194,15 +194,10 @@ func (batch *freezerTableBatch) commit() error {
		dataSize := int64(len(batch.dataBuffer))

core/rawdb: freezer index repair #29792

core/rawdb: freezer index repair #29792

Conversation

rjl493456442 commented May 16, 2024 • edited Loading

fjl May 21, 2024 • edited Loading

Choose a reason for hiding this comment

rjl493456442 commented May 22, 2024

rjl493456442 commented May 24, 2024

fjl May 28, 2024

Choose a reason for hiding this comment

fjl Jul 2, 2024

Choose a reason for hiding this comment

rjl493456442 Jul 2, 2024

Choose a reason for hiding this comment

fjl commented Aug 8, 2024

holiman commented Aug 26, 2024

holiman left a comment

Choose a reason for hiding this comment

fjl commented Aug 27, 2024

holiman commented Aug 28, 2024

holiman commented Sep 13, 2024

rjl493456442 commented May 16, 2024 •

edited

Loading

fjl May 21, 2024 •

edited

Loading