Re: [PATCH v4 32/34] monitor: fdset: Match against O_DIRECT

[Fabiano Rosas <farosas@suse.de>]

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. =)