Peter Xu <pet...@redhat.com> writes:
> On Mon, Jun 10, 2024 at 02:45:53PM -0300, Fabiano Rosas wrote:
>> >> AIUI, the issue here that users are already allowed to specify in
>> >> libvirt the equivalent to direct-io and multifd independent of each
>> >> other (bypass-cache, parallel). To start requiring both together now in
>> >> some situations would be a regression. I confess I don't know libvirt
>> >> code to know whether this can be worked around somehow, but as I said,
>> >> it's a relatively simple change from the QEMU side.
>> >
>> > Firstly, I definitely want to already avoid all the calls to either
>> > migration_direct_io_start() or *_finish(), now we already need to
>> > explicitly call them in three paths, and that's not intuitive and less
>> > readable, just like the hard coded rdma codes.
>>
>> Right, but that's just a side-effect of how the code is structured and
>> the fact that writes to the stream happen in small chunks. Setting
>> O_DIRECT needs to happen around aligned IO. We could move the calls
>> further down into qemu_put_buffer_at(), but that would be four fcntl()
>> calls for every page.
>
> Hmm.. why we need four fcntl()s instead of two?
Because we need to first get the flags before flipping the O_DIRECT
bit. And we do this once to enable and once to disable.
int flags = fcntl(fioc->fd, F_GETFL);
if (enabled) {
flags |= O_DIRECT;
} else {
flags &= ~O_DIRECT;
}
fcntl(fioc->fd, F_SETFL, flags);
>>
>> A tangent:
>> one thing that occured to me now is that we may be able to restrict
>> calls to qemu_fflush() to internal code like add_to_iovec() and maybe
>> use that function to gather the correct amount of data before writing,
>> making sure it disables O_DIRECT in case alignment is about to be
>> broken?
>
> IIUC dio doesn't require alignment if we don't care about perf? I meant it
> should be legal to write(fd, buffer, 5) even if O_DIRECT?
No, we may get an -EINVAL. See Daniel's reply.
>
> I just noticed the asserts you added in previous patch, I think that's
> better indeed, but still I'm wondering whether we can avoid enabling it on
> qemufile.
>
> It makes me feel slightly nervous when introducing dio to QEMUFile rather
> than iochannels - the API design of QEMUFile seems to easily encourage
> breaking things in dio worlds with a default and static buffering. And if
> we're going to blacklist most of the API anyway except the new one for
> mapped-ram, I start to wonder too why bother on top of QEMUFile anyway.
>
> IIRC you also mentioned in the previous doc patch so that libvirt should
> always pass in two fds anyway to the fdset if dio is enabled. I wonder
> whether it's also true for multifd=off and directio=on, then would it be
> possible to use the dio for guest pages with one fd, while keeping the
> normal stream to use !dio with the other fd. I'm not sure whether it's
> easy to avoid qemufile in the dio fd, even if not looks like we may avoid
> frequent fcntl()s?
Hm, sounds like a good idea. We'd need a place to put that new ioc
though. Either QEMUFile.direct_ioc and then make use of it in
qemu_put_buffer_at() or a more transparent QIOChannelFile.direct_fd that
gets set somewhere during file_start_outgoing_migration(). Let me try to
come up with something.
