Remove usage of FileSwitchDirectory (#42937)

We are still using `FileSwitchDirectory` in the case a user configures file based pre-load of mmaps. This is trappy for multiple reasons if the both directories used by `FileSwitchDirectory` point to the same filesystem directory. One issue is LUCENE-8835 that cause issues like #37111 - unless LUCENE-8835 isn't fixed we should not use it in elasticsearch. Instead we use a similar trick as we use for HybridFS and subclass mmap directory directly.

plugins/store-smb/src/main/java/org/elasticsearch/index/store/smbmmapfs/SmbMmapFsDirectoryFactory.java

@@ -22,17 +22,20 @@ package org.elasticsearch.index.store.smbmmapfs;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.LockFactory;
 import org.apache.lucene.store.MMapDirectory;
+import org.elasticsearch.index.IndexModule;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.store.FsDirectoryFactory;
 import org.elasticsearch.index.store.SmbDirectoryWrapper;
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.HashSet;
 
 public final class SmbMmapFsDirectoryFactory extends FsDirectoryFactory {
 
     @Override
     protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
-        return new SmbDirectoryWrapper(new MMapDirectory(location, lockFactory));
+        return new SmbDirectoryWrapper(setPreload(new MMapDirectory(location, lockFactory), lockFactory, new HashSet<>(
+            indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING))));
     }
 }

server/src/main/java/org/elasticsearch/index/store/FsDirectoryFactory.java

@@ -64,11 +64,7 @@ public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {
         final Path location = path.resolveIndex();
         final LockFactory lockFactory = indexSettings.getValue(INDEX_LOCK_FACTOR_SETTING);
         Files.createDirectories(location);
-        Directory wrapped = newFSDirectory(location, lockFactory, indexSettings);
-        Set<String> preLoadExtensions = new HashSet<>(
-                indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING));
-        wrapped = setPreload(wrapped, location, lockFactory, preLoadExtensions);
-        return wrapped;
+        return newFSDirectory(location, lockFactory, indexSettings);
     }
 
     protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
@@ -80,17 +76,20 @@ public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {
         } else {
             type = IndexModule.Type.fromSettingsKey(storeType);
         }
+        Set<String> preLoadExtensions = new HashSet<>(
+            indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING));
         switch (type) {
             case HYBRIDFS:
                 // Use Lucene defaults
                 final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory);
                 if (primaryDirectory instanceof MMapDirectory) {
-                    return new HybridDirectory(location, lockFactory, primaryDirectory);
+                    MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory;
+                    return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions));
                 } else {
                     return primaryDirectory;
                 }
             case MMAPFS:
-                return new MMapDirectory(location, lockFactory);
+                return setPreload(new MMapDirectory(location, lockFactory), lockFactory, preLoadExtensions);
             case SIMPLEFS:
                 return new SimpleFSDirectory(location, lockFactory);
             case NIOFS:
@@ -100,26 +99,17 @@ public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {
         }
     }
 
