Add CacheFile#fsync() method to ensure cached data are written on disk by tlrx · Pull Request #64201 · elastic/elasticsearch

tlrx · 2020-10-27T10:41:30Z

This pull request adds a new CacheFile.fsync() method to fsync the cache file on disk. This method uses the utility method IOUtils.fsync(Path, boolean) that executes a FileChannel.force() call under the hood.

elasticmachine · 2020-10-27T10:41:31Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

tlrx · 2020-10-27T10:42:26Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+                    try {
+                        // check again if the file is evicted before doing expensive I/O
+                        ensureOpen();
+                        IOUtils.fsync(file, false); // TODO don't forget to fsync parent directory


This method is intended to be called periodically by a task that will fsync all cache files of a given shard before fsyncing the parent directory.

original-brownbear

Just two questions, looks good overall

original-brownbear · 2020-10-27T11:35:01Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+                    try {
+                        // check again if the file is evicted before doing expensive I/O
+                        ensureOpen();
+                        IOUtils.fsync(file, false); // TODO don't forget to fsync parent directory


I wonder, can't we just call force on the file channel that we already have open if we have one and avoid fsync if we don't have one by fsyncing before closing the channel (would have to enhance acquireFileChannelReference to return null in case there's no eviction listeners on the current instance I guess)? No need to get a new file descriptor here maybe is there?

Yes, I also thought about borrowing the current file channel if we have one but I found that it added more complexity (mostly in incref/decref the file channel another time) to save a file descriptor that would be quickly released.

I can maybe give it another try by implementing something that use the file channel if there's one or use IOUtils.fsync() otherwise. Deferring the fsync at closing time does not seem the right thing to do as we don't want to fsync everytime a chunk is written but instead control when cache files are fsynced.

original-brownbear · 2020-10-27T12:23:40Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

                            reference.decRef();
                        }
                        gap.onCompletion();
+                        fsynced.set(false);


Might be better to set this false as early as possible (i.e. directly after the write to the file) to reduce contention statistically speaking? (since fsync and write will always be blocking each other anyway even if we do use multiple FileChannel instances I think).

I like that fsynced not only indicate that something was written to the file but that ranges are also updated - although it is not a requirement. I'm not sure if it makes a big difference but I can move this earlier.

Ah that's a good point :)

original-brownbear

Either way (regarding the file channels) LGTM, that comment does only matter if the fsync interval we will use is small I guess :)

henningandersen

I added a few comments, looking good otherwise.

henningandersen · 2020-10-28T12:38:39Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+                final List<Tuple<Long, Long>> completedRanges = tracker.getCompletedRanges();
+                assert completedRanges != null;
+
+                if (fsynced.compareAndSet(false, true)) {


I think there is an assumption of only ever going here in one thread, since otherwise we risk a concurrent thread seeing the completedRanges. I think that is fair, but perhaps for safety we should either lock the entire method or assert something to capture this?

I think this is OK, but your comment and your suggestion about returning an empty set of ranges if the cache file got evicted made me change a bit the semantic of this new fsync method.

I pushed d55278e which makes the method return a non-empty set of completed ranges only if the cache file was actually fsynced on disk; otherwise it returns an empty collection. This should address the case of concurrent threads and also simplify the logic to know if new ranges of data were effectively synchronized on disk or not.

henningandersen · 2020-10-28T12:39:23Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+                    try {
+                        // check again if the file is evicted before doing expensive I/O
+                        ensureOpen();
+                        IOUtils.fsync(file, false); // TODO don't forget to fsync parent directory


This also fsync's the metadata of the file. I think that is unnecessary?

Agreed - it is an extra I/O that we don't need. I pushed 6eea900

henningandersen · 2020-10-28T12:53:42Z

...n/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/CacheFileTests.java

+    public void testFSync() throws Exception {
+        try (FSyncTrackingFileSystemProvider fileSystem = setupFSyncCountingFileSystem()) {
+            final CacheFile cacheFile = new CacheFile("test", randomLongBetween(100, 1000), fileSystem.resolve("test"));
+            assertFalse(cacheFile.isFSynced());


I think counting the file as not fsync'ed when just created could lead to fsync'ing the empty file - which I think is unnecessary? We care mostly about the data, not the files.

Right - I pushed 2f8b0e7

henningandersen · 2020-10-28T12:58:00Z

...n/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/CacheFileTests.java

+                    if (randomBoolean()) {
+                        final long position = i * 10L; // simplify the test by completing small non-contiguous ranges
+                        final Tuple<Long, Long> range = Tuple.tuple(position, position + 1L);
+                        cacheFile.populateAndRead(


Would be good to also verify multiple populateAndRead calls within one fsync?

Sure, I randomized this in the test now.

henningandersen · 2020-10-28T13:04:56Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+     * @throws AlreadyClosedException            if the cache file is evicted
+     * @throws java.nio.file.NoSuchFileException if the cache file does not exist
+     */
+    public List<Tuple<Long, Long>> fsync() throws IOException {


Should this return a SortedSet instead to signal the expected order verified by the test, as well as uniqueness?

This is a good suggestion, thanks. I pushed 7b3590e

henningandersen · 2020-10-28T13:06:51Z

...n/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/CacheFileTests.java

+                }
+
+                cacheFile.startEviction();
+                expectThrows(AlreadyClosedException.class, cacheFile::fsync);


This can definitely be handled in a follow-up instead when you start using it. But it might be more appropriate to just return the empty set of ranges in this case, since we essentially having nothing cached for it, rather than have to exception handle in the fsync'er thread?

I agree. In case of an evicted file the method will now return an empty collection. See #64201 (comment)

henningandersen · 2020-10-28T13:13:42Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+                if (fsynced.compareAndSet(false, true)) {
+                    boolean success = false;
+                    try {
+                        // check again if the file is evicted before doing expensive I/O


I wonder if this is necessary, since there is no waiting above?

I removed it.

tlrx · 2020-11-02T12:17:54Z

@henningandersen Thanks for your review. I updated the code and changed a bit the semantic of the fsync method. Can you please have another look and tell me what you think? Thanks!

henningandersen

LGTM, thanks for the extra iteration.

henningandersen · 2020-11-03T12:04:20Z

...plugin/searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/CacheFile.java

+     * considered as fsynced (the initialization value is {@code true}) when it is created; and writing new data to the cache file
+     * will toggle the flag to {@code false}.
+     **/
+    private final AtomicBoolean fsynced = new AtomicBoolean(true);


I wonder if calling this needsFsync (and reverting the boolean) would be slightly more intuitive?

With the current name, a reader would be inclined to think the boolean is only true when the file is fsync'ed, but this is not true (it is also still true while writing and completing the gap).

Thanks, it makes sense. I pushed d8fdaaa2aab25e78033906f2fa8c956a6e340441.

tlrx · 2020-11-03T14:10:02Z

Thanks Armin and Henning! I'm merging this only on master for now and will backport this change and the futures one about the persistent cache in one go.

…iles (#64696) This committ changes the searchable snapshot's CacheService so that it now periodically fsync cache files using the method introduced in #64201. The synchronization is executed every 10 seconds by default (this interval can be changed using a new xpack.searchable.snapshot.cache.sync.interval setting).

elastic#64201) This commit adds a new CacheFile.fsync() method to fsync the cache file on disk. This method uses the utility method IOUtils.fsync(Path, boolean) that executes a FileChannel.force() call under the hood.

#66200) This commit adds a new CacheFile.fsync() method to fsync the cache file on disk. This method uses the utility method IOUtils.fsync(Path, boolean) that executes a FileChannel.force() call under the hood. Backport of #64201 for 7.11

…iles (elastic#64696) This committ changes the searchable snapshot's CacheService so that it now periodically fsync cache files using the method introduced in elastic#64201. The synchronization is executed every 10 seconds by default (this interval can be changed using a new xpack.searchable.snapshot.cache.sync.interval setting).

…files #64696 (#66216) This committ changes the searchable snapshot's CacheService so that it now periodically fsync cache files using the method introduced in #64201. The synchronization is executed every 10 seconds by default (this interval can be changed using a new xpack.searchable.snapshot.cache.sync.interval setting). Backport of #64696 for 7.11

Add CacheFile#fsync() method to ensure cached data are written on disk

314212b

tlrx added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.11.0 labels Oct 27, 2020

elasticmachine added the Team:Distributed Meta label for distributed team. label Oct 27, 2020

tlrx commented Oct 27, 2020

View reviewed changes

tlrx requested a review from original-brownbear October 27, 2020 11:18

original-brownbear reviewed Oct 27, 2020

View reviewed changes

original-brownbear approved these changes Oct 27, 2020

View reviewed changes

tlrx requested a review from henningandersen October 27, 2020 13:02

henningandersen reviewed Oct 28, 2020

View reviewed changes

tlrx added 4 commits November 2, 2020 11:14

fsynced no metadata

6eea900

fsynced at creation time

2f8b0e7

sorted set

7b3590e

feedback

d55278e

tlrx requested a review from henningandersen November 2, 2020 12:18

tlrx and others added 3 commits November 2, 2020 13:53

Merge branch 'master' into add-cache-file-fsync

8f5d13b

Merge branch 'master' into add-cache-file-fsync

f0c3cbe

Merge branch 'master' into add-cache-file-fsync

0d43e5b

henningandersen approved these changes Nov 3, 2020

View reviewed changes

needsFsync

d8fdaaa

tlrx merged commit 34fc303 into elastic:master Nov 3, 2020

tlrx deleted the add-cache-file-fsync branch November 3, 2020 14:08

tlrx added backport pending and removed v7.11.0 labels Nov 3, 2020

tlrx mentioned this pull request Nov 6, 2020

Allow searchable snapshot cache service to periodically fsync cache files #64696

Merged

tlrx mentioned this pull request Dec 11, 2020

Add CacheFile#fsync() method to ensure cached data are written on disk #66200

Merged

tlrx added v7.11.0 and removed backport pending labels Dec 12, 2020

tlrx mentioned this pull request Dec 12, 2020

Allow searchable snapshot cache service to periodically fsync cache files #64696 #66216

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

tlrx commented Oct 27, 2020

Uh oh!

elasticmachine commented Oct 27, 2020

Uh oh!

tlrx Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Nov 2, 2020

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tlrx Oct 27, 2020 •

edited

Loading