You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/12/10 03:07:12 UTC

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS 

Hello Dan Burkert, Mike Percy,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5457

to review the following change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................

env: do not convert all Flush() calls into fsync() on macOS

On Linux, we don't expect sync_file_range() to actually provide durability;
we use it to tell the kernel to start writing back dirty pages while we go
off and do other work. That is, it must be followed up with an fsync() if
durability is actually desired.

To that end, let's only convert Flush() to fsync() if a synchronous flush
was requested. Even then I'm not sure it makes sense (sync_file_range()
explicitly does NOT guarantee durability, even if SYNC_FILE_RANGE_WAIT_AFTER
was used), but at least callers will get the "only return when all dirty
pages have been written out" behavior they probably wanted.

Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
---
M src/kudu/util/env_posix.cc
1 file changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5457/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


env: do not convert all Flush() calls into fsync() on macOS

On Linux, we don't expect sync_file_range() to actually provide durability;
we use it to tell the kernel to start writing back dirty pages while we go
off and do other work. That is, it must be followed up with an fsync() if
durability is actually desired.

To that end, let's only convert Flush() to fsync() if a synchronous flush
was requested. Even then I'm not sure it makes sense (sync_file_range()
explicitly does NOT guarantee durability, even if SYNC_FILE_RANGE_WAIT_AFTER
was used), but at least callers will get the "only return when all dirty
pages have been written out" behavior they probably wanted.

I also snuck in a change to add SYNC_FILE_RANGE_WAIT_BEFORE to the
FLUSH_SYNC case on Linux. It's largely academic (since FLUSH_SYNC is never
actually used), but it's safer if it is ever used for data integrity.

Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Reviewed-on: http://gerrit.cloudera.org:8080/5457
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/env_posix.cc
1 file changed, 3 insertions(+), 2 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5457/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

PS1, Line 653: flags |= SYNC_FILE_RANGE_WAIT_AFTER
> I'm not quite familiar with intent of this method, but after looking into t
Hmm. It's largely academic since FLUSH_SYNC is never used (nor will it be, I think), but I suppose WAIT_BEFORE should be added. It's unclear to me, because I think WRITE | WAIT_AFTER could be interpreted as "write out all dirty pages and wait for all outstanding writes to finish", regardless of whether WAIT_BEFORE is added. But, it doesn't hurt.


-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


Patch Set 1:

Since this is about macOS durability semantics, and Kudu is not intended for production use on macOS, I'm ambivalent about this one so will leave it to people working on macOS to review. :)

-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5457

to look at the new patch set (#2).

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................

env: do not convert all Flush() calls into fsync() on macOS

On Linux, we don't expect sync_file_range() to actually provide durability;
we use it to tell the kernel to start writing back dirty pages while we go
off and do other work. That is, it must be followed up with an fsync() if
durability is actually desired.

To that end, let's only convert Flush() to fsync() if a synchronous flush
was requested. Even then I'm not sure it makes sense (sync_file_range()
explicitly does NOT guarantee durability, even if SYNC_FILE_RANGE_WAIT_AFTER
was used), but at least callers will get the "only return when all dirty
pages have been written out" behavior they probably wanted.

I also snuck in a change to add SYNC_FILE_RANGE_WAIT_BEFORE to the
FLUSH_SYNC case on Linux. It's largely academic (since FLUSH_SYNC is never
actually used), but it's safer if it is ever used for data integrity.

Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
---
M src/kudu/util/env_posix.cc
1 file changed, 3 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5457/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5457/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

PS1, Line 653: flags |= SYNC_FILE_RANGE_WAIT_AFTER
I'm not quite familiar with intent of this method, but after looking into the non-linux section, reading docs for the method and http://man7.org/linux/man-pages/man2/sync_file_range.2.html (section 'Some details'), I think SYNC_FILE_RANGE_WAIT_BEFORE might be added into the flags as well to mirror fsync() behavior for range of the file data.

In other words, I have a question: is SYNC_FILE_RANGE_WAIT_BEFORE flag omitted deliberately in case of FLUSH_SYNC mode?


-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] env: do not convert all Flush() calls into fsync() on macOS

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: env: do not convert all Flush() calls into fsync() on macOS
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5457/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

PS1, Line 653: flags |= SYNC_FILE_RANGE_WAIT_AFTER
> Hmm. It's largely academic since FLUSH_SYNC is never used (nor will it be, 
Ok, I see.  Thanks for the clarification.

To me, the description of SYNC_FILE_RANGE_WAIT_{BEFORE,AFTER} is a little bit confusing in the manual, but since 'Some details' recommend having both enabled for write-for-data-integrity operations, I also would prefer having both flags specified here.


-- 
To view, visit http://gerrit.cloudera.org:8080/5457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I01bdd8dbaaad0205c0795a87dd973c8bf0fb87dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes