You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/06/20 21:58:41 UTC
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10777
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
7 files changed, 136 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/1
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager-test.cc@631
PS3, Line 631: // a data length and its checksum, and the third is long for a record. The
Nit: too long
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager.h@216
PS3, Line 216: FRIEND_TEST(LogBlockManagerTest, TestExtraNullBytes);
Doesn't exist?
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util-test.cc
File src/kudu/util/pb_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util-test.cc@387
PS3, Line 387: if (version_ == 1) {
: ASSERT_TRUE(s.IsCorruption()) << s.ToString();
: } else {
: ASSERT_TRUE(s.IsIncomplete()) << s.ToString();
: }
Nit: unify with a ternary?
ASSERT_TRUE(version_ == 1 ? s.IsCorruption() : s.IsIncomplete()) << s.ToString();
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@256
PS3, Line 256: namespace {
Isn't this section already in an anonymous namespace?
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@317
PS3, Line 317: // This can happen e.g. on ext4 in the default data=ordered mode, when the
: // filesize metadata is updated but the new data is not persisted.
Maybe expand this a bit and/or link to the tytso thread?
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/slice.h@26
PS3, Line 26: #include "kudu/gutil/port.h"
Bear in mind that slice.h is part of the public API, which doesn't include port.h. That's why client_examples-test.sh is failing.
You'll need to find an alternate way of handling this. Perhaps adding ATTRIBUTE_NO_SANITIZE_THREAD to stubs.h and conditionally including it? We have some examples of that in various client headers.
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 18:00:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
lgtm, just a comment nit
http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc@625
PS4, Line 625: some extra bytes
some extra null bytes
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 00:30:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc@318
PS2, Line 318: // This can happen e.g. on ext4 in the default data=ordered mode, when the
: // filesize metadata is updated but the new data is not persisted.
> how does this happen, though? data=ordered is supposed to be:
The discussion Mike linked in his comment on KUDU-2260 explains it in a roundabout way (https://plus.google.com/+KentonVarda/posts/JDwHfAiLGNQ). If you read it make sure you expand all the comments as the page auto-folds the most important ones in the middle.
Here's the closest thing to a "money quote" I could find:
Kenton Kvarda: "However, I don't think anything you describe quite explains what I'm seeing.
I am not seeing a "zero length" issue. I'm seeing the opposite: I'm seeing the length being ahead of the data. That is:
- I opened a file containing M bytes of data.
- I appended N bytes of data.
- The system crashed before I could fsync().
- On restore, the file has size M + N, but the last N bytes are NUL, not the bytes I wrote.
- Note that M + N in my case was less than the block size, which is 4k. So, no allocation should have occurred.
Is this expected to be a possible outcome with ext4?
It seems to me that with delayed allocation, the file size update should also be delayed, so that the size is updated immediately after the data is flushed. This expectation seems consistent with the documentation for data=ordered, which says:" ...
Ted Ts'o: "We could delay the isize update until after the data block is written back, but in the case where the program is doing more than just an append, but modifying some of the bytes just before isize as well as appending to the file, it's important to note that there may be up to 5 seconds after the data block is written back and when the journal transaction containing the isize update is committed. So suppose the file contained the string "red blue" and the you seek back to the beginning and write "red white blue". There will be window where after a crash reading the file will return "red whit" since the isize field will not have been updated.
Essentially, if you want both the data and isize field to be consistent after a crash, you have to use fsync. So data=ordered has only been engineered to fix the stale data security problem, and doesn't pretend to do anything else."
Ts'o talks more about what data=ordered "really means" in that discussion as well.
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h@302
PS2, Line 302: ATTRIBUTE_NO_SANITIZE_THREAD
> is this attribute needed in the header? thought it would only get picked up
Do you mean you thought it would only get picked up on the definition?
From https://clang.llvm.org/docs/AttributeReference.html#no-sanitize-thread:
"Use __attribute__((no_sanitize_thread)) on a function declaration to specify that checks for data races on plain (non-atomic) memory accesses should not be inserted by ThreadSanitizer."
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 17:13:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10777
to look at the new patch set (#4).
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
6 files changed, 134 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/4
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 3:
Hm tidy bot and iwyu disagree about ordering here. Since iwyu can fail the build but tidy bot can only complain, I'll stick with iwyu's ordering.
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 22:15:21 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc@318
PS2, Line 318: if (IsAllZeros(length_and_cksum_buf)) {
: bool all_zeros;
> The discussion Mike linked in his comment on KUDU-2260 explains it in a rou
aha, that is a very interesting thread. Thanks for the pointer, I learned something new today.
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h@302
PS2, Line 302: // test timeouts. This is only used on local buffers anyway, so we don't lose much
> Do you mean you thought it would only get picked up on the definition?
Oops, yea, I meant that I thought it had to be on the definition rather than the declaration. I'm surprised that the docs say it has to be on the declaration rather than definition. Guess I learned _two_ things today.
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 00:00:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10777
to look at the new patch set (#3).
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
7 files changed, 136 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/3
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10777
to look at the new patch set (#5).
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
6 files changed, 134 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/5
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager-test.cc@631
PS3, Line 631: // a data length and its checksum, and the third is long for a record. The
> Nit: too long
Done
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/fs/log_block_manager.h@216
PS3, Line 216: FRIEND_TEST(LogBlockManagerTest, TestExtraNullBytes);
> Doesn't exist?
Oops.
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util-test.cc
File src/kudu/util/pb_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util-test.cc@387
PS3, Line 387: if (version_ == 1) {
: ASSERT_TRUE(s.IsCorruption()) << s.ToString();
: } else {
: ASSERT_TRUE(s.IsIncomplete()) << s.ToString();
: }
> Nit: unify with a ternary?
Done
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@58
PS3, Line 58: #include "kudu/util/coding.h"
> warning: #includes are not sorted properly [llvm-include-order]
Ignoring. See path-level comment about iwyu and tidy bot disagreeing on order.
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@256
PS3, Line 256: namespace {
> Isn't this section already in an anonymous namespace?
Done
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@317
PS3, Line 317: // This can happen e.g. on ext4 in the default data=ordered mode, when the
: // filesize metadata is updated but the new data is not persisted.
> Maybe expand this a bit and/or link to the tytso thread?
Linked. Is Ts'o well known enough you know his handle? :)
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/slice.h@26
PS3, Line 26: #include "kudu/gutil/port.h"
> Bear in mind that slice.h is part of the public API, which doesn't include
Hm OK I did something that made it work and seemed correct-ish. Let me know if it looks OK to you.
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 22:34:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Reviewed-on: http://gerrit.cloudera.org:8080/10777
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
6 files changed, 134 insertions(+), 51 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 17:17:52 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10777
to look at the new patch set (#2).
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
KUDU-2260: Log block manager should handle null bytes in metadata on crash
On ext4 with data=ordered (the default), it's possible for a write to
persist an increase to the filesize without persisting the actual data.
In this case, the file will contain null bytes at the end. In the LBM,
we considered this case to be corruption if there were enough null bytes
(>= 8) for a PB container record length and length checksum. However,
it's safe to call this an incomplete record and truncate the file at the
end of the last complete record.
This patch changes the P container reader code to return
Status::Incomplete if it encounters this situation. This will cause the
LBM to repair the container metadata appropriately.
Two regression tests, at the PB container file and LBM layers, are
included.
Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
---
M src/kudu/consensus/log_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/slice.h
7 files changed, 135 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/2
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc@625
PS4, Line 625: some extra bytes
> some extra null bytes
Done
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 15:31:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Would like for Mike to review and for Todd to read the answers you provided to his questions.
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/3/src/kudu/util/pb_util.cc@317
PS3, Line 317: // See https://plus.google.com/+KentonVarda/posts/JDwHfAiLGNQ.
: if (IsAllZeros(length_and_cksum_buf)) {
> Linked. Is Ts'o well known enough you know his handle? :)
Sadly (due to ext4 issues that we've chased), yes.
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 22:48:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 )
Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/pb_util.cc@318
PS2, Line 318: // This can happen e.g. on ext4 in the default data=ordered mode, when the
: // filesize metadata is updated but the new data is not persisted.
how does this happen, though? data=ordered is supposed to be:
data=ordered (*) All data are forced directly out to the main file
system prior to its metadata being committed to the
journal.
that's the bit I never understood about this bug.
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h
File src/kudu/util/slice.h:
http://gerrit.cloudera.org:8080/#/c/10777/2/src/kudu/util/slice.h@302
PS2, Line 302: ATTRIBUTE_NO_SANITIZE_THREAD
is this attribute needed in the header? thought it would only get picked up on the declaration
--
To view, visit http://gerrit.cloudera.org:8080/10777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b
Gerrit-Change-Number: 10777
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Jun 2018 16:10:12 +0000
Gerrit-HasComments: Yes