Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packages: add general io utils #17

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

Vicente-Cheng
Copy link
Collaborator

It will be used for general volume clones.

@Vicente-Cheng Vicente-Cheng force-pushed the add-io-util branch 3 times, most recently from 97e77c6 to edc9fd7 Compare July 18, 2024 09:39
@Vicente-Cheng Vicente-Cheng marked this pull request as ready for review July 21, 2024 03:58
@Vicente-Cheng Vicente-Cheng marked this pull request as draft July 22, 2024 02:53
@Vicente-Cheng Vicente-Cheng marked this pull request as ready for review July 22, 2024 03:52
@Vicente-Cheng Vicente-Cheng force-pushed the add-io-util branch 2 times, most recently from 0bcf9b1 to 2598c88 Compare July 23, 2024 03:31
Copy link
Member

@WebberHuang1118 WebberHuang1118 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Vicente-Cheng

Please take a look, and I will help to double-check the alignment issue you mentioned to me, thanks.

io/io.go Outdated Show resolved Hide resolved
io/io.go Outdated
Comment on lines 92 to 98
_, err := PReadExact(src, buf, chunkSize, uint64(i*chunkSize))
if err != nil {
fmt.Printf("Error reading data: %v\n", err)
ioError = err
continue
}
objs <- Content{offset: uint64(i * chunkSize), buf: buf}
Copy link
Member

@WebberHuang1118 WebberHuang1118 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have continuous direct reads in a single go routine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate more on your concerns about the single go routine (single producer) instead of the multi-producer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean to only have a single producer. IIUC, We will have 8 (from producerNum) producers, and in each producer, the read address is not continuous. For example, the first producer will read the address of chunk numbers 0, 8, 16, 24 ... so on. It could be just a nit for non-rotational devices. I'm ok with the current design, just a discussion. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Now, each producer will handle the continuous offset to read.

io/io.go Outdated Show resolved Hide resolved
@Vicente-Cheng Vicente-Cheng force-pushed the add-io-util branch 2 times, most recently from df1fa6b to 79007d3 Compare July 23, 2024 16:01
@Vicente-Cheng
Copy link
Collaborator Author

Please take a look, and I will help to double-check the alignment issue you mentioned to me, thanks.

Thanks @WebberHuang1118.
I did some checks and benchmarks for the posix_memalign. It seems we can fix the alignment size to 4k.
That would be more efficient to allocate the single alignment address on heap.

So, I dropped the dynamic alignment size when calling posix_memalign, which would also simplify the code.

io/io.go Outdated Show resolved Hide resolved
io/io.go Outdated Show resolved Hide resolved
@WebberHuang1118 WebberHuang1118 self-requested a review July 26, 2024 03:44
@Vicente-Cheng Vicente-Cheng force-pushed the add-io-util branch 3 times, most recently from 22df88a to 47189b1 Compare August 1, 2024 01:51
Copy link
Member

@WebberHuang1118 WebberHuang1118 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm just wondering if the wait group and channel handling are a little complex, however, I'm not sure if there is a simpler way to deal with this. I will try to figure it out and discuss it with you if I find one, thanks.

Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

    - suppport parallel ioReader/ioWriter
    - skip the zero buffer

Signed-off-by: Vicente Cheng <[email protected]>
@bk201 bk201 merged commit bdbb8cc into harvester:main Aug 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants