Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ceph_fuse: fix daemonization when pid file is non-empty #13532

Merged
merged 1 commit into from
Feb 28, 2017

Conversation

ukernel
Copy link
Contributor

@ukernel ukernel commented Feb 20, 2017

During daemonizing ceph-fuse, both the parent and child processes call
global_init_postfork_start(). global_init_postfork_start() calls
pidfile_write(). If pid file is non-empty, one pidfile_write() call
fails because it can't lock the pid file.

The fix is only child process calls global_init_postfork_start()

Signed-off-by: "Yan, Zheng" zyan@redhat.com

@ukernel ukernel added bug-fix cephfs Ceph File System labels Feb 20, 2017
@jcsp
Copy link
Contributor

jcsp commented Feb 20, 2017

@tchaikov would you mind reviewing this? I think you're more familiar with the preforker etc code

@tchaikov
Copy link
Contributor

will take a look shortly.

src/ceph_fuse.cc Outdated
int r = forker.parent_wait(err);
if (r) {
cerr << "ceph-fuse" << err << std::endl;
if (global_init_prefork(g_ceph_context) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global_init_prefork() should always return 0 here. we can either add an assert, or ignore the the return value.

src/ceph_fuse.cc Outdated
r = forker.prefork(err);
if (r < 0) {
cerr << "ceph-fuse" << err << std::endl;
forker.exit(r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exit(3) call does not allow the destructors to be called. take the cct for example, its dtor won't be called this way. so i'd recommend use return instead.

src/ceph_fuse.cc Outdated
if (r < 0) {
cerr << "ceph-fuse" << err << std::endl;
}
forker.exit(r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

@ukernel
Copy link
Contributor Author

ukernel commented Feb 22, 2017

@tchaikov updated

tchaikov
tchaikov previously approved these changes Feb 22, 2017
@ukernel
Copy link
Contributor Author

ukernel commented Feb 22, 2017

I saw following crash after replacing forker.exit(r) with 'return r'. The monitor code uses forker.exit(r) too. Maybe it's intentional to avoid the destructors

2017-02-22 10:16:40.070124 7fae407ba240 -1 /home/zhyan/Ceph/ceph/src/log/Log.cc: In function 'void ceph::logging::Log::stop()' thread 7fae407ba240 time 2017-02-22 10:16:40.069483
/home/zhyan/Ceph/ceph/src/log/Log.cc: 431: FAILED assert(is_started())

 ceph version 12.0.0-251-g2f6046c (2f6046cf2041468da66f4a4a03abd7d3bebb7461)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x102) [0x7fae37885fa2]
 2: (()+0x2a1704) [0x7fae378aa704]
 3: (CephContext::~CephContext()+0x4fb) [0x7fae37a3c74b]
 4: (CephContext::put()+0x173) [0x7fae37a3c9f3]
 5: (main()+0x96f) [0x5624194eb08f]
 6: (__libc_start_main()+0xf1) [0x7fae34d12731]
 7: (_start()+0x29) [0x5624194ec449]
 NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.

@tchaikov
Copy link
Contributor

oic, the reason we called global_init_postfork_start() is to start the log so both parent and child can return cleanly.

@ukernel
Copy link
Contributor Author

ukernel commented Feb 22, 2017

there are only two users of global_init_postfork_start(), ceph-fuse and ceph-mon. For ceph-mon, the parent process does not call global_init_postfork_start().

@tchaikov tchaikov dismissed their stale review February 22, 2017 03:45

as discussed over irc, i'd suggest to bring cbf18b1 back to start log in parent.

During daemonizing ceph-fuse, both the parent and child processes call
global_init_postfork_start(). global_init_postfork_start() calls
pidfile_write(). If pid file is non-empty, one pidfile_write() call
fails because it can't lock the pid file.

The fix is only child process calls global_init_postfork_start()

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
@jcsp jcsp merged commit 6a0c987 into ceph:master Feb 28, 2017
@ukernel ukernel deleted the wip-18995 branch May 11, 2017 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants