| Message ID | 1506432289.3182.16.camel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 26 Sep 2017, Jeff Layton wrote: > In order to get the correct semantics for a delegation or oplock, we > need to be able to break delegations on open. Can you explain this part? How do the delegation/oplock semantics relate to open(2)? I would assume they are related to reads and writes (and data consistency). Unless it's a CIFS thing? sage -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-09-26 at 13:31 +0000, Sage Weil wrote: > On Tue, 26 Sep 2017, Jeff Layton wrote: > > In order to get the correct semantics for a delegation or oplock, > we > > need to be able to break delegations on open. > > Can you explain this part? How do the delegation/oplock semantics > relate > to open(2)? I would assume they are related to reads and writes (and > data > consistency). Unless it's a CIFS thing? You're correct that we only really need to break delegations on conflicting access, but there is a problem here: NFS clients can cache shared/deny mode locks if they hold a delegation. Once you've granted an open, it's too late to go back and enforce it once you go to recall the delegation. Since we don't enforce deny mode locks in cephfs, we could argue that all of that is the purview of the NFS server and not worry about it...at least until we toss something like samba into the mix, at which point things get a bit more complicated...
On Tue, 2017-09-26 at 10:06 -0400, Jeff Layton wrote: > On Tue, 2017-09-26 at 13:31 +0000, Sage Weil wrote: > > On Tue, 26 Sep 2017, Jeff Layton wrote: > > > In order to get the correct semantics for a delegation or oplock, > > > > we > > > need to be able to break delegations on open. > > > > Can you explain this part? How do the delegation/oplock semantics > > relate > > to open(2)? I would assume they are related to reads and writes (and > > data > > consistency). Unless it's a CIFS thing? > > > You're correct that we only really need to break delegations on > conflicting access, but there is a problem here: > > NFS clients can cache shared/deny mode locks if they hold a delegation. > Once you've granted an open, it's too late to go back and enforce it > once you go to recall the delegation. > > Since we don't enforce deny mode locks in cephfs, we could argue that > all of that is the purview of the NFS server and not worry about > it...at least until we toss something like samba into the mix, at which > point things get a bit more complicated... > After some discussion with Sage on IRC, I think I understand what's going on now. The issue is that Client::get_caps doesn't actually request any caps. It just waits on the needed caps, under the assumption that you've already requested the ones you need via an earlier call. In this case, the file already existed before the client connected, and there is no interaction with the file by that client prior to the open. There is no reason that the MDS thinks that the client wants any caps for this file, and so the call hangs waiting on caps that will never arrive. I've been able to fix the patch by having it call get_caps after the open call. With that, the MDS knows that the client is holding the file open, and so will want Fr and/or Fw caps for that. That's sufficient to satisfy the right delegation break semantic. The upshot here is that this isn't a bug, after all... Cheers,
From 6f2d562f06529631235bea02b821f83a1a3d0e73 Mon Sep 17 00:00:00 2001 From: Jeffrey Layton <jlayton@ceres.poochiereds.net> Date: Mon, 25 Sep 2017 15:09:48 -0400 Subject: [PATCH] client: request caps on open Signed-off-by: Jeff Layton <jlayton@redhat.com> --- src/client/Client.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index 78517c808a..ffe877fdf8 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -8425,11 +8425,22 @@ int Client::_open(Inode *in, int flags, mode_t mode, Fh **fhp, in->get_open_ref(cmode); // make note of pending open, since it effects _wanted_ caps. - if ((flags & O_TRUNC) == 0 && - in->caps_issued_mask(want)) { + if ((flags & O_TRUNC) == 0 && in->caps_issued_mask(want)) { // update wanted? check_caps(in, CHECK_CAPS_NODELAY); } else { + int need = 0, have; + + /* Wait on minimal caps, just to break delegations */ + if (cmode & CEPH_FILE_MODE_WR) + need |= CEPH_CAP_FILE_WR; + if (cmode & CEPH_FILE_MODE_RD) + need |= CEPH_CAP_FILE_RD; + + result = get_caps(in, need, want, &have, -1); + if (result < 0) + return result; + MetaRequest *req = new MetaRequest(CEPH_MDS_OP_OPEN); filepath path; in->make_nosnap_relative_path(path); @@ -8444,6 +8455,7 @@ int Client::_open(Inode *in, int flags, mode_t mode, Fh **fhp, req->head.args.open.old_size = in->size; // for O_TRUNC req->set_inode(in); result = make_request(req, perms); + put_cap_ref(in, need); } // success? -- 2.13.5
In order to get the correct semantics for a delegation or oplock, we need to be able to break delegations on open. Cephfs' open routine is pretty light with the caps though. One way to fix that is to request a enough caps to cause a delegation break if any are outstanding. So, I've been playing with the attached patch. It may be wrong and not what we want, but it seems like it ought to work. This patch however causes the LibCephFS.Fchmod test to hang, waiting on caps, iff the DoubleChmod test is run before it. If you run just Fchmod test, it works fine. They both use the same filename and hence the same inode. The test hangs at the first Fchmod test ceph_open call, waiting for the caps. At that point though, there should be no other clients accessing the mount. Is it possible that outstanding caps granted to the client from the earlier test could be surviving a ceph_shutdown, such that it blocks the next client from getting them? To reproduce I've been running this on a build with the attached patch vs. a vstart cluster: $ ./bin/ceph_test_libcephfs --gtest_filter=LibCephFS.DoubleChmod:LibCephFS.Fchmod I'll continue to dig into this, but I'm starting to wonder if this is exposing a bug in the mds. Should I open a tracker bug on this?