Skip to content

Commit 9d2adfb

Browse files
s1monws1monw
committed
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.
1 parent 7f2f0b7 commit 9d2adfb

File tree

3 files changed

+146
-73
lines changed

3 files changed

+146
-73
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,20 @@
2222
import org.apache.lucene.store.Directory;
2323
import org.apache.lucene.store.LockFactory;
2424
import org.apache.lucene.store.MMapDirectory;
25+
import org.elasticsearch.index.IndexModule;
2526
import org.elasticsearch.index.IndexSettings;
2627
import org.elasticsearch.index.store.FsDirectoryFactory;
2728
import org.elasticsearch.index.store.SmbDirectoryWrapper;
2829

2930
import java.io.IOException;
3031
import java.nio.file.Path;
32+
import java.util.HashSet;
3133

3234
public final class SmbMmapFsDirectoryFactory extends FsDirectoryFactory {
3335

3436
@Override
3537
protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
36-
return new SmbDirectoryWrapper(new MMapDirectory(location, lockFactory));
38+
return new SmbDirectoryWrapper(setPreload(new MMapDirectory(location, lockFactory), lockFactory, new HashSet<>(
39+
indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING))));
3740
}
3841
}

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

Lines changed: 83 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,7 @@ public Directory newDirectory(IndexSettings indexSettings, ShardPath path) throw
6464
final Path location = path.resolveIndex();
6565
final LockFactory lockFactory = indexSettings.getValue(INDEX_LOCK_FACTOR_SETTING);
6666
Files.createDirectories(location);
67-
Directory wrapped = newFSDirectory(location, lockFactory, indexSettings);
68-
Set<String> preLoadExtensions = new HashSet<>(
69-
indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING));
70-
wrapped = setPreload(wrapped, location, lockFactory, preLoadExtensions);
71-
return wrapped;
67+
return newFSDirectory(location, lockFactory, indexSettings);
7268
}
7369

7470
protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
@@ -80,17 +76,20 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index
8076
} else {
8177
type = IndexModule.Type.fromSettingsKey(storeType);
8278
}
79+
Set<String> preLoadExtensions = new HashSet<>(
80+
indexSettings.getValue(IndexModule.INDEX_STORE_PRE_LOAD_SETTING));
8381
switch (type) {
8482
case HYBRIDFS:
8583
// Use Lucene defaults
8684
final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory);
8785
if (primaryDirectory instanceof MMapDirectory) {
88-
return new HybridDirectory(location, lockFactory, primaryDirectory);
86+
MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory;
87+
return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions));
8988
} else {
9089
return primaryDirectory;
9190
}
9291
case MMAPFS:
93-
return new MMapDirectory(location, lockFactory);
92+
return setPreload(new MMapDirectory(location, lockFactory), lockFactory, preLoadExtensions);
9493
case SIMPLEFS:
9594
return new SimpleFSDirectory(location, lockFactory);
9695
case NIOFS:
@@ -100,26 +99,17 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index
10099
}
101100
}
102101

