From: Fabiano Rosas <farosas@suse.de>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org, berrange@redhat.com, armbru@redhat.com,
Claudio Fontana <cfontana@suse.de>, Jim Fehlig <jfehlig@suse.com>
Subject: Re: [PATCH 7/9] monitor: fdset: Match against O_DIRECT
Date: Fri, 03 May 2024 18:19:30 -0300 [thread overview]
Message-ID: <87seyymw4d.fsf@suse.de> (raw)
In-Reply-To: <ZjUyta-q8GuTDDjJ@x1n>
Peter Xu <peterx@redhat.com> writes:
> On Fri, Apr 26, 2024 at 11:20:40AM -0300, Fabiano Rosas wrote:
>> We're about to enable the use of O_DIRECT in the migration code and
>> due to the alignment restrictions imposed by filesystems we need to
>> make sure the flag is only used when doing aligned IO.
>>
>> The migration will do parallel IO to different regions of a file, so
>> we need to use more than one file descriptor. Those cannot be obtained
>> by duplicating (dup()) since duplicated file descriptors share the
>> file status flags, including O_DIRECT. If one migration channel does
>> unaligned IO while another sets O_DIRECT to do aligned IO, the
>> filesystem would fail the unaligned operation.
>>
>> The add-fd QMP command along with the fdset code are specifically
>> designed to allow the user to pass a set of file descriptors with
>> different access flags into QEMU to be later fetched by code that
>> needs to alternate between those flags when doing IO.
>>
>> Extend the fdset matching to behave the same with the O_DIRECT flag.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> monitor/fds.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor/fds.c b/monitor/fds.c
>> index 4ec3b7eea9..62e324fcec 100644
>> --- a/monitor/fds.c
>> +++ b/monitor/fds.c
>> @@ -420,6 +420,11 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>> int fd = -1;
>> int dup_fd;
>> int mon_fd_flags;
>> + int mask = O_ACCMODE;
>> +
>> +#ifdef O_DIRECT
>> + mask |= O_DIRECT;
>> +#endif
>>
>> if (mon_fdset->id != fdset_id) {
>> continue;
>> @@ -431,7 +436,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>> return -1;
>> }
>>
>> - if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
>> + if ((flags & mask) == (mon_fd_flags & mask)) {
>> fd = mon_fdset_fd->fd;
>> break;
>> }
>
> I think I see what you wanted to do, picking out the right fd out of two
> when qemu_open_old(), which makes sense.
>
> However what happens if the mgmt app only passes in 1 fd to the fdset? The
> issue is we have a "fallback dup()" plan right after this chunk of code:
>
I'm validating the fdset at file_parse_fdset() beforehand. If there's
anything else than 2 fds then we'll error out:
if (nfds != 2) {
error_setg(errp, "Outgoing migration needs two fds in the fdset, "
"got %d", nfds);
qmp_remove_fd(*id, false, -1, NULL);
*id = -1;
return false;
}
> dup_fd = qemu_dup_flags(fd, flags);
> if (dup_fd == -1) {
> return -1;
> }
>
> mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
> mon_fdset_fd_dup->fd = dup_fd;
> QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
>
> I think it means even if the mgmt app only passes in 1 fd (rather than 2,
> one with O_DIRECT, one without), QEMU can always successfully call
> qemu_open_old() twice for each case, even though silently the two FDs will
> actually impact on each other. This doesn't look ideal if it's true.
>
> But I also must confess I don't really understand this code at all: we
> dup(), then we try F_SETFL on all the possible flags got passed in.
> However AFAICT due to the fact that dup()ed FDs will share "struct file" it
> means mostly all flags will be shared, except close-on-exec. I don't ever
> see anything protecting that F_SETFL to only touch close-on-exec, I think
> it means it'll silently change file status flags for the other fd which we
> dup()ed from. Does it mean that we have issue already with such dup() usage?
I think you're right, but I also think there's a requirement even from
this code that the fds in the fdset cannot be dup()ed. I don't see it
enforced anywhere, but maybe that's a consequence of the larger use-case
for which this feature was introduced.
For our scenario, the open() man page says one can use kcmp() to compare
the fds and determine if they are a result of dup(). Maybe we should do
that extra check? We're defining a pretty rigid interface between QEMU
and the management layer, so not likely to break once it's written. I'm
also not sure how bad would it be to call syscall() directly from QEMU
(kcmp has no libc wrapper).
>
> Thanks,
next prev parent reply other threads:[~2024-05-03 21:19 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 14:20 [PATCH 0/9] migration/mapped-ram: Add direct-io support Fabiano Rosas
2024-04-26 14:20 ` [PATCH 1/9] monitor: Honor QMP request for fd removal immediately Fabiano Rosas
2024-05-03 16:02 ` Peter Xu
2024-05-16 21:46 ` Fabiano Rosas
2024-05-08 7:17 ` Daniel P. Berrangé
2024-05-16 22:00 ` Fabiano Rosas
2024-05-17 7:33 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 2/9] migration: Fix file migration with fdset Fabiano Rosas
2024-05-03 16:23 ` Peter Xu
2024-05-03 19:56 ` Fabiano Rosas
2024-05-03 21:04 ` Peter Xu
2024-05-03 21:31 ` Fabiano Rosas
2024-05-03 21:56 ` Peter Xu
2024-05-08 8:02 ` Daniel P. Berrangé
2024-05-08 12:49 ` Peter Xu
2024-05-08 8:00 ` Daniel P. Berrangé
2024-05-08 20:45 ` Fabiano Rosas
2024-04-26 14:20 ` [PATCH 3/9] tests/qtest/migration: Fix file migration offset check Fabiano Rosas
2024-05-03 16:47 ` Peter Xu
2024-05-03 20:36 ` Fabiano Rosas
2024-05-03 21:08 ` Peter Xu
2024-05-08 8:10 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 4/9] migration: Add direct-io parameter Fabiano Rosas
2024-04-26 14:33 ` Markus Armbruster
2024-05-03 18:05 ` Peter Xu
2024-05-03 20:49 ` Fabiano Rosas
2024-05-03 21:16 ` Peter Xu
2024-05-14 14:10 ` Markus Armbruster
2024-05-14 17:57 ` Fabiano Rosas
2024-05-15 7:17 ` Markus Armbruster
2024-05-15 12:51 ` Fabiano Rosas
2024-05-08 8:25 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 5/9] migration/multifd: Add direct-io support Fabiano Rosas
2024-05-03 18:29 ` Peter Xu
2024-05-03 20:54 ` Fabiano Rosas
2024-05-03 21:18 ` Peter Xu
2024-05-08 8:27 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 6/9] tests/qtest/migration: Add tests for file migration with direct-io Fabiano Rosas
2024-05-03 18:38 ` Peter Xu
2024-05-03 21:05 ` Fabiano Rosas
2024-05-03 21:25 ` Peter Xu
2024-05-08 8:34 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 7/9] monitor: fdset: Match against O_DIRECT Fabiano Rosas
2024-05-03 18:53 ` Peter Xu
2024-05-03 21:19 ` Fabiano Rosas [this message]
2024-05-03 22:16 ` Peter Xu
2024-04-26 14:20 ` [PATCH 8/9] migration: Add support for fdset with multifd + file Fabiano Rosas
2024-05-08 8:53 ` Daniel P. Berrangé
2024-05-08 18:23 ` Peter Xu
2024-05-08 20:39 ` Fabiano Rosas
2024-05-09 8:08 ` Daniel P. Berrangé
2024-05-17 22:43 ` Fabiano Rosas
2024-05-18 8:36 ` Daniel P. Berrangé
2024-04-26 14:20 ` [PATCH 9/9] tests/qtest/migration: Add a test for mapped-ram with passing of fds Fabiano Rosas
2024-05-08 8:56 ` Daniel P. Berrangé
2024-05-02 20:01 ` [PATCH 0/9] migration/mapped-ram: Add direct-io support Peter Xu
2024-05-02 20:34 ` Fabiano Rosas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87seyymw4d.fsf@suse.de \
--to=farosas@suse.de \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=cfontana@suse.de \
--cc=jfehlig@suse.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).