Direct IO #133

zvin · 2020-03-27T18:27:41Z

Major change

Only use aligned buffers to write to block devices.
It means all read streams read into aligned buffers when the target is a block device.
When we don't control the read stream, use a transform that will copy its buffers to aligned buffers.
Each read stream uses a pool of 1 MiB buffers to minimize allocations.

This should improve speed and reduce cpu usage.

Change-type: patch

Change-type: major

Change-type: patch

package.json

lib/source-destination/file.ts

lib/fs.ts

lib/lockable-buffer.ts

lib/aligned-lockable-buffer.ts

lib/block-transform-stream.ts

lib/source-destination/multi-destination.ts

Change-type: major

Page- · 2020-03-30T16:48:45Z

lib/sparse-stream/sparse-write-stream.ts

-				this.bytesWritten += chunk.buffer.length;
+		const unlock = isAlignedLockableBuffer(chunk.buffer)
+			? await chunk.buffer.rlock()
+			: noop;


You could also make this

Suggested change

: noop;

: undefined;

and use unlock?.() below if you want

Page- · 2020-03-30T16:50:21Z

lib/sparse-stream/sparse-transform-stream.ts

+		this.push({ position: chunk.position, buffer });
+		this.bytesWritten += chunk.buffer.length;
+		this.position += chunk.buffer.length;
+		callback();


This callback isn't guaranteed to be called if await buffer.lock() fails - is that a case that should be handled?

yes it should be handled, updated

Page- · 2020-03-30T16:51:11Z

lib/sparse-stream/sparse-read-stream.ts

+			if (this.state.hasher !== undefined) {
+				this.state.hasher.update(buffer);
+			}


This can be

Suggested change

if (this.state.hasher !== undefined) {

this.state.hasher.update(buffer);

}

this.state.hasher?.update(buffer);

if you want

Page- · 2020-03-30T16:51:49Z

lib/sparse-stream/sparse-read-stream.ts

+			this.alignedReadableState !== undefined
+				? this.alignedReadableState.getCurrentBuffer().slice(0, length)
+				: Buffer.allocUnsafe(length);
+		const unlock = isAlignedLockableBuffer(buffer) ? await buffer.lock() : noop;


This could also be

Suggested change

const unlock = isAlignedLockableBuffer(buffer) ? await buffer.lock() : noop;

const unlock = isAlignedLockableBuffer(buffer) ? await buffer.lock() : undefined;

and unlock?.()

Page- · 2020-03-30T16:57:24Z

lib/source-destination/multi-destination.ts

