pipe: implement readv and writev #91
Conversation
|
Hi, thanks for the patches! I'm leaving comments in the individual commits, but in general: LGTM, but you should restructure things differently. For instance: |
| @@ -415,7 +416,7 @@ ssize_t pipe::append_iter(iovec_iter *iter, bool atomic) | |||
| // See if we have space in this pipe buffer | |||
| // TODO: Idea to test: memmove data back if we have offset != 0 | |||
| // May compact things a bit. | |||
| if (PAGE_SIZE - last_buf->len_ <= len) | |||
| if (PAGE_SIZE - last_buf->len_ <= iter->bytes) | |||
There was a problem hiding this comment.
This function has typos and tiny bugs which should be fixed up and squashed with the previous commit
| @@ -451,7 +452,7 @@ ssize_t pipe::append_iter(iovec_iter *iter, bool atomic) | |||
| } | |||
| } | |||
|
|
|||
| page *p = cached_page | |||
| page *p = cached_page; | |||
kernel/kernel/fs/pipe.cpp
Outdated
|
|
||
| if (wait_for_event_mutex_interruptible( | ||
| &write_queue, | ||
| (is_atomic_write && available_space() >= (iter->bytes - ret)) || !is_full() || |
There was a problem hiding this comment.
Hmm. I know that the ->write code doesn't handle this. But I think this could be slightly simplified to
(is_atomic_write && available_space() >= iter->bytes) || !is_full() ||
because is_atomic_write means (hopefully) that we're not going to do a partial write, and ret is used to know how much we've written from to the pipe (for partial writes). I think this is correct, but I haven't looked at this code for a while. Thoughts?
There was a problem hiding this comment.
Yes, this should be correct, since iter->bytes shows amount of unwritten bytes
petershh
force-pushed
the
pipe-iter
branch
2 times, most recently
from
86bc000 to
7fb6508
Compare
kernel/kernel/fs/pipe.cpp
Outdated
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright (c) 2017 - 2023 Pedro Falcato | |||
| * Copyright (c) 2017 - 2024 Pedro Falcato | |||
There was a problem hiding this comment.
I didn't actually touch this this time around, so whether you want to add your name or not is fine for me. But this isn't :)
kernel/kernel/fs/pipe.cpp
Outdated
| while (!iter->empty()) | ||
| { | ||
| iovec iov = iter->curiovec(); | ||
| if (iov.iov_len == 0) |
There was a problem hiding this comment.
I'm pretty sure iovec_iter already deals with these iov_len = 0 cases. Did you find an edge case? Do these lines fix anything?
There was a problem hiding this comment.
This is older code which was written without clear understanding of what should be done. Will remove
kernel/kernel/fs/pipe.cpp
Outdated
|
|
||
| ssize_t copied = copy_to_iter(iter, page_buf, pbf->len_); | ||
|
|
||
| if (copied < 0) { |
There was a problem hiding this comment.
Wrong brace style, see the rest of the code (and use clang-format!)
kernel/kernel/fs/pipe.cpp
Outdated
| curr_len -= copied; | ||
| ret += copied; | ||
|
|
||
| if (!can_read()) { |
Signed-off-by: Peter Shkenev <[email protected]>
Signed-off-by: Peter Shkenev <[email protected]>
|
Merged, thanks! |
Now these use pipe::read_iter and pipe::write_iter and don't block when they shouldn't