| Message ID | 20240220224138.24759-33-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: File based migration with multifd and fixed-ram | expand |
Fabiano Rosas <farosas@suse.de> writes: > 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 function to behave the same with the > O_DIRECT flag. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > monitor/fds.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/monitor/fds.c b/monitor/fds.c > index 9a28e4b72b..42bf3eb982 100644 > --- a/monitor/fds.c > +++ b/monitor/fds.c > @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int fd_flags) static bool monitor_fdset_flags_match(int flags, int fd_flags) { bool match = false; > if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) { > match = true; > + > +#ifdef O_DIRECT > + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { > + match = false; > + } > +#endif > } > > return match; } I'd prefer something like static bool monitor_fdset_flags_match(int flags, int fd_flags) { #ifdef O_DIRECT if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { return false; } #endif if ((flags & O_ACCMODE) != (fd_flags & O_ACCMODE)) { return false; } return true; }
Markus Armbruster <armbru@redhat.com> writes: > Fabiano Rosas <farosas@suse.de> writes: > >> 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 function to behave the same with the >> O_DIRECT flag. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> monitor/fds.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/monitor/fds.c b/monitor/fds.c >> index 9a28e4b72b..42bf3eb982 100644 >> --- a/monitor/fds.c >> +++ b/monitor/fds.c >> @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int fd_flags) > static bool monitor_fdset_flags_match(int flags, int fd_flags) > { > bool match = false; > >> if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) { >> match = true; >> + >> +#ifdef O_DIRECT >> + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { >> + match = false; >> + } >> +#endif >> } >> >> return match; > } > > I'd prefer something like > > static bool monitor_fdset_flags_match(int flags, int fd_flags) > { > #ifdef O_DIRECT > if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { > return false; > } > #endif > > if ((flags & O_ACCMODE) != (fd_flags & O_ACCMODE)) { > return false; > > } > > return true; > } This makes the O_DIRECT flag dictate the outcome when it's present. I want O_DIRECT to be considered only when all other flags have matched. Otherwise we regress the original use-case if the user happened to have put O_DIRECT in the flags. A non-match due to different O_ACCMODE would become a match due to (possibly) matching O_DIRECT.
Fabiano Rosas <farosas@suse.de> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Fabiano Rosas <farosas@suse.de> writes: >> >>> 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 function to behave the same with the >>> O_DIRECT flag. >>> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>> --- >>> monitor/fds.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/monitor/fds.c b/monitor/fds.c >>> index 9a28e4b72b..42bf3eb982 100644 >>> --- a/monitor/fds.c >>> +++ b/monitor/fds.c >>> @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int fd_flags) >> static bool monitor_fdset_flags_match(int flags, int fd_flags) >> { >> bool match = false; >> >>> if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) { >>> match = true; >>> + >>> +#ifdef O_DIRECT >>> + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { >>> + match = false; >>> + } >>> +#endif >>> } >>> >>> return match; >> } >> >> I'd prefer something like >> >> static bool monitor_fdset_flags_match(int flags, int fd_flags) >> { >> #ifdef O_DIRECT >> if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { >> return false; >> } >> #endif >> >> if ((flags & O_ACCMODE) != (fd_flags & O_ACCMODE)) { >> return false; >> >> } >> >> return true; >> } > > This makes the O_DIRECT flag dictate the outcome when it's present. I > want O_DIRECT to be considered only when all other flags have matched. > > Otherwise we regress the original use-case if the user happened to have > put O_DIRECT in the flags. A non-match due to different O_ACCMODE would > become a match due to (possibly) matching O_DIRECT. The fact that I missed this signifies one of two things: either was suffering from code review brain (quite possible!), or this needs a comment and/or clearer coding. If I understand you correctly, you want to return true when the bits selected by the two masks together match. If we didn't need ifdeffery, we wouldn't use nested conditionals for comparing bits under a mask. We'd use something like int mask = O_ACCMODE | O_DIRECT; return (flags & mask) == (fd_flags & mask); Bring back the ifdeffery: int mask = O_ACCMODE; #ifdef O_DIRECT mask |= O_DIRECT; #endif return (flags & mask) == (fd_flags & mask); Or maybe even #ifndef O_DIRECT #define O_DIRECT 0 #endif int mask = O_ACCMODE | O_DIRECT; return (flags & mask) == (fd_flags & mask); Not sure this is even worth a helper function. Or am I stull suffering from code review brain?
Markus Armbruster <armbru@redhat.com> writes: > Fabiano Rosas <farosas@suse.de> writes: > >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Fabiano Rosas <farosas@suse.de> writes: >>> >>>> 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 function to behave the same with the >>>> O_DIRECT flag. >>>> >>>> Signed-off-by: Fabiano Rosas <farosas@suse.de> >>>> --- >>>> monitor/fds.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/monitor/fds.c b/monitor/fds.c >>>> index 9a28e4b72b..42bf3eb982 100644 >>>> --- a/monitor/fds.c >>>> +++ b/monitor/fds.c >>>> @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int fd_flags) >>> static bool monitor_fdset_flags_match(int flags, int fd_flags) >>> { >>> bool match = false; >>> >>>> if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) { >>>> match = true; >>>> + >>>> +#ifdef O_DIRECT >>>> + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { >>>> + match = false; >>>> + } >>>> +#endif >>>> } >>>> >>>> return match; >>> } >>> >>> I'd prefer something like >>> >>> static bool monitor_fdset_flags_match(int flags, int fd_flags) >>> { >>> #ifdef O_DIRECT >>> if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { >>> return false; >>> } >>> #endif >>> >>> if ((flags & O_ACCMODE) != (fd_flags & O_ACCMODE)) { >>> return false; >>> >>> } >>> >>> return true; >>> } >> >> This makes the O_DIRECT flag dictate the outcome when it's present. I >> want O_DIRECT to be considered only when all other flags have matched. >> >> Otherwise we regress the original use-case if the user happened to have >> put O_DIRECT in the flags. A non-match due to different O_ACCMODE would >> become a match due to (possibly) matching O_DIRECT. > > The fact that I missed this signifies one of two things: either was > suffering from code review brain (quite possible!), or this needs a > comment and/or clearer coding. > > If I understand you correctly, you want to return true when the bits > selected by the two masks together match. > > If we didn't need ifdeffery, we wouldn't use nested conditionals for > comparing bits under a mask. We'd use something like > > int mask = O_ACCMODE | O_DIRECT; > > return (flags & mask) == (fd_flags & mask); > > Bring back the ifdeffery: > > int mask = O_ACCMODE; > > #ifdef O_DIRECT > mask |= O_DIRECT; > #endif > > return (flags & mask) == (fd_flags & mask); Could be. I'll change it. > > Or maybe even > > #ifndef O_DIRECT > #define O_DIRECT 0 > #endif > > int mask = O_ACCMODE | O_DIRECT; > > return (flags & mask) == (fd_flags & mask); > > Not sure this is even worth a helper function. Agreed. > > Or am I stull suffering from code review brain? Yes, stull suffering. =)
Fabiano Rosas <farosas@suse.de> writes: > Markus Armbruster <armbru@redhat.com> writes: [...] >> Or am I stull suffering from code review brain? > > Yes, stull suffering. =) %-} /me hoists white flag
diff --git a/monitor/fds.c b/monitor/fds.c index 9a28e4b72b..42bf3eb982 100644 --- a/monitor/fds.c +++ b/monitor/fds.c @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int fd_flags) if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) { match = true; + +#ifdef O_DIRECT + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) { + match = false; + } +#endif } return match;
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 function to behave the same with the O_DIRECT flag. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- monitor/fds.c | 6 ++++++ 1 file changed, 6 insertions(+)