Add ceph_ll_setlk and ceph_ll_getlk #9566
Conversation
|
@ffilz, this looks fine. Can you add test cases to src/test/libcephfs/flock.cc as well? :) |
|
Waiting for tests before including this in a fs suite run |
|
@ffilz please could you update this with tests so that we can merge it? |
|
Oh, also, please update the libcephfs change's commit message to start "libcephfs: " (see other ceph commits for style) |
|
How much of a test should we add for setlk and getlk? Is there truly no tests of the underlying C++ methods? |
|
There's lots of tests of the underlying Client logic in that test file I referenced, @ffilz. We just want enough to demonstrate that these new glue functions (instead of the fd-based ceph_flock) function as well. |
|
@ffilz ping -- can we get this finished off so that it can be merged? |
|
Sorry, I've been deep in trying to figure out why FSAL_CEPH isn't working on Ganesha... Will try and find time this week to add some tests. |
ffilz
force-pushed
the
master
branch
2 times, most recently
from
1a0e997
to
ea0c76e
Compare
|
I think that covers it. I added unit tests in a new file since setlk/getlk are different from flock. |
|
Looks good, @ffilz please can you remove the "#if 0" blocks and this will be good to merge. |
Signed-off-by: Frank S. Filz <ffilzlnx@mindspring.com>
Signed-off-by: Frank S. Filz <ffilzlnx@mindspring.com>
|
Hmm, I pushed too soon... So I finished up the stuff that had been ifdefed out, but it fails. So then I tried running flock.cc, and it fails in the same place... Basically the multi-threaded test synchronization is failing. I'm not sure how much effort should be put into the test now, the basic functionality is working (and I did another test outside Ganesha of ceph_ll_setlk and it's working fine, multi-threaded and multi-process). |
|
@ffilz ah, you're hitting http://tracker.ceph.com/issues/16556, can you try uncommenting out your tests in conjunction with #10452? |
|
Yes, pull request #10452 solves the problem. I've pushed updated patches with the #if 0 taken out (and some code I didn't finish completed). |
|
One last run in the lab before merging: http://pulpito.ceph.com/jspray-2016-07-28_06:14:58-fs:basic-wip-setlk-distro-basic-mira |
|
It looks like there were some failures, but I don't fully understand all the output... |
|
Weird, it passes locally but is failing in the lab on the final unlink in all the recordlock tests. |
|
@ffilz oh, the problem is that you're mixing ll and regular functions, and the initial ceph_ll_create is being passed uid/gid=0/0, while the final ceph_unlink call is subject to uid/gid checks for the current user. I'd suggest checking that ceph_unlink to be a ceph_ll_unlink |
|
Ah, cool, will do. |
Signed-off-by: Frank S. Filz <ffilzlnx@mindspring.com>
|
Ok, let's try that now... |
|
Unfortunately LibCephFS.InterProcessRecordLocking is generating a lockdep failure: http://qa-proxy.ceph.com/teuthology/jspray-2016-08-02_12:58:37-fs-wip-jcsp-testing-20160802-distro-basic-mira/346922/teuthology.log It's pretty weird that there are no symbols in the backtrace, not immediately obvious to me what's going on here. |
|
Remember we need pull request #10452... Should I merge on top of that? Or just wait for that to merge first, then we can merge this one? |
|
Pushed wip-jcsp-testing-20160805 + will re-run fs/verify against that. I can live with the uncertainty of what the root cause is if #10452 fixes it |
|
Passed here: http://pulpito.ceph.com/jspray-2016-08-07_03:15:31-fs:verify-wip-jcsp-testing-20160805-distro-basic-mira/ (Yeah, believe it or not that's a pass, the failures are OSD InvalidReads and an ansible puke) |
ghost
mentioned this pull request
No description provided.