Add Windows native method to retrieve the number of allocated bytes on disk for file #79698

tlrx · 2021-10-25T09:27:12Z

In #79371 we fixed a bug where cache files were not created as sparse files on Windows platforms because the wrong options were used when creating the files for the first time. This bug got unnoticed as we were lacking a way to retrieve the exact number of bytes allocated for a given file on disk.

This pull request adds a new Natives.allocatedSizeInBytes(Path) method for that exact purpose (only implemented for Windows for now) and a test in CacheFileTests that would fail on Windows if the cache file is not sparse.

As a next step I'd like to add support for stat() on Linux x86 64bits platforms, and then try to craft an appropriate assertion that would verify that files are allocated on disk according to the ranges that are written in cache (this may need some non trivial computation with block sizes etc)

elasticmachine · 2021-10-25T09:27:16Z

Pinging @elastic/es-distributed (Team:Distributed)

…n disk for file

tlrx · 2021-10-25T10:53:36Z

@elasticmachine run elasticsearch-ci/part-2-windows (unrelated failure)

tlrx · 2021-10-25T15:15:18Z

Took 4 hours but the test CacheFileTests.testCacheFileCreatedAsSparseFile() passed on Windows:
https://gradle-enterprise.elastic.co/s/girhb4jk25hiu/tests/:x-pack:plugin:searchable-snapshots:test/org.elasticsearch.xpack.searchablesnapshots.cache.common.CacheFileTests/testCacheFileCreatedAsSparseFile

tlrx · 2021-10-25T18:51:27Z

Test failed as expected:
https://gradle-enterprise.elastic.co/s/stxcoxtiq6hr2/console-log/raw

org.elasticsearch.xpack.searchablesnapshots.cache.common.CacheFileTests > testCacheFileCreatedAsSparseFile FAILED
    java.lang.AssertionError: 
    Expected: not <1048576L>
         but: was <1048576L>
        at __randomizedtesting.SeedInfo.seed([B63C9F8865D2540F:33B26286A2CDA87E]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
        at org.junit.Assert.assertThat(Assert.java:956)
        at org.junit.Assert.assertThat(Assert.java:923)
        at org.elasticsearch.xpack.searchablesnapshots.cache.common.CacheFileTests.testCacheFileCreatedAsSparseFile(CacheFileTests.java:411)

Now reverting the create/create_new option and test should succeed again.

tlrx · 2021-10-26T07:23:42Z

Ok, test passed again as expected:
https://gradle-enterprise.elastic.co/s/4haqwey4fclsc/tests/:x-pack:plugin:searchable-snapshots:test/org.elasticsearch.xpack.searchablesnapshots.cache.common.CacheFileTests/testCacheFileCreatedAsSparseFile?top-execution=1

DaveCTurner

Looks great :) I left a couple of small comments.

DaveCTurner · 2021-10-26T08:24:23Z

...s/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/common/CacheFileTests.java

            fill(cacheFile.getChannel(), Math.toIntExact(cacheFile.getLength() - 1L), Math.toIntExact(cacheFile.getLength()));

            sizeOnDisk = Natives.allocatedSizeInBytes(file);
-            assertThat(sizeOnDisk, not(equalTo(1048576L)));
+            assertThat("Cache file should be sparse and not fully allocated on disk", sizeOnDisk, not(equalTo(1048576L)));


Should we be asserting its size is <1MiB rather than ≠1MiB? I worry that some funny rounding (e.g. counting the size of the directory entry) could pass this assertion even for non-sparse files.

After we make this assertion could we fill the file with genuine data and assert that its size on disk is now ≥1MiB?

Should we be asserting its size is <1MiB rather than ≠1MiB?

Yes it is more appropriate (I think I did it and then reverted it somehow 🤔 )

After we make this assertion could we fill the file with genuine data and assert that its size on disk is now ≥1MiB?

Yes too.

I pushed f109191

DaveCTurner · 2021-10-26T08:31:52Z

server/src/main/java/org/elasticsearch/bootstrap/JNAKernel32Library.java

