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

[Fabiano Rosas]

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.