103-
private static Directory setPreload(Directory directory, Path location, LockFactory lockFactory,
102+
public static MMapDirectory setPreload(MMapDirectory mMapDirectory, LockFactory lockFactory,
104103
Set<String> preLoadExtensions) throws IOException {
105-
if (preLoadExtensions.isEmpty() == false
106-
&& directory instanceof MMapDirectory
107-
&& ((MMapDirectory) directory).getPreload() == false) {
104+
assert mMapDirectory.getPreload() == false;
105+
if (preLoadExtensions.isEmpty() == false) {
108106
if (preLoadExtensions.contains("*")) {
109-
((MMapDirectory) directory).setPreload(true);
110-
return directory;
107+
mMapDirectory.setPreload(true);
108+
} else {
109+
return new PreLoadMMapDirectory(mMapDirectory, lockFactory, preLoadExtensions);
111110
}
112-
MMapDirectory primary = new MMapDirectory(location, lockFactory);
113-
primary.setPreload(true);
114-
return new FileSwitchDirectory(preLoadExtensions, primary, directory, true) {
115-
@Override
116-
public String[] listAll() throws IOException {
117-
// avoid listing twice
118-
return primary.listAll();
119-
}
120-
};
121111
}
122-
return directory;
112+
return mMapDirectory;
123113
}
124114

125115
/**
@@ -131,15 +121,35 @@ public static boolean isHybridFs(Directory directory) {
131121
}
132122

133123
static final class HybridDirectory extends NIOFSDirectory {
134-
private final FSDirectory randomAccessDirectory;
124+
private final MMapDirectory delegate;
135125

136-
HybridDirectory(Path location, LockFactory lockFactory, FSDirectory randomAccessDirectory) throws IOException {
137-
super(location, lockFactory);
138-
this.randomAccessDirectory = randomAccessDirectory;
126+
HybridDirectory(LockFactory lockFactory, MMapDirectory delegate) throws IOException {
127+
super(delegate.getDirectory(), lockFactory);
128+
this.delegate = delegate;
139129
}
140130

141131
@Override
142132
public IndexInput openInput(String name, IOContext context) throws IOException {
133+
if (useDelegate(name)) {
134+
// we need to do these checks on the outer directory since the inner doesn't know about pending deletes
135+
ensureOpen();
136+
ensureCanRead(name);
137+
// we only use the mmap to open inputs. Everything else is managed by the NIOFSDirectory otherwise
138+
// we might run into trouble with files that are pendingDelete in one directory but still
139+
// listed in listAll() from the other. We on the other hand don't want to list files from both dirs
140+
// and intersect for perf reasons.
141+
return delegate.openInput(name, context);
142+
} else {
143+
return super.openInput(name, context);
144+
}
145+
}
146+
147+
@Override
148+
public void close() throws IOException {
149+
IOUtils.close(super::close, delegate);
150+
}
151+
152+
boolean useDelegate(String name) {
143153
String extension = FileSwitchDirectory.getExtension(name);
144154
switch(extension) {
145155
// We are mmapping norms, docvalues as well as term dictionaries, all other files are served through NIOFS
@@ -148,26 +158,59 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
148158
case "dvd":
149159
case "tim":
150160
case "cfs":
151-
// we need to do these checks on the outer directory since the inner doesn't know about pending deletes
152-
ensureOpen();
153-
ensureCanRead(name);
154-
// we only use the mmap to open inputs. Everything else is managed by the NIOFSDirectory otherwise
155-
// we might run into trouble with files that are pendingDelete in one directory but still
156-
// listed in listAll() from the other. We on the other hand don't want to list files from both dirs
157-
// and intersect for perf reasons.
158-
return randomAccessDirectory.openInput(name, context);
161+
return true;
159162
default:
160-
return super.openInput(name, context);
163+
return false;
164+
}
165+
}
166+
167+
MMapDirectory getDelegate() {
168+
return delegate;
169+
}
170+
}
171+
// TODO it would be nice to share code between PreLoadMMapDirectory and HybridDirectory but due to the nesting aspect of
172+
// directories here makes it tricky. It would be nice to allow MMAPDirectory to pre-load on a per IndexInput basis.
173+
static final class PreLoadMMapDirectory extends MMapDirectory {
174+
private final MMapDirectory delegate;
175+
private final Set<String> preloadExtensions;
176+
177+
PreLoadMMapDirectory(MMapDirectory delegate, LockFactory lockFactory, Set<String> preload) throws IOException {
178+
super(delegate.getDirectory(), lockFactory);
179+
super.setPreload(false);
180+
this.delegate = delegate;
181+
this.delegate.setPreload(true);
182+
this.preloadExtensions = preload;
183+
assert getPreload() == false;
184+
}
185+
186+
@Override
187+
public void setPreload(boolean preload) {
188+
throw new IllegalArgumentException("can't set preload on a preload-wrapper");
189+
}
190+
191+
@Override
192+
public IndexInput openInput(String name, IOContext context) throws IOException {
193+
if (useDelegate(name)) {
194+
// we need to do these checks on the outer directory since the inner doesn't know about pending deletes
195+
ensureOpen();
196+
ensureCanRead(name);
197+
return delegate.openInput(name, context);
161198
}
199+
return super.openInput(name, context);
162200
}
163201

164202
@Override
165-
public void close() throws IOException {
166-
IOUtils.close(super::close, randomAccessDirectory);
203+
public synchronized void close() throws IOException {
204+
IOUtils.close(super::close, delegate);
205+
}
206+
207+
boolean useDelegate(String name) {
208+
final String extension = FileSwitchDirectory.getExtension(name);
209+
return preloadExtensions.contains(extension);
167210
}
168211

169-
Directory getRandomAccessDirectory() {
170-
return randomAccessDirectory;
212+
MMapDirectory getDelegate() {
213+
return delegate;
171214
}
172215
}
173216
}

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

Lines changed: 59 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,9 @@
1818
*/
1919
package org.elasticsearch.index.store;
2020

21+
import org.apache.lucene.store.AlreadyClosedException;
2122
import org.apache.lucene.store.Directory;
22-
import org.apache.lucene.store.FileSwitchDirectory;
23+
import org.apache.lucene.store.IOContext;
2324
import org.apache.lucene.store.MMapDirectory;
2425
import org.apache.lucene.store.NIOFSDirectory;
2526
import org.apache.lucene.store.NoLockFactory;
@@ -36,6 +37,7 @@
3637
import org.elasticsearch.index.shard.ShardPath;
3738
import org.elasticsearch.test.ESTestCase;
3839
import org.elasticsearch.test.IndexSettingsModule;
40+
import org.hamcrest.Matchers;
3941

4042
import java.io.IOException;
4143
import java.nio.file.Files;
@@ -49,34 +51,65 @@ public void testPreload() throws IOException {
4951
doTestPreload();
5052
doTestPreload("nvd", "dvd", "tim");
5153
doTestPreload("*");
54+
Settings build = Settings.builder()
55+
.put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.HYBRIDFS.name().toLowerCase(Locale.ROOT))
56+
.putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), "dvd", "bar")
57+
.build();
58+
try (Directory directory = newDirectory(build)) {
59+
assertTrue(FsDirectoryFactory.isHybridFs(directory));
60+
FsDirectoryFactory.HybridDirectory hybridDirectory = (FsDirectoryFactory.HybridDirectory) directory;
61+
assertTrue(hybridDirectory.useDelegate("foo.dvd"));
62+
assertTrue(hybridDirectory.useDelegate("foo.nvd"));
63+
assertTrue(hybridDirectory.useDelegate("foo.tim"));
64+
assertTrue(hybridDirectory.useDelegate("foo.cfs"));
65+
assertFalse(hybridDirectory.useDelegate("foo.bar"));
66+
MMapDirectory delegate = hybridDirectory.getDelegate();
67+
assertThat(delegate, Matchers.instanceOf(FsDirectoryFactory.PreLoadMMapDirectory.class));
68+
FsDirectoryFactory.PreLoadMMapDirectory preLoadMMapDirectory = (FsDirectoryFactory.PreLoadMMapDirectory) delegate;
69+
assertTrue(preLoadMMapDirectory.useDelegate("foo.dvd"));
70+
assertTrue(preLoadMMapDirectory.useDelegate("foo.bar"));
71+
}
72+
}
73+
74+
private Directory newDirectory(Settings settings) throws IOException {
75+
IndexSettings idxSettings = IndexSettingsModule.newIndexSettings("foo", settings);
76+
Path tempDir = createTempDir().resolve(idxSettings.getUUID()).resolve("0");
77+
Files.createDirectories(tempDir);
78+
ShardPath path = new ShardPath(false, tempDir, tempDir, new ShardId(idxSettings.getIndex(), 0));
79+
return new FsDirectoryFactory().newDirectory(idxSettings, path);
5280
}
5381

