[ovs-dev] Switch ovsdb log fsync to data only

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

We do not check metadata - mtime, atime, anywhere, so we
do not need to update it every time we sync the log.
if the system supports it, the log update should be
data only

Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 ovsdb/log.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On Tue, Apr 21, 2020 at 09:23:57AM +0100, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> We do not check metadata - mtime, atime, anywhere, so we
> do not need to update it every time we sync the log.
> if the system supports it, the log update should be
> data only
> 
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
LGTM,
But how do you know we do not check mtime or atime of the ovsdb log file?
If there isn't a lot of performance overhead updating the metadata,
why not keep it as it is now?

William

> ---
>  ovsdb/log.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/ovsdb/log.c b/ovsdb/log.c
> index c82a79c9f..41af77679 100644
> --- a/ovsdb/log.c
> +++ b/ovsdb/log.c
> @@ -658,7 +658,16 @@ ovsdb_log_write_and_free(struct ovsdb_log *log, struct json *json)
>  struct ovsdb_error *
>  ovsdb_log_commit_block(struct ovsdb_log *file)
>  {
> +#if (_POSIX_C_SOURCE >= 199309L || _XOPEN_SOURCE >= 500)
> +    /* we do not check metadata - mtime, atime, anywhere, so we
> +     * do not need to update it every time we sync the log.
> +     * if the system supports it, the log update should be
> +     * data only
> +     */
> +    if (file->stream && fdatasync(fileno(file->stream))) {
> +#else
>      if (file->stream && fsync(fileno(file->stream))) {
> +#endif
>          return ovsdb_io_error(errno, "%s: fsync failed", file->display_name);
>      }
>      return NULL;
> -- 
> 2.20.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 21/04/2020 17:04, William Tu wrote:
> On Tue, Apr 21, 2020 at 09:23:57AM +0100, anton.ivanov@cambridgegreys.com wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> We do not check metadata - mtime, atime, anywhere, so we
>> do not need to update it every time we sync the log.
>> if the system supports it, the log update should be
>> data only
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> LGTM,
> But how do you know we do not check mtime or atime of the ovsdb log file?

By searching through the code :)

stat and stat64 are fairly easy to grep for and so is their use. They are mostly confined to ssl-stream.c

> If there isn't a lot of performance overhead updating the metadata,
> why not keep it as it is now?

The performance overhead on a spinning rust disk is massive - it is several times. In fact, you can hear it. My source trees are on a SAS 7.2K RPM array and with the current upstream the testsuite sounds like a hectic Call of Duty bout.

With this, the sound becomes much "smoother" - you can hear that the disks are no longer searching like crazy.

On a non-rotational - not so much, but still in the 10s of percent. The kernel has to issue at least two sets of requests instead of one and there is quite a bit of locking and barriers on metadata updates.

A.

>
> William
>
>> ---
>>   ovsdb/log.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/ovsdb/log.c b/ovsdb/log.c
>> index c82a79c9f..41af77679 100644
>> --- a/ovsdb/log.c
>> +++ b/ovsdb/log.c
>> @@ -658,7 +658,16 @@ ovsdb_log_write_and_free(struct ovsdb_log *log, struct json *json)
>>   struct ovsdb_error *
>>   ovsdb_log_commit_block(struct ovsdb_log *file)
>>   {
>> +#if (_POSIX_C_SOURCE >= 199309L || _XOPEN_SOURCE >= 500)
>> +    /* we do not check metadata - mtime, atime, anywhere, so we
>> +     * do not need to update it every time we sync the log.
>> +     * if the system supports it, the log update should be
>> +     * data only
>> +     */
>> +    if (file->stream && fdatasync(fileno(file->stream))) {
>> +#else
>>       if (file->stream && fsync(fileno(file->stream))) {
>> +#endif
>>           return ovsdb_io_error(errno, "%s: fsync failed", file->display_name);
>>       }
>>       return NULL;
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Tue, Apr 21, 2020 at 05:24:01PM +0100, Anton Ivanov wrote:
> 
> On 21/04/2020 17:04, William Tu wrote:
> >On Tue, Apr 21, 2020 at 09:23:57AM +0100, anton.ivanov@cambridgegreys.com wrote:
> >>From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> >>
> >>We do not check metadata - mtime, atime, anywhere, so we
> >>do not need to update it every time we sync the log.
> >>if the system supports it, the log update should be
> >>data only
> >>
> >>Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> >LGTM,
> >But how do you know we do not check mtime or atime of the ovsdb log file?
> 
> By searching through the code :)
> 
> stat and stat64 are fairly easy to grep for and so is their use. They are mostly confined to ssl-stream.c
> 
> >If there isn't a lot of performance overhead updating the metadata,
> >why not keep it as it is now?
> 
> The performance overhead on a spinning rust disk is massive - it is several times. In fact, you can hear it. My source trees are on a SAS 7.2K RPM array and with the current upstream the testsuite sounds like a hectic Call of Duty bout.
> 
> With this, the sound becomes much "smoother" - you can hear that the disks are no longer searching like crazy.
> 
> On a non-rotational - not so much, but still in the 10s of percent. The kernel has to issue at least two sets of requests instead of one and there is quite a bit of locking and barriers on metadata updates.
> 
> A.
Applied to master, thanks.