From 0cabf44a60b1ffcc4f4c75f745d0375dca0af53e Mon Sep 17 00:00:00 2001 From: Leonid Usov Date: Fri, 26 Apr 2024 03:20:42 +0300 Subject: [PATCH] mds/quiesce: db: quiesce-await should EPERM if a set is past QS_QUIESCED Fixes: https://tracker.ceph.com/issues/65669 Signed-off-by: Leonid Usov (cherry picked from commit fd88b52d6fb11c206b3a06e8533c4551c902f173) Fixes: https://tracker.ceph.com/issues/66034 --- doc/cephfs/fs-volumes.rst | 44 ++++++++++++++++++++++++++++------- src/mds/QuiesceDbManager.cc | 7 +++--- src/test/mds/TestQuiesceDb.cc | 9 ++++--- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/doc/cephfs/fs-volumes.rst b/doc/cephfs/fs-volumes.rst index 5422795f0d821..8ad5258814ae6 100644 --- a/doc/cephfs/fs-volumes.rst +++ b/doc/cephfs/fs-volumes.rst @@ -1071,15 +1071,41 @@ Note that the commands above are all non-blocking. If we want to wait for the qu to reach the `QUIESCED` state, we should await it at some point. ``--await`` can be given along with other arguments to let the system know our intention. -Technically, there are two types of await: `quiesce await` and `release await`. The former -is the default, and the latter can only be achieved with ``--release`` present in the argument list. -To avoid confision, it is not permitted to issue a `quiesce await` when the set is already `RELEASING` -or `RELEASED`. Trying to ``--release`` a set that is not `QUIESCED` is an ``EPERM`` error as well, regardless -of whether await is requested alongside. However, it's not an error to `release await` -an already released set, or to `quiesce await` a `QUIESCED` one. - -When awaiting, one may also specify a maximum duration that they would like this await request to block for, -not affecting the two intrinsic timeouts discussed above. If the target awaited state isn't reached +There are two types of await: `quiesce await` and `release await`. The former is the default, +and the latter can only be achieved with ``--release`` present in the argument list. +To avoid confision, it is not permitted to issue a `quiesce await` when the set is not `QUIESCING`. +Trying to ``--release`` a set that is not `QUIESCED` is an ``EPERM`` error as well, regardless +of whether await is requested alongside. However, it's not an error to `release await` +an already released set, or to `quiesce await` a `QUIESCED` one - those are successful no-ops. + +Since a set is awaited after the application of the ``--await``-augmented command, the await operation +may mask a successful result with its own error. A good example is trying to cancel-await a set: + +.. prompt:: bash $ auto + + $ ceph fs quiesce fs1 --set-id set1 --cancel --await + { + // ... + "sets": { + "set1": { + // ... + "state": { + "name": "CANCELED", + "age": 0 + }, + // ... + } + } + } + Error EPERM: + +Although ``--cancel`` will succeed syncrhonously for a set in an active state, awaiting a canceled +set is not permitted, hence this call will result in an ``EPERM``. This is deliberately different from +returning a ``EINVAL`` error, denoting an error on the user's side, to simplify the system's behavior +when ``--await`` is requested. As a result, it's also a simpler model for the user to work with. + +When awaiting, one may specify a maximum duration that they would like this await request to block for, +orthogonally to the two intrinsic set timeouts discussed above. If the target awaited state isn't reached within the specified duration, then ``EINPROGRESS`` is returned. For that, one should use the argument ``--await-for=``. One could think of ``--await`` as equivalent to ``--await-for=Infinity``. While it doesn't make sense to specify both arguments, it is not considered an error. If diff --git a/src/mds/QuiesceDbManager.cc b/src/mds/QuiesceDbManager.cc index d070434dbff31..2c727d7152608 100644 --- a/src/mds/QuiesceDbManager.cc +++ b/src/mds/QuiesceDbManager.cc @@ -623,12 +623,13 @@ int QuiesceDbManager::leader_process_request(RequestContext* req_ctx) } if (request.await) { + // quiesce-await is only allowed for sets that are quiescing or quiesced. // this check may have a false negative for a quiesced set // that will be released in another request in the same batch // in that case, this await will be enqueued but then found and completed // with the same error in `leader_upkeep_awaits` - if ((set.is_releasing() || set.is_released()) && !request.is_release()) { - dout(2) << dset("can't quiesce-await a set that was released (") << set.rstate.state << ")" << dendl; + if (set.rstate.state > QS_QUIESCED && !request.is_release()) { + dout(2) << dset("can't quiesce-await a set in the state: ") << set.rstate.state << dendl; return EPERM; } @@ -1055,7 +1056,7 @@ QuiesceTimeInterval QuiesceDbManager::leader_upkeep_awaits() break; case QS_EXPIRED: case QS_TIMEDOUT: - rc = ETIMEDOUT; + rc = ETIMEDOUT; break; case QS_QUIESCED: rc = 0; // fallthrough diff --git a/src/test/mds/TestQuiesceDb.cc b/src/test/mds/TestQuiesceDb.cc index cf05aaa038d0a..4f98e5dbab535 100644 --- a/src/test/mds/TestQuiesceDb.cc +++ b/src/test/mds/TestQuiesceDb.cc @@ -1010,13 +1010,12 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait) EXPECT_EQ(ERR(ECANCELED), await3.wait_result()); EXPECT_EQ(ERR(ECANCELED), await4.wait_result()); - // awaiting a set in a terminal state should immediately - // complete with the corresponding error - ASSERT_EQ(ERR(ECANCELED), run_request([](auto& r) { + // awaiting a set in a terminal state should be illegal + EXPECT_EQ(ERR(EPERM), run_request([](auto& r) { r.set_id = "set1"; r.await = sec(100); })); - ASSERT_EQ(ERR(ETIMEDOUT), run_request([](auto& r) { + EXPECT_EQ(ERR(EPERM), run_request([](auto& r) { r.set_id = "set2"; r.await = sec(100); })); @@ -1082,7 +1081,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) { EXPECT_EQ(QS_EXPIRED, last_request->response.sets.at("set1").rstate.state); - EXPECT_EQ(ERR(ETIMEDOUT), run_request([](auto& r) { + EXPECT_EQ(ERR(EPERM), run_request([](auto& r) { r.set_id = "set1"; r.await = sec(0.1); })); -- 2.39.5