Re: [PATCH v2 2/3] block: io_uring: add READV_PI/WRITEV_PI operations
From: Alexander V. Buev <hidden>
Date: 2022-02-11 09:40:03
Also in:
io-uring
On 2/10/22 12:03 PM, Alexander V. Buev wrote:quoted
quoted
On 2/10/22 6:08 AM, Alexander V. Buev wrote:quoted
Added new READV_PI/WRITEV_PI operations to io_uring. Added new pi_addr & pi_len fields to SQE struct. Added new pi_iter field and IOCB_USE_PI flag to kiocb struct. Make corresponding corrections to io uring trace event. +struct io_rw_pi_state { + struct iov_iter iter; + struct iov_iter_state iter_state; + struct iovec fast_iov[UIO_FASTIOV_PI]; +}; + +struct io_rw_pi { + struct io_rw rw; + struct iovec *pi_iov; + u32 nr_pi_segs; + struct io_rw_pi_state *s; +};One immediate issue I see here is that io_rw_pi is big, and we try very hard to keep the per-command payload to 64-bytes. This would be 88 bytes by my count :-/ Do you need everything from io_rw? If not, I'd just make io_rw_pi contain the bits you need and see if you can squeeze it into the existing cacheline.In short - Yes. Current patch code call existing io_read/io_write functions. This functions use io_rw struct information and process this data. I wanted to use existing functions but may be this is wrong way in this case. The second problem with request size is that the patch adds pi_iter pointer to kiocb struct. This also increase whole request union length. So I can see some (may be possible) solution for this: 1) do not store whole kiocb struct in request and write fully separated io_read/write_pi functions 2) make special CONFIG_XXX variable and simplify hide this code as defaultOption 2 really sucks, because then obviously everyone wants their feature enabled, and then we are back to square one. So never rely on a config option, if it can be avoided. I'd like to see what option 1 looks like, that sounds like a far better solution.
Accepted. I am starting to prepare v3 in this way. Thanks to all for feedback! -- Alexander Buev