-    private static Directory setPreload(Directory directory, Path location, LockFactory lockFactory,
+    public static MMapDirectory setPreload(MMapDirectory mMapDirectory, LockFactory lockFactory,
             Set<String> preLoadExtensions) throws IOException {
-        if (preLoadExtensions.isEmpty() == false
-                && directory instanceof MMapDirectory
-                && ((MMapDirectory) directory).getPreload() == false) {
+        assert mMapDirectory.getPreload() == false;
+        if (preLoadExtensions.isEmpty() == false) {
             if (preLoadExtensions.contains("*")) {
-                ((MMapDirectory) directory).setPreload(true);
-                return directory;
+                mMapDirectory.setPreload(true);
+            } else {
+                return new PreLoadMMapDirectory(mMapDirectory, lockFactory, preLoadExtensions);
             }
-            MMapDirectory primary = new MMapDirectory(location, lockFactory);
-            primary.setPreload(true);
-            return new FileSwitchDirectory(preLoadExtensions, primary, directory, true) {
-                @Override
-                public String[] listAll() throws IOException {
-                    // avoid listing twice
-                    return primary.listAll();
-                }
-            };
         }
-        return directory;
+        return mMapDirectory;
     }
 
     /**
@@ -131,15 +121,35 @@ public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {
     }
 
     static final class HybridDirectory extends NIOFSDirectory {
-        private final FSDirectory randomAccessDirectory;
+        private final MMapDirectory delegate;
 
-        HybridDirectory(Path location, LockFactory lockFactory, FSDirectory randomAccessDirectory) throws IOException {
-            super(location, lockFactory);
-            this.randomAccessDirectory = randomAccessDirectory;
+        HybridDirectory(LockFactory lockFactory, MMapDirectory delegate) throws IOException {
+            super(delegate.getDirectory(), lockFactory);
+            this.delegate = delegate;
         }
 
         @Override
         public IndexInput openInput(String name, IOContext context) throws IOException {
+            if (useDelegate(name)) {
+                // we need to do these checks on the outer directory since the inner doesn't know about pending deletes
+                ensureOpen();
+                ensureCanRead(name);
+                // we only use the mmap to open inputs. Everything else is managed by the NIOFSDirectory otherwise
+                // we might run into trouble with files that are pendingDelete in one directory but still
+                // listed in listAll() from the other. We on the other hand don't want to list files from both dirs
+                // and intersect for perf reasons.
+                return delegate.openInput(name, context);
+            } else {
+                return super.openInput(name, context);
+            }
+        }
+
+        @Override
+        public void close() throws IOException {
+            IOUtils.close(super::close, delegate);
+        }
+
+        boolean useDelegate(String name) {
             String extension = FileSwitchDirectory.getExtension(name);
             switch(extension) {
                 // We are mmapping norms, docvalues as well as term dictionaries, all other files are served through NIOFS
@@ -148,26 +158,59 @@ public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {
                 case "dvd":
                 case "tim":
                 case "cfs":
-                    // we need to do these checks on the outer directory since the inner doesn't know about pending deletes
-                    ensureOpen();
-                    ensureCanRead(name);
-                    // we only use the mmap to open inputs. Everything else is managed by the NIOFSDirectory otherwise
-                    // we might run into trouble with files that are pendingDelete in one directory but still
-                    // listed in listAll() from the other. We on the other hand don't want to list files from both dirs
-                    // and intersect for perf reasons.
-                    return randomAccessDirectory.openInput(name, context);
+                    return true;
                 default:
-                    return super.openInput(name, context);
+                    return false;
+            }
+        }
+
+        MMapDirectory getDelegate() {
+            return delegate;
+        }
+    }
+    // TODO it would be nice to share code between PreLoadMMapDirectory and HybridDirectory but due to the nesting aspect of
+    // directories here makes it tricky. It would be nice to allow MMAPDirectory to pre-load on a per IndexInput basis.
+    static final class PreLoadMMapDirectory extends MMapDirectory {
+        private final MMapDirectory delegate;
+        private final Set<String> preloadExtensions;
+
+        PreLoadMMapDirectory(MMapDirectory delegate, LockFactory lockFactory, Set<String> preload) throws IOException {
+            super(delegate.getDirectory(), lockFactory);
+            super.setPreload(false);
+            this.delegate = delegate;
+            this.delegate.setPreload(true);
+            this.preloadExtensions = preload;
+            assert getPreload() == false;
+        }
+
+        @Override
+        public void setPreload(boolean preload) {
+            throw new IllegalArgumentException("can't set preload on a preload-wrapper");
+        }
+
+        @Override
+        public IndexInput openInput(String name, IOContext context) throws IOException {
+            if (useDelegate(name)) {
+                // we need to do these checks on the outer directory since the inner doesn't know about pending deletes
+                ensureOpen();
+                ensureCanRead(name);
+                return delegate.openInput(name, context);
             }
+            return super.openInput(name, context);
         }
 
         @Override
-        public void close() throws IOException {
-            IOUtils.close(super::close, randomAccessDirectory);
+        public synchronized void close() throws IOException {
+            IOUtils.close(super::close, delegate);
+        }
+
+        boolean useDelegate(String name) {
+            final String extension = FileSwitchDirectory.getExtension(name);
+            return preloadExtensions.contains(extension);
         }
 
-        Directory getRandomAccessDirectory() {
-            return randomAccessDirectory;
+        MMapDirectory getDelegate() {
+            return delegate;
         }
     }
 }

server/src/test/java/org/elasticsearch/index/store/FsDirectoryFactoryTests.java

@@ -18,8 +18,9 @@
  */
 package org.elasticsearch.index.store;
 
+import org.apache.lucene.store.AlreadyClosedException;
 import org.apache.lucene.store.Directory;
-import org.apache.lucene.store.FileSwitchDirectory;
+import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.MMapDirectory;
 import org.apache.lucene.store.NIOFSDirectory;
 import org.apache.lucene.store.NoLockFactory;
@@ -36,6 +37,7 @@ import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.index.shard.ShardPath;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.IndexSettingsModule;
+import org.hamcrest.Matchers;
 
 import java.io.IOException;
 import java.nio.file.Files;
@@ -49,34 +51,65 @@ public class FsDirectoryFactoryTests extends ESTestCase {
         doTestPreload();
         doTestPreload("nvd", "dvd", "tim");
         doTestPreload("*");
+        Settings build = Settings.builder()
+            .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT))
+            .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "dvd", "bar")
+            .build();
+        try (Directory directory = newDirectory(build)) {
+            assertTrue(FsDirectoryFactory.isHybridFs(directory));
+            FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory;
+            assertTrue(hybridDirectory.useDelegate("foo.dvd"));
+            assertTrue(hybridDirectory.useDelegate("foo.nvd"));
+            assertTrue(hybridDirectory.useDelegate("foo.tim"));
+            assertTrue(hybridDirectory.useDelegate("foo.cfs"));
+            assertFalse(hybridDirectory.useDelegate("foo.bar"));
+            MMapDirectory delegate = hybridDirectory.getDelegate();
+            assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class));
+            FsDirectoryFactory.PreLoadMMapDirectory preLoadMMapDirectory = (FsDirectoryFactory.PreLoadMMapDirectory) delegate;
+            assertTrue(preLoadMMapDirectory.useDelegate("foo.dvd"));
+            assertTrue(preLoadMMapDirectory.useDelegate("foo.bar"));
+        }
+    }
+
+    private Directory newDirectory(Settings settings) throws IOException {
+        IndexSettings idxSettings = IndexSettingsModule.newIndexSettings("foo", settings);
+        Path tempDir = createTempDir().resolve(idxSettings.getUUID()).resolve("0");
+        Files.createDirectories(tempDir);
+        ShardPath path = new ShardPath(false, tempDir, tempDir, new ShardId(idxSettings.getIndex(), 0));
+        return new FsDirectoryFactory().newDirectory(idxSettings, path);
     }
 
     private void doTestPreload(String...preload) throws IOException {
         Settings build = Settings.builder()
-                .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "mmapfs")
-                .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), preload)
-                .build();
-        IndexSettings settings = IndexSettingsModule.newIndexSettings("foo", build);
-        Path tempDir = createTempDir().resolve(settings.getUUID()).resolve("0");
-        Files.createDirectories(tempDir);
-        ShardPath path = new ShardPath(false, tempDir, tempDir, new ShardId(settings.getIndex(), 0));
-        FsDirectoryFactory fsDirectoryFactory = new FsDirectoryFactory();
-        Directory directory = fsDirectoryFactory.newDirectory(settings, path);
-        assertFalse(directory instanceof SleepingLockWrapper);
-        if (preload.length == 0) {
-            assertTrue(directory.toString(), directory instanceof MMapDirectory);
-            assertFalse(((MMapDirectory) directory).getPreload());
-        } else if (Arrays.asList(preload).contains("*")) {
-            assertTrue(directory.toString(), directory instanceof MMapDirectory);
-            assertTrue(((MMapDirectory) directory).getPreload());
-        } else {
-            assertTrue(directory.toString(), directory instanceof FileSwitchDirectory);
-            FileSwitchDirectory fsd = (FileSwitchDirectory) directory;
-            assertTrue(fsd.getPrimaryDir() instanceof MMapDirectory);
-            assertTrue(((MMapDirectory) fsd.getPrimaryDir()).getPreload());
-            assertTrue(fsd.getSecondaryDir() instanceof MMapDirectory);
-            assertFalse(((MMapDirectory) fsd.getSecondaryDir()).getPreload());
+            .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "mmapfs")
+            .putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), preload)
+            .build();
+        Directory directory = newDirectory(build);
+        try (Directory dir = directory){
+            assertSame(dir, directory); // prevent warnings
+            assertFalse(directory instanceof SleepingLockWrapper);
+            if (preload.length == 0) {
+                assertTrue(directory.toString(), directory instanceof MMapDirectory);
+                assertFalse(((MMapDirectory) directory).getPreload());
+            } else if (Arrays.asList(preload).contains("*")) {
+                assertTrue(directory.toString(), directory instanceof MMapDirectory);
+                assertTrue(((MMapDirectory) directory).getPreload());
+            } else {
+                assertTrue(directory.toString(), directory instanceof FsDirectoryFactory.PreLoadMMapDirectory);
+                FsDirectoryFactory.PreLoadMMapDirectory preLoadMMapDirectory = (FsDirectoryFactory.PreLoadMMapDirectory) directory;
+                for (String ext : preload) {
+                    assertTrue("ext: " + ext, preLoadMMapDirectory.useDelegate("foo." + ext));
+                    assertTrue("ext: " + ext, preLoadMMapDirectory.getDelegate().getPreload());
+                }
+                assertFalse(preLoadMMapDirectory.useDelegate("XXX"));
+                assertFalse(preLoadMMapDirectory.getPreload());
+                preLoadMMapDirectory.close();
+                expectThrows(AlreadyClosedException.class, () -> preLoadMMapDirectory.getDelegate().openInput("foo.bar",
+                    IOContext.DEFAULT));
+            }
         }