5482
private void doTestPreload(String...preload) throws IOException {
5583
Settings build = Settings.builder()
56-
.put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "mmapfs")
57-
.putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), preload)
58-
.build();
59-
IndexSettings settings = IndexSettingsModule.newIndexSettings("foo", build);
60-
Path tempDir = createTempDir().resolve(settings.getUUID()).resolve("0");
61-
Files.createDirectories(tempDir);
62-
ShardPath path = new ShardPath(false, tempDir, tempDir, new ShardId(settings.getIndex(), 0));
63-
FsDirectoryFactory fsDirectoryFactory = new FsDirectoryFactory();
64-
Directory directory = fsDirectoryFactory.newDirectory(settings, path);
65-
assertFalse(directory instanceof SleepingLockWrapper);
66-
if (preload.length == 0) {
67-
assertTrue(directory.toString(), directory instanceof MMapDirectory);
68-
assertFalse(((MMapDirectory) directory).getPreload());
69-
} else if (Arrays.asList(preload).contains("*")) {
70-
assertTrue(directory.toString(), directory instanceof MMapDirectory);
71-
assertTrue(((MMapDirectory) directory).getPreload());
72-
} else {
73-
assertTrue(directory.toString(), directory instanceof FileSwitchDirectory);
74-
FileSwitchDirectory fsd = (FileSwitchDirectory) directory;
75-
assertTrue(fsd.getPrimaryDir() instanceof MMapDirectory);
76-
assertTrue(((MMapDirectory) fsd.getPrimaryDir()).getPreload());
77-
assertTrue(fsd.getSecondaryDir() instanceof MMapDirectory);
78-
assertFalse(((MMapDirectory) fsd.getSecondaryDir()).getPreload());
84+
.put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "mmapfs")
85+
.putList(IndexModule.INDEX_STORE_PRE_LOAD_SETTING.getKey(), preload)
86+
.build();
87+
Directory directory = newDirectory(build);
88+
try (Directory dir = directory){
89+
assertSame(dir, directory); // prevent warnings
90+
assertFalse(directory instanceof SleepingLockWrapper);
91+
if (preload.length == 0) {
92+
assertTrue(directory.toString(), directory instanceof MMapDirectory);
93+
assertFalse(((MMapDirectory) directory).getPreload());
94+
} else if (Arrays.asList(preload).contains("*")) {
95+
assertTrue(directory.toString(), directory instanceof MMapDirectory);
96+
assertTrue(((MMapDirectory) directory).getPreload());
97+
} else {
98+
assertTrue(directory.toString(), directory instanceof FsDirectoryFactory.PreLoadMMapDirectory);
99+
FsDirectoryFactory.PreLoadMMapDirectory preLoadMMapDirectory = (FsDirectoryFactory.PreLoadMMapDirectory) directory;
100+
for (String ext : preload) {
101+
assertTrue("ext: " + ext, preLoadMMapDirectory.useDelegate("foo." + ext));
102+
assertTrue("ext: " + ext, preLoadMMapDirectory.getDelegate().getPreload());
103+
}
104+
assertFalse(preLoadMMapDirectory.useDelegate("XXX"));
105+
assertFalse(preLoadMMapDirectory.getPreload());
106+
preLoadMMapDirectory.close();
107+
expectThrows(AlreadyClosedException.class, () -> preLoadMMapDirectory.getDelegate().openInput("foo.bar",
108+
IOContext.DEFAULT));
109+
}
79110
}
111+
expectThrows(AlreadyClosedException.class, () -> directory.openInput(randomBoolean() && preload.length != 0 ?
112+
"foo." + preload[0] : "foo.bar", IOContext.DEFAULT));
80113
}
81114

