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/11/10 00:35:29 UTC
[kudu-CR] log block manager: push down container locks
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5029
to review the following change.
Change subject: log block manager: push down container locks
......................................................................
log block manager: push down container locks
This simplifies container logic a bit. PosixRWFile is now fully reentrant,
and there's a new lock in WritablePBContainerFile to synchronize accesses
to offset_.
Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 48 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/5029/1
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manager: push down container locks
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/5029
to look at the new patch set (#4).
Change subject: log block manager: push down container locks
......................................................................
log block manager: push down container locks
This simplifies container logic a bit. PosixRWFile is now fully reentrant,
and there's a new lock in WritablePBContainerFile to synchronize accesses
to offset_.
Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 55 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/5029/4
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manager: push down container locks
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: log block manager: push down container locks
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 570: pending_sync_.Store(true);
> Done
It's gotta be Release:
F1114 19:41:22.799252 15328 atomic.cc:35] Store does not support kMemOrderBarrier: only kMemNorderNoBarrier, kMemOrderAcquire, kMemOrderRelease are supported.
Line 636: if (!pending_sync_.CompareAndSwap(true, false)) {
> Done
Here too.
F1114 19:43:26.498050 22134 atomic.cc:35] CompareAndSwap/CompareAndSet does not support kMemOrderBarrier: only kMemNorderNoBarrier, kMemOrderAcquire, kMemOrderRelease are supported.
I think this one needs to be kMemOrderAcquire, since it "consumes" the value provided in Write().
Line 649: if (closed_.CompareAndSwap(false, true)) {
> Done
Same issue. Why does this one need a memory ordering? What reordering on closed_ are we concerned with here?
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log block manager: push down container locks
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5029
to look at the new patch set (#2).
Change subject: log block manager: push down container locks
......................................................................
log block manager: push down container locks
This simplifies container logic a bit. PosixRWFile is now fully reentrant,
and there's a new lock in WritablePBContainerFile to synchronize accesses
to offset_.
Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 51 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/5029/2
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manager: push down container locks
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: log block manager: push down container locks
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 426: // All operations are safe for concurrent use by multiple threads.
> I would note that this doesn't protect against FS races.
Done
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 570: pending_sync_.Store(true);
> Pretty sure you want at least kMemOrderRelease here, although I would recom
Done
Line 636: if (!pending_sync_.CompareAndSwap(true, false)) {
> I think this needs kMemOrderBarrier.
Done
Line 649: if (closed_.CompareAndSwap(false, true)) {
> kMemOrderBarrier.
Done
Line 697: static int LockOrUnlock(int fd, bool lock) {
> warning: 'LockOrUnlock' is a static definition in anonymous namespace; stat
Done
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 619: std::lock_guard<Mutex> l(offset_lock_);
> This seems overly pessimistic; is there a reason it can't take the lock, up
Heh, if it were that easy I would have made offset_ an AtomicInt and looped with CAS.
The issue is that Write() can fail, and if it does, we don't want to update offset_. Granted, Write() failing will probably lead to a fatal error soon anyway, but from a pure "preventing this in-memory abstraction from becoming corrupt" standpoint, it's more correct for offset_ to only be updated once we know the Write() will succeed.
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/pb_util.h
File src/kudu/util/pb_util.h:
Line 250: // Every function is thread-safe unless indicated otherwise.
> Also add the note about FS races here.
They don't actually apply here because offset_lock_ forces all Append() calls to be serialized. Though that's an implementation detail.
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log block manager: push down container locks
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: log block manager: push down container locks
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 426: // All operations are safe for concurrent use by multiple threads.
I would note that this doesn't protect against FS races.
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 570: pending_sync_.Store(true);
Pretty sure you want at least kMemOrderRelease here, although I would recommend a kMemOrderBarrier just to be safe.
My reasoning is that there is some interplay here between pending_sync_ and closed_, so it's important that the accesses don't get reordered.
Line 636: if (!pending_sync_.CompareAndSwap(true, false)) {
I think this needs kMemOrderBarrier.
Line 649: if (closed_.CompareAndSwap(false, true)) {
kMemOrderBarrier.
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 619: std::lock_guard<Mutex> l(offset_lock_);
This seems overly pessimistic; is there a reason it can't take the lock, updated offset_, drop the lock, then do the write at the old offset?
http://gerrit.cloudera.org:8080/#/c/5029/1/src/kudu/util/pb_util.h
File src/kudu/util/pb_util.h:
Line 250: // Every function is thread-safe unless indicated otherwise.
Also add the note about FS races here.
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log block manager: push down container locks
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: log block manager: push down container locks
......................................................................
log block manager: push down container locks
This simplifies container logic a bit. PosixRWFile is now fully reentrant,
and there's a new lock in WritablePBContainerFile to synchronize accesses
to offset_.
Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
Reviewed-on: http://gerrit.cloudera.org:8080/5029
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 55 insertions(+), 46 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manager: push down container locks
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: log block manager: push down container locks
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f69bc8b455bce2e1d89ed22713e57a9c3006dc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No