+        expectThrows(AlreadyClosedException.class, () -> directory.openInput(randomBoolean() && preload.length != 0 ?
+            "foo." + preload[0] : "foo.bar", IOContext.DEFAULT));
     }
 
     public void testStoreDirectory() throws IOException {
@@ -102,7 +135,7 @@ public class FsDirectoryFactoryTests extends ESTestCase {
         try (Directory directory = service.newFSDirectory(tempDir, NoLockFactory.INSTANCE, indexSettings)) {
             switch (type) {
                 case HYBRIDFS:
-                    assertHybridDirectory(directory);
+                    assertTrue(FsDirectoryFactory.isHybridFs(directory));
                     break;
                 case NIOFS:
                     assertTrue(type + " " + directory.toString(), directory instanceof NIOFSDirectory);
@@ -115,7 +148,7 @@ public class FsDirectoryFactoryTests extends ESTestCase {
                     break;
                 case FS:
                     if (Constants.JRE_IS_64BIT && MMapDirectory.UNMAP_SUPPORTED) {
-                        assertHybridDirectory(directory);
+                        assertTrue(FsDirectoryFactory.isHybridFs(directory));
                     } else if (Constants.WINDOWS) {
                         assertTrue(directory.toString(), directory instanceof SimpleFSDirectory);
                     } else {
@@ -127,10 +160,4 @@ public class FsDirectoryFactoryTests extends ESTestCase {
             }
         }
     }
-
-    private void assertHybridDirectory(Directory directory) {
-        assertTrue(directory.toString(), directory instanceof FsDirectoryFactory.HybridDirectory);
-        Directory randomAccessDirectory = ((FsDirectoryFactory.HybridDirectory) directory).getRandomAccessDirectory();
-        assertTrue("randomAccessDirectory:  " +  randomAccessDirectory.toString(), randomAccessDirectory instanceof MMapDirectory);
-    }
 }