-	public async createSparseWriteStream(): Promise<SparseWritable> {
-		return await this.createStream('createSparseWriteStream');
+	public async createSparseWriteStream(
+		...args: any[]


I believe this can be properly typed using

Suggested change

...args: any[]

...args: Parameters<SourceDestination['createSparseWriteStream']>

Page- · 2020-03-30T16:57:29Z

lib/source-destination/multi-destination.ts

-	public async createWriteStream(): Promise<NodeJS.WritableStream> {
-		return await this.createStream('createWriteStream');
+	public async createWriteStream(
+		...args: any[]


I believe this can be properly typed using

Suggested change

...args: any[]

...args: Parameters<SourceDestination['createWriteStream']>

nice, thanks

lib/source-destination/dmg.ts

Page- · 2020-03-30T17:03:59Z

lib/block-transform-stream.ts

-	public _flush(callback: (error?: Error) => void) {
-		this.writeBuffers(true);
-		callback();
+	public async _flush(callback: (error?: Error) => void): Promise<void> {


This returns an immediately-resolved promise, is that intended or should this be

Suggested change

public async _flush(callback: (error?: Error) => void): Promise<void> {

public _flush(callback: (error?: Error) => void): void {

or have an await?

right, fixed

Page- · 2020-03-30T17:04:52Z

lib/block-transform-stream.ts

 		chunk: Buffer,
 		_encoding: string,
 		callback: (error?: Error) => void,
-	) {
+	): Promise<void> {


This is also an immediately-resolved promise

Page- · 2020-03-30T17:05:38Z

lib/block-read-stream.ts

 			const { bytesRead } = await this.tryRead(buffer);
-			if (bytesRead === 0) {
+			unlock();


Should this be in a finally block?

Page- · 2020-03-31T12:25:55Z

lib/sparse-stream/shared.ts

@@ -55,7 +55,7 @@ export interface SparseWritable extends NodeJS.WritableStream {
 	_write(
 		chunk: SparseStreamChunk,
 		encoding: string,
-		callback: (err?: Error | void) => void,
+		callback: (err?: Error | void | null) => void,


I think this should be

Suggested change

callback: (err?: Error | void | null) => void,

callback: (err?: Error | null) => void,

as void doesn't really make sense as a parameter value

Page- · 2020-03-31T12:27:10Z

lib/utils.ts

@@ -84,7 +84,7 @@ export function difference<T>(setA: Set<T>, setB: Set<T>): Set<T> {

 export async function asCallback<T>(
 	promise: Promise<T>,
-	callback: (error: Error | void, value?: T) => void,
+	callback: (error: Error | void | null, value?: T) => void,


Another place to drop void

Suggested change

callback: (error: Error | void | null, value?: T) => void,

callback: (error: Error | null, value?: T) => void,

tests/zip.spec.ts

tsconfig.json

Change-type: major

Page- · 2020-03-31T12:49:25Z

lib/utils.ts

+				const unlock = await chunk.buffer.rlock();
+				const data = Buffer.allocUnsafe(chunk.buffer.length);
+				chunk.buffer.copy(data);
+				unlock();


Should this be in a try/finally?

good catch, added

Page- · 2020-03-31T12:50:13Z

lib/utils.ts

-		stream.on('data', chunks.push.bind(chunks));
+		stream.on('data', async (chunk: SparseStreamChunk) => {
+			if (isAlignedLockableBuffer(chunk.buffer)) {
+				const unlock = await chunk.buffer.rlock();


If this lock fails (can it fail?) then the chunk will be silently lost

Right, this function is only used in tests, fixed it anyway.

Page- · 2020-03-31T12:50:33Z

lib/utils.ts

+					const unlock = await chunk.rlock();
+					data = Buffer.allocUnsafe(chunk.length);
+					chunk.copy(data);
+					unlock();


Same concerns here about try/catch/finally

Page- · 2020-03-31T12:54:58Z

lib/source-destination/multi-destination.ts

 	private async createStream(
 		methodName: 'createWriteStream' | 'createSparseWriteStream',
+		...args: any[]
 	) {


If you want 100% typing you can do:

Suggested change

private async createStream(

methodName: 'createWriteStream' | 'createSparseWriteStream',

...args: any[]

) {

private async createStream(

methodName: 'createWriteStream',

...args: Parameters<SourceDestination['createWriteStream']>

) {

private async createStream(

methodName: 'createSparseWriteStream',

...args: Parameters<SourceDestination['createSparseWriteStream']>

) {

private async createStream(

methodName: 'createWriteStream' | 'createSparseWriteStream',

...args: Parameters<SourceDestination['createWriteStream']> | Parameters<SourceDestination['createSparseWriteStream']>

) {

although I'm also OK with skipping it as it's a private method

added, thanks

Page- · 2020-03-31T12:58:49Z

lib/block-transform-stream.ts

-			let block = Buffer.concat(this._buffers);
+	private async writeBuffers(flush = false): Promise<void> {
+		if (flush || this.inputBytes >= this.chunkSize) {
+			// TODO optimize


fwiw node 12.16.2 is slated to include buffer.concat performance improvements: nodejs/node#32313 / nodejs/node#31522

I think we can probably completely avoid Buffer.concat, but that'll make it more complex to read.

Page- · 2020-03-31T13:00:22Z

lib/block-read-stream.ts

-				this.push(null);
-				return;
+			const unlock = await buffer.lock();
+			let bytesRead: number;


Typescript should be able to infer the type if you want:

Suggested change

let bytesRead: number;

let bytesRead;

but I'm not fussed either way

Change-type: patch

Change-type: major

Change-type: patch

zvin added 6 commits March 11, 2020 12:10

Don't use .then in asCallback implementation

8c48d52

Change-type: patch

Use known buffers size when concatenating buffers

9dd3536

Change-type: patch

Drop node 6 support

a0be4f2

Change-type: major

LockableBuffer class

de87be5

Use aligned buffers to read and write block devices with O_DIRECT

1a7a17c

Change-type: major

Update generated docs

554b213

Change-type: patch

zvin requested review from Page- and thundron March 27, 2020 18:28

zvin changed the title ~~Directio~~ Direct IO Mar 27, 2020

Page- reviewed Mar 27, 2020

View reviewed changes

zvin added 2 commits March 30, 2020 17:46

Address comments

8c46ec7

BlockDeviceAdapter constructor takes an options argument

f7d318f

Change-type: major

Page- reviewed Mar 30, 2020

View reviewed changes

Page- reviewed Mar 31, 2020

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

Drop node8 support

fd66806

Change-type: major

zvin force-pushed the directio branch from d2475c6 to fd66806 Compare March 31, 2020 12:38

Address PR comments

4276cf5

Page- reviewed Mar 31, 2020

View reviewed changes

zvin added 4 commits March 31, 2020 15:30

Address PR comments

d0053b9

Update blockmap to ^4.0.1

de08262

Change-type: patch

File constructor takes an options object

cc79d78

Change-type: major

BlockDevice constructor takes an options argument

56ccb38

Change-type: major

zvin force-pushed the directio branch from a216481 to 56ccb38 Compare April 1, 2020 11:19

zvin added 5 commits April 1, 2020 13:35

ConfiguredSource constructor takes an options argument

0478382

Change-type: major

Update udif to ^0.17.0

16016bb

Change-type: patch

Update lzma-native to ^6.0.0

4133b2f

Change-type: patch

Stop using readable-stream

436a5af

Change-type: patch

Remove unneeded @ts-ignore comment

7c588aa

Page- approved these changes Apr 1, 2020

View reviewed changes

zvin force-pushed the directio branch from 10d399c to b7d24d2 Compare April 7, 2020 13:53

zvin added 2 commits April 7, 2020 16:22

Retry incomplete reads

f021668

Change-type: patch

Don't use O_EXLOCK on Windows

da2fa24

Change-type: patch

zvin force-pushed the directio branch from b7d24d2 to a52b7d5 Compare April 7, 2020 14:24

zvin added 2 commits April 7, 2020 17:26

Use setF_NOCACHE on macos for block devices

c6e0990

Change-type: patch

Update generated docs

3d0724a

Change-type: patch

zvin force-pushed the directio branch from a52b7d5 to 3d0724a Compare April 7, 2020 15:28

zvin merged commit 9960a67 into master Apr 7, 2020

zvin deleted the directio branch April 7, 2020 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direct IO #133

Direct IO #133

zvin commented Mar 27, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 30, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

Page- Mar 31, 2020

zvin Mar 31, 2020

	const unlock = isAlignedLockableBuffer(buffer) ? await buffer.lock() : noop;
	const unlock = isAlignedLockableBuffer(buffer) ? await buffer.lock() : undefined;

	...args: any[]
	...args: Parameters<SourceDestination['createSparseWriteStream']>

	public async _flush(callback: (error?: Error) => void): Promise<void> {
	public _flush(callback: (error?: Error) => void): void {

	callback: (err?: Error \| void \| null) => void,
	callback: (err?: Error \| null) => void,

	callback: (error: Error \| void \| null, value?: T) => void,
	callback: (error: Error \| null, value?: T) => void,

Direct IO #133

Direct IO #133

Conversation

zvin commented Mar 27, 2020

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

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

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

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

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