+     * @param lpFileSizeHigh pointer to high-order DWORD for compressed file size (or null if not needed)
+     * @return the low-order DWORD for compressed file siz
+     */
+    native int GetCompressedFileSizeW(WString lpFileName, IntByReference lpFileSizeHigh);


Should we define this in test/framework rather than server since it's not used in prod code today? Or are we planning to use it in prod later on (e.g. expose sparse sizes in stats)?

Personally I'd like this to be returned as part of searchable snapshot's Cache Stats API, so that we have information about the shared cache and cold cache that sit altogether. What do you think?

I can try to make it work in the test framework; it should be OK as long as the static instance is created before the security manager is installed.

Also: I'd like to have assertion in CacheFile itself that would retrieve the allocated size on disk and compares it with the completed ranges of a cache file, and verify if size vs ranges is not completly out of bounds.

That would also require the method to be in server. I tried to make it work in the plugin itself but for JNA kernel32 library it's very difficult given all the permissions it requires (createClassLoader, setSecurityManager etc). Linux is OK though.

👍 having these stats in future seems like a good plan, so I'm ok to leave the method where it is.

DaveCTurner

LGTM (one nit)

DaveCTurner · 2021-10-26T09:08:51Z

...s/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/common/CacheFileTests.java

@@ -411,7 +412,16 @@ public void testCacheFileCreatedAsSparseFile() throws Exception {
            fill(cacheFile.getChannel(), Math.toIntExact(cacheFile.getLength() - 1L), Math.toIntExact(cacheFile.getLength()));

            sizeOnDisk = Natives.allocatedSizeInBytes(file);
-            assertThat("Cache file should be sparse and not fully allocated on disk", sizeOnDisk, not(equalTo(1048576L)));
+            assertThat("Cache file should be sparse and not fully allocated on disk", sizeOnDisk, lessThan(1048576L));


I have a slight preference for extracting the 1048576L as a constant (it appears in 3 places now and anyway I don't immediately recognise that this is a nice power of two).

DaveCTurner · 2021-10-26T09:10:11Z

server/src/main/java/org/elasticsearch/bootstrap/JNAKernel32Library.java

+     * @param lpFileSizeHigh pointer to high-order DWORD for compressed file size (or null if not needed)
+     * @return the low-order DWORD for compressed file siz
+     */
+    native int GetCompressedFileSizeW(WString lpFileName, IntByReference lpFileSizeHigh);


👍 having these stats in future seems like a good plan, so I'm ok to leave the method where it is.

tlrx · 2021-10-26T12:52:10Z

I'm holding merging this: there is a test failure and the method does not return the expected result. I'm investigating.

tlrx · 2021-10-26T16:51:32Z

All good, the test failure was caused by DisableFsyncFS which is now suppressed.

DaveCTurner

I left some suggestions, also I think there's a (super-rare) case in which we incorrectly return empty().

server/src/main/java/org/elasticsearch/common/filesystem/FileSystemNatives.java

DaveCTurner · 2021-11-05T09:03:40Z

server/src/main/java/org/elasticsearch/common/filesystem/WindowsFileSystemNatives.java

+            return OptionalLong.empty();
+        }
+
+        final long allocatedSize = (((long) lpFileSizeHigh.getValue()) << 32) | (lpFileSizeLow & 0xffffffffL);


👍 to a comment here, although it's pretty much the standard idiom for doing this computation.

Suggested change

final long allocatedSize = (((long) lpFileSizeHigh.getValue()) << 32) | (lpFileSizeLow & 0xffffffffL);

// convert lpFileSizeLow to unsigned long and combine with signed/shifted lpFileSizeHigh

final long allocatedSize = (((long) lpFileSizeHigh.getValue()) << Integer.SIZE) | (lpFileSizeLow & 0xffffffffL);

DaveCTurner · 2021-11-05T09:04:51Z

server/src/main/java/org/elasticsearch/common/filesystem/WindowsFileSystemNatives.java

+        final IntByReference lpFileSizeHigh = new IntByReference();
+
+        final int lpFileSizeLow = GetCompressedFileSizeW(fileName, lpFileSizeHigh);
+        if (lpFileSizeLow == 0xffffffff) {


It's still an int here so I think we can just say -1, but also the API docs call this constant INVALID_FILE_SIZE. Could we use the same name here?

Suggested change

if (lpFileSizeLow == 0xffffffff) {

if (lpFileSizeLow == INVALID_FILE_SIZE) {

I pushed 5928b39

DaveCTurner · 2021-11-05T09:06:36Z

server/src/main/java/org/elasticsearch/common/filesystem/WindowsFileSystemNatives.java

+        final int lpFileSizeLow = GetCompressedFileSizeW(fileName, lpFileSizeHigh);
+        if (lpFileSizeLow == 0xffffffff) {
+            final int err = Native.getLastError();
+            logger.warn("error [{}] when executing native method GetCompressedFileSizeW for file [{}]", err, path);


lpFileSizeLow can legitimately be INVALID_FILE_SIZE (== -1) if the file size is one less than a multiple of 2^32; in this case getLastError() returns NO_ERROR and we should carry on with the computation.

Wow thanks for catching this David! I remember I left this aside while I was fighting to make this work with the security manager but I forgot to come back to it.

I pushed 3357e8f

DaveCTurner

LGTM

tlrx · 2021-11-05T15:22:44Z

Thanks everybody!

…n disk for file (elastic#79698) In elastic#79371 we fixed a bug where cache files were not created as sparse files on Windows platforms because the wrong options were used when creating the files for the first time. This bug got unnoticed as we were lacking a way to retrieve the exact number of bytes allocated for a given file on disk. This commit adds a FileSystemNatives.allocatedSizeInBytes(Path) method for that exact purpose (only implemented for Windows for now) and a test in CacheFileTests that would fail on Windows if the cache file is not sparse. Relates elastic#79371

elasticsearchmachine · 2021-11-05T15:23:32Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

…d bytes on disk for a file Follow up elastic#79698

…n disk for file (#79698) (#80426) In #79371 we fixed a bug where cache files were not created as sparse files on Windows platforms because the wrong options were used when creating the files for the first time. This bug got unnoticed as we were lacking a way to retrieve the exact number of bytes allocated for a given file on disk. This commit adds a FileSystemNatives.allocatedSizeInBytes(Path) method for that exact purpose (only implemented for Windows for now) and a test in CacheFileTests that would fail on Windows if the cache file is not sparse. Relates #79371 Co-authored-by: Elastic Machine <[email protected]>

…n disk for file (#79698) (#80427) In #79371 we fixed a bug where cache files were not created as sparse files on Windows platforms because the wrong options were used when creating the files for the first time. This bug got unnoticed as we were lacking a way to retrieve the exact number of bytes allocated for a given file on disk. This commit adds a FileSystemNatives.allocatedSizeInBytes(Path) method for that exact purpose (only implemented for Windows for now) and a test in CacheFileTests that would fail on Windows if the cache file is not sparse. Relates #79371 Co-authored-by: Elastic Machine <[email protected]>

…d bytes on disk for a file (#80437) This commit introduces a new native method for Linux 64bits platforms to retrieve the number of bytes allocated on disk for a given sparse file. It calls the native glibc method __xstat. Follow up #79698

tlrx added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 test-windows Trigger CI checks on Windows v7.16.1 labels Oct 25, 2021

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 25, 2021

Add Windows native method to retrieve the number of allocated bytes o…

d13a95b

…n disk for file

tlrx force-pushed the windows-native-method-allocated-file-size branch from 972f252 to d13a95b Compare October 25, 2021 09:32

test should fail

f4c5d77

test should succeed

8f95d7b

more docs

806f501

tlrx requested review from DaveCTurner and original-brownbear October 26, 2021 07:35

tlrx changed the title ~~[Draft] Add Windows native method to retrieve the number of allocated bytes on disk for file~~ Add Windows native method to retrieve the number of allocated bytes on disk for file Oct 26, 2021

DaveCTurner reviewed Oct 26, 2021

View reviewed changes

test

f109191

tlrx requested a review from DaveCTurner October 26, 2021 08:55

DaveCTurner approved these changes Oct 26, 2021

View reviewed changes

tlrx added 2 commits October 26, 2021 12:25

nits + fsync

0ef20a0

debug last failure @testlogging

39adb83

avoid DisableFsyncFS

15abf62

tlrx requested a review from arteam October 26, 2021 14:45

remove @testlogging

d1c4f77

DaveCTurner reviewed Nov 5, 2021

View reviewed changes

tlrx added 6 commits November 5, 2021 11:38

static method to init static member

9258da1

@FunctionalInterface

e1aff78

oneMb

c00295f

comment

853a9de

INVALID_FILE_SIZE

5928b39

NO_ERROR

3357e8f

tlrx requested review from arteam and DaveCTurner November 5, 2021 11:26

DaveCTurner approved these changes Nov 5, 2021

View reviewed changes

Merge branch 'master' into windows-native-method-allocated-file-size

f77c0e0

tlrx added v8.1.0 auto-backport-and-merge labels Nov 5, 2021

tlrx merged commit 4cf8e4b into elastic:master Nov 5, 2021

tlrx deleted the windows-native-method-allocated-file-size branch November 5, 2021 15:22

tlrx mentioned this pull request Nov 5, 2021

[8.0] Add Windows native method to retrieve the number of allocated bytes on disk for file (#79698) #80426

Merged

tlrx mentioned this pull request Nov 5, 2021

[7.16] Add Windows native method to retrieve the number of allocated bytes on disk for file (#79698) #80427

Merged

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Nov 5, 2021

Add Linux x86-64bits native method to retrieve the number of allocate…

b7ff9fc

…d bytes on disk for a file Follow up elastic#79698

tlrx mentioned this pull request Nov 5, 2021

Add Linux x86-64bits native method to retrieve the number of allocated bytes on disk for a file #80437

Merged

danhermann added v7.16.0 and removed v7.16.1 labels Nov 10, 2021

mark-vieira added v8.0.0-rc1 and removed v8.0.0 labels Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows native method to retrieve the number of allocated bytes on disk for file #79698

Add Windows native method to retrieve the number of allocated bytes on disk for file #79698

tlrx commented Oct 25, 2021 •

edited

Loading

elasticmachine commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 26, 2021

DaveCTurner left a comment

DaveCTurner Oct 26, 2021

tlrx Oct 26, 2021

DaveCTurner Oct 26, 2021

tlrx Oct 26, 2021

tlrx Oct 26, 2021

DaveCTurner Oct 26, 2021

DaveCTurner left a comment •

edited

Loading

DaveCTurner Oct 26, 2021

DaveCTurner Oct 26, 2021

tlrx commented Oct 26, 2021

tlrx commented Oct 26, 2021

DaveCTurner left a comment

DaveCTurner Nov 5, 2021

DaveCTurner Nov 5, 2021

tlrx Nov 5, 2021

DaveCTurner Nov 5, 2021

tlrx Nov 5, 2021

DaveCTurner left a comment

tlrx commented Nov 5, 2021

elasticsearchmachine commented Nov 5, 2021

	final long allocatedSize = (((long) lpFileSizeHigh.getValue()) << 32) \| (lpFileSizeLow & 0xffffffffL);
	// convert lpFileSizeLow to unsigned long and combine with signed/shifted lpFileSizeHigh
	final long allocatedSize = (((long) lpFileSizeHigh.getValue()) << Integer.SIZE) \| (lpFileSizeLow & 0xffffffffL);

	if (lpFileSizeLow == 0xffffffff) {
	if (lpFileSizeLow == INVALID_FILE_SIZE) {

Add Windows native method to retrieve the number of allocated bytes on disk for file #79698

Add Windows native method to retrieve the number of allocated bytes on disk for file #79698

Conversation

tlrx commented Oct 25, 2021 • edited Loading

elasticmachine commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 25, 2021

tlrx commented Oct 26, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Oct 26, 2021

tlrx commented Oct 26, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

tlrx commented Nov 5, 2021

elasticsearchmachine commented Nov 5, 2021

💚 Backport successful

tlrx commented Oct 25, 2021 •

edited

Loading

DaveCTurner left a comment •

edited

Loading