On 2/16/22 2:35 AM, Jeff Layton wrote:
On Tue, 2022-02-15 at 20:23 +0800, xiubli@xxxxxxxxxx wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>
No need to update snapshot context when any of the following two
cases happens:
1: if my context seq matches realm's seq and realm has no parent.
2: if my context seq equals or is larger than my parent's, this
works because we rebuild_snap_realms() works _downward_ in
hierarchy after each update.
This fix will avoid those inodes which accidently calling
ceph_queue_cap_snap() and make no sense, for exmaple:
There have 6 directories like:
/dir_X1/dir_X2/dir_X3/
/dir_Y1/dir_Y2/dir_Y3/
Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
make a root snapshot under /.snap/root_snap. And every time when
we make snapshots under /dir_Y1/..., the kclient will always try
to rebuild the snap context for snap_X2 realm and finally will
always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
no sense.
That's because the snap_X2's seq is 2 and root_snap's seq is 3.
So when creating a new snapshot under /dir_Y1/... the new seq
will be 4, and then the mds will send kclient a snapshot backtrace
in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
it will always rebuild the from the last realm, that's the root_snap.
So later when rebuilding the snap context it will always rebuild
the snap_X2 realm and then try to queue cap snaps for all the inodes
related in snap_X2 realm, and we are seeing the logs like:
"ceph: queue_cap_snap 00000000a42b796b nothing dirty|writing"
URL: https://tracker.ceph.com/issues/44100
Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
---
fs/ceph/snap.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index d075d3ce5f6d..1f24a5de81e7 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
num += parent->cached_context->num_snaps;
}
- /* do i actually need to update? not if my context seq
- matches realm seq, and my parents' does to. (this works
- because we rebuild_snap_realms() works _downward_ in
- hierarchy after each update.) */
+ /* do i actually need to update? No need when any of the following
+ * two cases:
+ * #1: if my context seq matches realm's seq and realm has no parent.
+ * #2: if my context seq equals or is larger than my parent's, this
+ * works because we rebuild_snap_realms() works _downward_ in
+ * hierarchy after each update.
+ */
This may be, but that downward building is done via unbounded recursion
in rebuild_snap_realms().
I'm ok with taking this patch in the short term, but I would really like
to see steps made to eliminate recursion from this code altogether. A
sufficiently deep hierarchy could blow out the stack. How could we
redesign this code to avoid it?
Yeah, the recursion really a potential issue here, let me figure out one
method to eliminate it later in the following patches, there has some
other code also may need to improve, and I am still going through the
code, not very clear yet. But not related to the perfermance issue this
patch is trying to fix.
-- Xiubo
if (realm->cached_context &&
- realm->cached_context->seq == realm->seq &&
- (!parent ||
- realm->cached_context->seq >= parent->cached_context->seq)) {
+ ((realm->cached_context->seq == realm->seq && !parent) ||
+ (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
" (unchanged)\n",
realm->ino, realm, realm->cached_context,
