CMCDragonkai
commented
2 years ago This is a big upgrade... it would result in v3.5.0, external API doesn't really change, it's mostly internal things. Lots of patterns produced now.
Closed CMCDragonkai closed 2 years ago
CMCDragonkai
commented
2 years ago This is a big upgrade... it would result in v3.5.0, external API doesn't really change, it's mostly internal things. Lots of patterns produced now.
CMCDragonkai
commented
2 years ago The INodeManager.gcAll originally used this.db.transact to lock up the DB every time it did any deletion operations. This should no longer be necessary with transactional iteration. No locks are even needed here since it's only called by INodeManager.start and INodeManager.stop.
CMCDragonkai
commented
2 years ago In integrating the @matrixai/db. I've found the NonEmptyArray type to be quite annoying. It requires alot of casting.
If we remove the requirement, and that would mean [] has to mean something. It could mean the key '' and thus be equivalent to [''].
That could simplify the code and remove all the as unknown as KeyPath.
CMCDragonkai
commented
2 years ago Only issue is that some places the keyPath is spread into other key paths like ['data', ...keyPath]. Prior to this happening, the an empty keypath must be turned into [''] to ensure that a keypath always has at least 1 entry.
CMCDragonkai
commented
2 years ago The @matrixai/db has been updated to 3.2.2 and now KeyPath [] becomes ['']. No more as unknown as KeyPath type casting required.
CMCDragonkai
commented
2 years ago When I removed the INodeManager.transact method I noticed quite an increase in verbosity. I realised the with* convenience wrappers like in RWLockWriter.withReadF() and RWLockWriter.withReadG() are actually quite nice here, and they can take over the transact method.
However I wanted to improve the API a little bit. Since we now know how to do NonEmptyArray, we can also create variadic argument types that specify the last type as a callback.
So now we have:
public async withTransactionF<T>(
...args: [
...inos: INodeIndex[],
f: (tran: DBTransaction) => Promise<T>
]
) {
const f = args.pop() as (tran: DBTransaction) => Promise<T>;
return withF(
[
this.db.transaction(),
this.getLocks(...args as Array<INodeIndex>)
],
([tran]) => f(tran)
);
}
public withTransactionG<T, TReturn, TNext>(
...args: [
...inos: INodeIndex[],
g: (tran: DBTransaction) => AsyncGenerator<T, TReturn, TNext>,
]
): AsyncGenerator<T, TReturn, TNext> {
const g = args.pop() as (tran: DBTransaction) => AsyncGenerator<T, TReturn, TNext>;
return withG(
[
this.db.transaction(),
this.getLocks(...args as Array<INodeIndex>)
],
([tran]) => g(tran)
);
}This actually conveniently replaces the transact methods. And you get to state up front what inodes you need to lock if any.
This is nicer than having the inodes that you need to lock at the very end.
Furthermore, notice that I "specialise" the resources to just the transaction, because the user won't have any use for the lock objects.
The user can always still compose much more complex resource contexts by using the getLocks and putting together the ResourceAcquire.
CMCDragonkai
commented
2 years ago Also moving to selective imports so that tests can be run by themselves.
import { permissions } from '@';To:
import * as permissions from '@/permissions';I tried getting a vscode regex to do a big find and replace. Couldn't work due to matches braces requirements.
CMCDragonkai
commented
2 years ago There are no functions that are exposing a generator interface atm. Even streams are using the FD abstraction, and they don't actually hold lock/transaction during their lifetime. This is because every write is a single operation. So for now there's no use of INodeManager.withTransactionG except internally inside INodeManager.
CMCDragonkai
commented
2 years ago I've found that the FileDescriptor methods of write and read aren't using one entire transaction for their operations. It seems like could be encapsulated in one transaction... should test this later.
CMCDragonkai
commented
2 years ago The EncryptedFS has been using alot of:
fd ? [fd.ino] : [],And also for target. They should be using fd != null or target != null. The ino could be 0 in its type, although this is prevented by starting the resource counter at 1. I'm changing over.
CMCDragonkai
commented
2 years ago Everything is migrated over, however I got a few test failures in EncryptedFS.
CMCDragonkai
commented
2 years ago The canary check logic should be moved into DB during await DB.create() and DB.start. It doesn't make sense for it to be in the EFS. Plus now that DB has "hidden" sublevels, this can be done in the root level.
CMCDragonkai
commented
2 years ago This is now done in js-db at 3.2.3. And the canary check has been removed. The ErrorEncryptedFSKey remains though and wraps around the ErrorDBKey when thrown.
CMCDragonkai
commented
2 years ago Current tests state, all passes except 2:
First:
● EncryptedFS Files › truncate › truncates the fd position
expect(received).toEqual(expected) // deep equality
- Expected - 2
+ Received + 2
Object {
"data": Array [
100,
- 98,
- 99,
+ 101,
+ 102,
],
"type": "Buffer",
}
183 | await efs.ftruncate(fd, 4);
184 | await efs.read(fd, buf, 0, buf.length);
> 185 | expect(buf).toEqual(Buffer.from('dbc'));
| ^
186 | await efs.close(fd);
187 | });
188 | });
at Object.<anonymous> (tests/EncryptedFS.files.test.ts:185:19) ● EncryptedFS Concurrency › Appending to a file that is being written for stream
expect(received).toContain(expected) // indexOf
Expected substring: "A"
Received string: "BBBBBBBBBB"
820 |
821 | const fileContents = (await efs.readFile('file')).toString();
> 822 | expect(fileContents).toContain('A');
| ^
823 | expect(fileContents).toContain('B');
824 | expect(fileContents).toContain('AB');
825 |
at Object.<anonymous> (tests/EncryptedFS.concurrent.test.ts:822:26)Both appear to involve concurrent writing/reading to file blocks which is the trickiest part, and that which is impacted by https://github.com/MatrixAI/js-encryptedfs/pull/63#issuecomment-1094910309
CMCDragonkai
commented
2 years ago Replacing INodeManager's lock collection with LockBox from @matrixai/async-locks 2.2.0.
CMCDragonkai
commented
2 years ago No problems integrating LockBox.
In doing so I discovered that there's no need to remove entries from the lock collection since the lock entries are now removed when there is nothing holding them.
This does mean that it has to construct the lock object each time someone is trying to lock it. Which can be a bit inefficient.
But at the same time, not having to keep it around reduces programmer burden to figure out when to clear or delete a lock entry.
In the future, the LockBox can additionally be cached if necessary.
CMCDragonkai
commented
2 years ago For the first test failure involving truncates the fd position.
It appears the efs.ftruncate(fd, 4); isn't doing anything.
The data in teh /fdtest should be abcdef, and when truncating it should become abcd. The pointer should be at d, and therefore the end result should be dbc.
I confirmed this with a normal fs. However this is now broken. Need to trace through ftruncate.
CMCDragonkai
commented
2 years ago The ftruncate leads to the INodeManager.fileWriteBlock. At the very end, it says it's writing 4 bytes. However the resulting block is still abcdef and not abcd.
Need to compare with what is in master.
CMCDragonkai
commented
2 years ago In master, it doesn't go into that case at all. It instead goes to fileWriteBlock:
if (!block) {
await tran.put(dataDomain, key, data, true);
bytesWritten = data.length;
} else {The reason is because:
let block = await this.fileGetBlock(tran, ino, idx);Is undefined. It is looking up ino = 2 and idx = 0.
This is quite strange. Because in master, the block doesn't exist, and thus fileGetBlock is undefined.
However in the current PR, it does exist.
Now should it exist or not? If you already have a file that has abcdef. If you open up a r+ descriptor, that doesn't clear the file.
Therefore it seems to make sense that the block would still exist, and the master is wrong to not have the buffer.
CMCDragonkai
commented
2 years ago Reduced this problem to the simplest reproduction:
import fs from 'fs/promises';
async function main () {
await fs.writeFile('./tmp/fdtest', 'abcdef');
const fd = await fs.open('./tmp/fdtest', 'r+');
await fd.truncate(4);
await fd.close();
// File is abcd
console.log(await fs.readFile('./tmp/fdtest', { encoding: 'utf-8' }));
}
main();vs:
import os from 'os';
import fs from 'fs';
import pathNode from 'path';
import Logger, { StreamHandler, LogLevel } from '@matrixai/logger';
import { EncryptedFS, utils } from './src';
async function main () {
const logger = new Logger('EncryptedFS Files', LogLevel.WARN, [
new StreamHandler(),
]);
const dataDir = await fs.promises.mkdtemp(
pathNode.join(os.tmpdir(), 'encryptedfs-test-'),
);
const dbPath = `${dataDir}/db`;
const dbKey: Buffer = utils.generateKeySync(256);
const efs = await EncryptedFS.createEncryptedFS({
dbKey,
dbPath,
umask: 0o022,
logger,
});
await efs.writeFile('/fdtest', 'abcdef');
// File is abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
const fd = await efs.open('/fdtest', 'r+');
// File is abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
await efs.ftruncate(fd, 4);
await efs.close(fd);
// File should be abcd, but is instead abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
}
main();In the current branch, the resulting file isn't truncated. It's still abcdef.
In the master branch it does it correctly to abcd just like the normal fs.
CMCDragonkai
commented
2 years ago The main difference is in INodeManager.fileWriteBlock.
When looking up the block during truncation, in master, we have undefined, which means it ends up writing the abcd.
However in the current branch, it does find the block which means it doesn't write a new block being just abcd, but instead it would "write-over" the abcdef, and thus end up with the same result.
The question is why does master see no block while this current branch does see the block.
CMCDragonkai
commented
2 years ago The fileGetBlocks has been changed from master using createReadStream, to now using the tran.iterator in this current branch.
This could mean that it is correctly acquiring the block in the current branch, because it is still abcdef.
I'm not sure if the fileGetBlocks is correct anyway since it has a while loop that as long as the buffer index is not equal to the block count, it yields empty blocks. But this while loop will never become true/false...?
CMCDragonkai
commented
2 years ago I noticed that ftruncate has to call await this.iNodeMgr.fileClearData(fd.ino, tran); prior to calling fileSetBlocks.
This would mean that it is intended that the block is undefined if fileClearData is meant to clear all the blocks in that sublevel.
However it appears that even though it is clearing the right key: [ 'INodeManager', 'data', '2', <Buffer 00> ], the block is still being found afterwards. This might be a bug in our branch then.
CMCDragonkai
commented
2 years ago I've checked, right after calling await tran.del([...dataPath, k]) I try to then await tran.get([...dataPath, k]); it appears the await tran.del has no effect. Possibly due to the tran.iterator.
I will need to investigate the js-db for a possible bug here.
CMCDragonkai
commented
2 years ago Confirmed this is a problem in js-db.
import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';
import { DB } from './src';
import * as testUtils from './tests/utils';
async function main () {
const logger = new Logger(`${DB.name} Test`, LogLevel.WARN, [
new StreamHandler(),
]);
const crypto = {
key: testUtils.generateKeySync(256),
ops: {
encrypt: testUtils.encrypt,
decrypt: testUtils.decrypt,
},
};
const db = await DB.createDB({
dbPath: './tmp/db',
crypto,
fresh: true,
logger
});
const acquireTran = db.transaction();
await db.put(
['INodeManager', 'data', '2', Buffer.from([0])],
Buffer.from('abcdef'),
true
);
let releaseTran, tran;
[releaseTran, tran] = await acquireTran();
console.log('BEFORE DELETE', await tran!.get(
['INodeManager', 'data', '2', Buffer.from([0])],
true
));
await tran!.del(
['INodeManager', 'data', '2', Buffer.from([0])],
);
console.log('AFTER DELETE', await tran!.get(
['INodeManager', 'data', '2', Buffer.from([0])],
true
));
await releaseTran();
console.log('AFTER TRANSACTION', await db.get(
['INodeManager', 'data', '2', Buffer.from([0])],
true
));
await db.stop();
}
main();This shows that when there's data in the database at a particular key. If I later start a transaction, acquire that data, but then delete it, if I get it again, this doesn't reflect the mutation I had just done. It thinks that data is still there.
Only after the transaction is committed, is the data fully removed. Which is what we want, but at the very least within the transaction we need to have some consistency in that within a transaction get after delete should be undefined.
CMCDragonkai
commented
2 years ago Additionally some improvements on DB is possible here:
withTransactionF and withTransactionG.ResourceAcquire type right now always specifies that the resource is optional. However that should only the default generic parameter, if it is specified, then the resource should always be there. That avoids having to use tran! when we know it's there.CMCDragonkai
commented
2 years ago Upgrading to @matrixai/db to 3.3.0 has fixed this bug of get after delete consistency.
Furthermore I reckon the ftruncate isn't entirely efficient here. Right now it loads all the blocks into memory, and then proceeds to overwrite the existing blocks in the DB.
With our transactional iteration, we should be able to identify the block index we need to truncate to, and then do a more precise deletion of the blocks above, and the overwrite of that specific block that needs to be done. Will address this after all bugs are solved.
However the second bug is still a problem.
● EncryptedFS Concurrency › Appending to a file that is being written for stream
expect(received).toContain(expected) // indexOf
Expected substring: "A"
Received string: "BBBBBBBBBB"
820 |
821 | const fileContents = (await efs.readFile('file')).toString();
> 822 | expect(fileContents).toContain('A');Just discovered that createWriteStream and createReadStream seems to be incorrect. It's written asynchronously, but it is unnecessary to make these asynchronous as they just create a stream object.
CMCDragonkai
commented
2 years ago The stream test failing because if we have 2 concurrent operations, one that writes from a stream and another is appending to the file, we should see something like:
AAAAAAAAAABBBBBBBBBBor
BBBBBBBBBBAAAAAAAAAAHowever in our EFS we are currently only seeing: BBBBBBBBBB.
This probably has to do with our new DBTransaction.
import fs from 'fs';
import fsPromises from 'fs/promises';
async function main () {
await fsPromises.writeFile('./tmp/streamtest', '');
const writeStream = fs.createWriteStream('/tmp/streamtest');
await Promise.all([
new Promise((resolve) => {
writeStream.write(Buffer.from('AAAAAAAAAA'), () => {
writeStream.end(() => {
resolve(null);
});
});
}),
fsPromises.appendFile('/tmp/streamtest', 'BBBBBBBBBB'),
]);
// Shows AAAAAAAAAABBBBBBBBBB
console.log((await fsPromises.readFile('/tmp/streamtest')).toString());
}
main();This is easily reproduced with just:
await efs.writeFile('/fdtest', '');
await Promise.all([
efs.appendFile('/fdtest', 'AAA'),
efs.appendFile('/fdtest', 'BBB'),
]);
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8' }));
await efs.stop();I think this might be a lost update problem.
Basically 2 transaction are committing to the same key. However they should be locking onto the same key and therefore the appends should end up being written one at a time.
CMCDragonkai
commented
2 years ago This may be related to task 5, that is a single transaction context for the entire write/read operation rather than how it is done now.
CMCDragonkai
commented
2 years ago I'm thinking that each FileDescriptor itself should have a lock. This is because FileDescriptor has the position state that is being mutated during read and write operations. It would make sense, that each individual read/write operations should be blocking each other.
Checking this https://stackoverflow.com/questions/5057737/simultaneous-read-on-file-descriptor-from-two-threads it seems POSIX is unspecified on this operation.
I need to check with normal fs, and see how concurrent read, or write works here.
A solution may turn out to be:
FileDescriptor instance to synchronise the read and write, setPos methods.read, write and setPos operation.CMCDragonkai
commented
2 years ago This script proves that at least for JS, concurrent reads and writes on a file descriptor is synchronised.
import fs from 'fs';
import fsPromises from 'fs/promises';
async function main () {
await fsPromises.writeFile('./tmp/ctest', 'ABCDEFG');
let fd = fs.openSync('./tmp/ctest', 'r');
const b1 = Buffer.alloc(5);
const b2 = Buffer.alloc(5);
console.log(b1, b2);
const results = await Promise.all([
new Promise((resolve) => {
// @ts-ignore
fs.read(fd, { buffer: b1 }, (e, bytesRead, buf) => {
console.log(e, bytesRead, buf);
resolve(buf);
});
}),
new Promise((resolve) => {
// @ts-ignore
fs.read(fd, { buffer: b2 }, (e, bytesRead, buf) => {
console.log(e, bytesRead, buf);
resolve(buf);
});
}),
]);
fs.closeSync(fd);
fd = fs.openSync('./tmp/ctest', 'w');
// @ts-ignore
console.log(results.map(b => b.toString()));
await Promise.all([
new Promise<void>((resolve) => {
// @ts-ignore
fs.write(fd, Buffer.from('aaa'), () => {
resolve();
});
}),
new Promise<void>((resolve) => {
fs.write(fd, Buffer.from('bbb'), () => {
resolve();
});
}),
]);
console.log(await fsPromises.readFile('./tmp/ctest', { encoding: 'utf-8' }));
}
main();Since setPos has an optional tran parameter. I'm also adding the optional tran parameter to read and write. Although not all EFS methods would actually end up using it.
CMCDragonkai
commented
2 years ago In applying the lock and a single transaction context in task 5, we now fixed that specific test failure, however many other tests in the tests/EncryptedFS.concurrent.test.ts are broken. I believe this was due to an incorrect implementation done for the file descriptor from the beginning, so the tests for the file descriptors are themselves incorrect.
Need to review all the concurrent tests again. @tegefaulkes
CMCDragonkai
commented
2 years ago Some of the concurrent tests are quite brittle as they expect deterministic concurrent behaviour. This is incorrect, for example when reading a directory and deleting the directory, it should deal with the possibility that the directory is first deleted, or last deleted.
I'm simplifying these tests so that they actually test for non-determinism.
@tegefaulkes
CMCDragonkai
commented
2 years ago Another concurrency bug this time by doing 10 append file operations on a file that doesn't exist.
The problem here is that because the file doesn't exist, each append file operation is trying to create a new file, and therefore a new inode number.
Our locks on inode doesn't help here because it's all locking on different inode numbers.
If the file already did exist, then concurrent appendFile would work as they would all lock on the same inode number.
It seems what we need to do is to ensure that our open on the same "key" here is the same lock. In this case the key is the directory entry for that directory.
The same situation is actually happening with concurrent writeFile when the file doesn't exist, because each operation is technically creating new inodes.
See this:
test.only('10 short writes with EncryptedFS.writeFile', async () => {
const contents = [
'one',
'two',
'three',
'four',
'five',
'six',
'seven',
'eight',
'nine',
'ten',
];
const promises: Array<any> = [];
for (const content of contents) {
promises.push(efs.writeFile('test', content));
}
await Promise.all(promises);
// One of the writes wins
expect(contents).toContainEqual(await efs.readFile('test', { encoding: 'utf-8' }));
// Only ten or inode 11 is supposed to be left
console.log(await efs.readFile('test', { encoding: 'utf-8' }));
// However all of these inodes still exist (they are just gc-able)
for (const i of [2,3,4,5,6,7,8,9,10,11]) {
console.log(await iNodeMgr.get(i as INodeIndex));
}
});This is basically the object-map locking problem (https://gist.github.com/CMCDragonkai/f58f08e7eaab0430ed4467ca35527a42). How do we ensure that the creation of a specific object is also locked.
When I developed the LockBox it was intended to also help this particular usecase, so we need to figure out how to resolve this in EFS, and it can be used in the rest of PK.
CMCDragonkai
commented
2 years ago The problem is that our lockbox is mapping inode index to locks, but at the abstraction level of EFS, it's trying to create the same file indexed by the file name/directory entry.
So while it is correct that we should be indexing locks by the inode index, we also need to index the inode index by the directory entry/file name.
This would imply that each directory should be locking the inode allocation for a given entry as well. So if there 10 async requests for a new inode for the entry file, then each request should be given the same inode index.
CMCDragonkai
commented
2 years ago There are potential solutions for this.
LockBox for each directory that is indexed on each directory entry. Remember LockBox are in-memory objects, and we no longer have a Directory object. It's all managed by INodeManager. Which means keeping references to the LockBox would have be done by the INodeManager which is kind of annoying. You'd have to create a LockBox for each directory inode possibly dynamically. This would enforce mutual exclusion at the directory-level so that creation operations involving the same entry can be blocked.INodeManager lockbox indexed by inode index. This can be done without introducing additional locks simply because inode allocation is synchronous operation on the js-resource-counter tree structure.I prefer solution 2, and will be attempting this. This however requires an intermediate in-memory map data structure that maps directory entry names to allocated inode indexes. This would have to be maintained by INodeManager or EncryptedFS. It seems like a INodeManager problem. Because Directory objects do not exist, you still have to dynamically (synchronously) construct this map structure for when directory inodes are being interacted with. At the very least, solution 2 doesn't require using the LockBox, because inode allocations are synchronous.
CMCDragonkai
commented
2 years ago Under EncryptedFS._open, this line is where the node is allocated:
if (target == null) {
if (!navigated.remaining && openFlags & constants.O_CREAT) {
let navigatedDirStat;
const fileINode = this.iNodeMgr.inoAllocate();
await this.iNodeMgr.withTransactionF(
fileINode,
navigated.dir,
async (tran) => {The indoe is allocated being locked.
We could use the navigated object to provide properties that index this inode allocation.
navigated.dir // directory inode index
navigated.target // target inode index (it is undefined)
navigated.name // directory entry nameWe could pass this into inoAllocate(navigated.dir, navigated.name) and inoDeallocate(iNode, navigated.dir, navigated.name). There are only 5 places in EFS where inoAllocate is called, and it is always under the context of creating something under a directory EXCEPT for the root inode. Immediately afterwards a transaction context is created that does something like: withTransactionF(navigated.dir, targetInode). This should enable us to coalesce calls to allocate inodes. Since we would need both parameters like navigated.dir and navigated.name, we could pass an object or tuple {dir: navigated.dir, name: navigated.name} that is optional. The inoDeallocate right now just takes the inode index. If our coalescing structure is bimap, we could look up the inode as well and deallocate it rather than needing to pass the dir and name as well.
There is an additional issue here.
The _open will wrap the functionality in a transaction locked on the target inode if it existed prior. However if it didn't exist, and it was created then, the transaction context created for the new inode index doesn't wrap the subsequent file descriptor creation.
The subsequent code checks the permission of the created inode or existing inode and creates a file descriptor. The createFd doesn't actually have to be asynchronous atm. But the expectation is that the inode does exist though.
This has a race condition where it's possible that inode gets deleted before FileDescriptorManager.createFd is called. It's quite unlikely though because no other code knows about that inode index yet. But it is possible because another context might fetch all the inodes that directory.
It seems that would be safer to ensure that the subsequent operations at the end of _open should also be within the new transaction context involving the creation of a new inode.
CMCDragonkai
commented
2 years ago It seems we might need tran.queueFinally to be able to call inoAllocated(navigated) so that way the intermediate entries can be removed.
/**
* Allocates an INodeIndex
* This is a purely functional method
* If passed without the navigated parameter, it assumed that this is for the root INodeIndex
* If passed with the navigated parameter, it will return the same INodeIndex for the same input parameters
* If the inode allocation is unsuccessful, you must deallocate the inode
* Not doing so will result in memory-leak of INodeIndexes
*/
public inoAllocate(navigated?: Readonly<{ dir: INodeIndex, name: string }>): INodeIndex {
let key: string;
if (navigated == null) {
key = ''
} else {
key = navigated.dir + navigated.name;
}
let iNodeIndex = this.iNodeAllocations.get(key);
if (iNodeIndex == null) {
iNodeIndex = this.iNodeCounter.allocate();
this.iNodeAllocations.set(key, iNodeIndex!);
}
return iNodeIndex!;
// return this.iNodeCounter.allocate();
}
/**
* Remove the entry from the inodeAllocations
* If you fail to do so, you should also delete it from the table
*/
public inoAllocated(navigated?: Readonly<{ dir: INodeIndex, name: string }>): void {
let key: string;
if (navigated == null) {
key = ''
} else {
key = navigated.dir + navigated.name;
}
this.iNodeAllocations.delete(key);
}
public inoDeallocate(ino: INodeIndex): void {
return this.iNodeCounter.deallocate(ino);
}Notice that inoDeallocate is not involved here. Instead there is a inoAllocated that finishes by removing the mapping entry from the intermediate data structure.
However this exposes another bug, where the file no longer exists if there are multiple concurrent writeFile.
It doesn't quite make sense, because readdir is showing that the entry does exist.
This may be because navigatedFrom is also breaking up the transaction contexts, and we really need that to be within 1 transaction... I'm not sure if there was a good reason to break up the transaction contexts.
CMCDragonkai
commented
2 years ago Will need to review https://gitlab.com/MatrixAI/Engineering/Polykey/js-encryptedfs/-/merge_requests/46#note_649228698 for reasons for why the transaction contexts were broken up in navigateFrom.
CMCDragonkai
commented
2 years ago I'm thinking the inoAllocate and inoDeallocate can be part of the ResourceAcquire type as well. This way we can compose the inode allocation with lock allocation. Right now, we usually want to lock a given inode, but if we are creating a new inode (on specific name in a particular directory), then we also want to lock on that. I think there might be a way to incorporate this into the withTransactionF, by passing something like an undefined to represent the need for a new inode... This can avoid us having to use tran.queueFailure and tran.queueSuccess.
The main thing is that concurrent operations get the same inode when trying to create the same key. Afterwards, once the inode is created, the entry can removed, as all subsequent operations will find that inode and should not be trying to create the inode.
CMCDragonkai
commented
2 years ago Turns out the fileGetBlocks was buggy. It didn't handle the case of sparse blocks, where the file descriptor is set to a position beyond the end of the file. When returning the zeroed-out blocks, it's important to increment the blockCount otherwise it will result in an infinite loop.
I added a new test read sparse blocks from a file to tests/inodes/INodeManager.file.test.ts to cover this case.
Note that fallocate and ftruncate could create sparse files, but at the moment, the implementation doesn't do this. I believe it just writes zeroed-out blocks, but I haven't looked too deeply.
CMCDragonkai
commented
2 years ago At some point this the O_TRUNC was added in:
flags & (vfs.constants.O_WRONLY | (flags & (vfs.constants.O_RDWR | (flags & vfs.constants.O_TRUNC))))From originally:
flags & (vfs.constants.O_WRONLY | (flags & vfs.constants.O_RDWR))I believe this was a linting change. I'm not sure if it has the same effect.
I think the original is much clearer. And I'd like to test if eslint is the one doing this.
CMCDragonkai
commented
2 years ago I have some success by restructuring the _open.
Note that INodeManager.inoAllocation() is now a method that returns ResourceAcquire<INodeIndex>. This enables futher composition to become withF.
Furthermore although ResourceAcquire is asynchronous. It is actually synchronous internally, there's no await internally. This means it really behaves synchronously because promises are eagerly-evaluated and not lazy-evaluated.
For example:
function doThisSync () {
console.log('called do this sync');
return 1;
}
async function doThis () {
console.log('called do this async');
return 1;
}
async function main () {
const p = doThis();
doThisSync();
await p;
}
main();Running this will show called do this async gets called first before called do this sync. In fact, anything synchronous before the first await in an asynchronous function would be synchronously executed.
This should mean that our mapping to inode indexes should work.
However we then need to check if the inode did in fact get created, and if so, we need to restart the opening procedure. I've done this by using a while loop. But it does involving releasing the transaction and going back to attempting to lock the transaction. This can mean things get out of order since we aren't preserving the same transaction. I'd like to preserve it, but this requires some thinking, but it may just be cosmetic. Further testing required.
CMCDragonkai
commented
2 years ago Additional issue is that the navigateFrom uses alot of transactional contexts. When I changed to bringing them into 1 transactional context, it resulted in not being able to find a particular target. A bit strange. So testing this.
CMCDragonkai
commented
2 years ago And now that we have switched over _open to use inoAllocation, the other methods of EncryptedFS that creates inodes will need to do the same such as mkdir, mknod, symlink.
The navigateFrom also should be restructured to directly use the ResourceAcquire types. Trying to nest itself within withTransactionF becomes problematic because it is a recursive function that sometimes calles this.navigate and this.navigateFrom. It also mutates it's parameters as a sort of reference variables that is used across the recursions. A hold over from the original VFS design.
If we are locking we can only be locking the current directory, and once we navigate out, we need to release our lock.
The _open was non-recursive which was a bit easier, whereas here, we don't actually want to maintain transaction context under the recursive call graph. Therefore as soon as we do a "tail call here" we want to release the transaction for the current directory.
CMCDragonkai
commented
2 years ago TODOs...
_open still doesn't preserve the transaction object once the node is created, once it goes to the top of the loop, the same transaction context should be maintained.navigateFrom so it's using a single transaction context for the directory it's interrogating at 1 recursion level, but subsequent recursions should not hold the transaction context.inoAllocate to using inoAllocations resource context instead. This may need to be done in other situations too like in INodeManager tests, the inoAllocate may not be necessary anymore.mkdir, mknod, _open to see their effects.Additionally note that concurrent calls to writeFile results in interleaving the results. According to Node FS docs, it is "unsafe" to call writeFile concurrently. So something like:
await Promise.all([
fsPromises.writeFile('./tmp/ctest', 'one'),
fsPromises.writeFile('./tmp/ctest', 'two'),
fsPromises.writeFile('./tmp/ctest', 'three'),
fsPromises.writeFile('./tmp/ctest', 'ten'),
]);
console.log(await fsPromises.readFile('./tmp/ctest', { encoding: 'utf-8'}));In fact results in tenee.
EFS now mirrors this behaviour.
Description
Several core libraries have been updated and js-encryptedfs needs to start using them.
@matrixai/async-init- no major changes@matrixai/async-locks- theINodeManageruses a collection ofMutexfor mutual exclusion of inodes. This means operations to individual inodes may be blocked. We can switch this withLock, and in the future optimise withRWLockWriterorRWLockReader@matrixai/db- the DB now supports proper DB transactions, we should switch theINodeManagerAPI to fully support it@matrixai/errors- we make all of our errors extendAbstractError<T>and also provide static descriptions to all of them, as well as use thecausechain@matrixai/workers- no major changes here@matrixai/resources- since the@matrixai/dbno longer does any locking, the acquisition of theDBTransactionandLockhas to be done together withwithForwithGIssues Fixed
Tasks
AbstractErrorand usecausechain and static descriptions@matrixai/dbintoINodeManager, remove sublevel objects and replace with full keypaths.@matrixai/async-locksto use theLockclass instead ofMutexfromasync-mutex@matrixai/resourceswithFandwithGto replace some of ourtransactAPI.FileDescriptor.writeandFileDescriptor.readshould be using a single transaction context for the entire operation: https://github.com/MatrixAI/js-encryptedfs/pull/63#issuecomment-1094910309tests/inodesEncryptedFS.ftruncateso that it does not need to iterate over the entire file blocks before truncating, this targets #53~ - do this in #53fileGetBlocksand see if it correctly implemented - infinite loop solved by incrementing the block count inside the while loop, and added new test toINodeManager.file.test.tsread sparse blocks from a fileto cover thistests/utils.ts#expectErrorhas been changed to take the exception class as well, and then theerrnoobject to be checkedftruncateFileDescriptorhaving its own lock. This prevents some stream concurrency errorsnavigateFromseems racy with multiple transaction contexts being used_openseems racy with the final FD creation with respect to inode creationmkdirseems to have strange behavour when the target already exists, need to compare behaviour with real filesystemmkdirFinal checklist