82115
public void testStoreDirectory() throws IOException {
@@ -102,7 +135,7 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo
102135
try (Directory directory = service.newFSDirectory(tempDir, NoLockFactory.INSTANCE, indexSettings)) {
103136
switch (type) {
104137
case HYBRIDFS:
105-
assertHybridDirectory(directory);
138+
assertTrue(FsDirectoryFactory.isHybridFs(directory));
106139
break;
107140
case NIOFS:
108141
assertTrue(type + " " + directory.toString(), directory instanceof NIOFSDirectory);
@@ -115,7 +148,7 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo
115148
break;
116149
case FS:
117150
if (Constants.JRE_IS_64BIT && MMapDirectory.UNMAP_SUPPORTED) {
118-
assertHybridDirectory(directory);
151+
assertTrue(FsDirectoryFactory.isHybridFs(directory));
119152
} else if (Constants.WINDOWS) {
120153
assertTrue(directory.toString(), directory instanceof SimpleFSDirectory);
121154
} else {
@@ -127,10 +160,4 @@ private void doTestStoreDirectory(Path tempDir, String typeSettingValue, IndexMo
127160
}
128161
}
129162
}
130-
131-
private void assertHybridDirectory(Directory directory) {
132-
assertTrue(directory.toString(), directory instanceof FsDirectoryFactory.HybridDirectory);
133-
Directory randomAccessDirectory = ((FsDirectoryFactory.HybridDirectory) directory).getRandomAccessDirectory();
134-
assertTrue("randomAccessDirectory: " + randomAccessDirectory.toString(), randomAccessDirectory instanceof MMapDirectory);
135-
}
136163
}

0 commit comments

Comments
 (0)