From bf89e7e0d2dacf96dccb998e518c747990edf3fd Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Sat, 10 Oct 2026 11:13:10 +0300 Subject: [PATCH 1/2] Add an end key to K/V listings A listing goes on from block to block until it finds a key, and blocks emptied by deletes are never freed. So a listing that runs past the end of its range reads every empty block up to the next existing key, each with a version check on an OSD. After many creates and deletes, this made a single RMDIR in the NFS proxy take hundreds of milliseconds. list_start(start, end) stops at the first key >= end and does not read blocks beyond it. vitastor-kv list/dump/dumpjson now pass their end argument. --- .gitea/workflows/test.yml | 18 +++++++++++++++ src/kv/kv_cli.cpp | 2 +- src/kv/kv_db.cpp | 26 +++++++++++++++++++++ src/kv/vitastor_kv.h | 2 ++ tests/run_tests.sh | 1 + tests/test_kv_list_bounds.sh | 45 ++++++++++++++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 1 deletion(-) create mode 100755 tests/test_kv_list_bounds.sh diff --git a/.gitea/workflows/test.yml b/.gitea/workflows/test.yml index 6c72c970..896c36ff 100644 --- a/.gitea/workflows/test.yml +++ b/.gitea/workflows/test.yml @@ -1067,6 +1067,24 @@ jobs: echo "" done + test_kv_list_bounds: + runs-on: ubuntu-latest + needs: build + container: ${{env.TEST_IMAGE}}:${{github.sha}} + steps: + - name: Run test + id: test + timeout-minutes: 3 + run: /root/vitastor/tests/test_kv_list_bounds.sh + - name: Print logs + if: always() && steps.test.outcome == 'failure' + run: | + for i in /root/vitastor/testdata/*.log /root/vitastor/testdata/*.txt; do + echo "-------- $i --------" + cat $i + echo "" + done + test_splitbrain: runs-on: ubuntu-latest needs: build diff --git a/src/kv/kv_cli.cpp b/src/kv/kv_cli.cpp index f5c7c709..19feaa56 100644 --- a/src/kv/kv_cli.cpp +++ b/src/kv/kv_cli.cpp @@ -664,7 +664,7 @@ void kv_cli_t::handle_cmd(const std::vector & cmd, std::function= 2 ? cmd[1] : ""; std::string end = cmd.size() >= 3 ? cmd[2] : ""; - lst->handle = db->list_start(start); + lst->handle = db->list_start(start, end); lst->db = db; lst->format = opname == "dump" ? 1 : (opname == "dumpjson" ? 2 : 0); lst->cb = std::move(cb); diff --git a/src/kv/kv_db.cpp b/src/kv/kv_db.cpp index 7e124666..7ea02828 100644 --- a/src/kv/kv_db.cpp +++ b/src/kv/kv_db.cpp @@ -190,6 +190,8 @@ struct kv_op_t kv_db_t *db; int opcode; std::string key, value; + // for list: stop before the first key >= list_end, unless list_end is empty + std::string list_end; int res; bool in_next = false; bool next_retry_pending = false; @@ -2021,6 +2023,12 @@ void kv_op_t::next_get() } if (kv_it != blk->data.end()) { + if (list_end != "" && kv_it->first >= list_end) + { + // End of the listing range + finish(-ENOENT); + return; + } // Send this item assert(blk->type == KV_LEAF || blk->type == KV_LEAF_SPLIT); this->res = 0; @@ -2032,6 +2040,12 @@ void kv_op_t::next_get() // Find next block else if (blk->type == KV_LEAF_SPLIT) { + if (list_end != "" && blk->right_half >= list_end) + { + // The right half is beyond the listing range + finish(-ENOENT); + return; + } // Left half finished, go to the right recheck_policy = KV_RECHECK_LEAF; key = blk->right_half; @@ -2063,6 +2077,12 @@ void kv_op_t::next_go_up() // Block can be updated and key_lt may be lower. Don't rewind it key = blk->key_lt; } + if (list_end != "" && key >= list_end) + { + // Next keys are beyond the listing range, don't read the next block + finish(-ENOENT); + return; + } skip_equal = false; path.pop_back(); cur_level--; @@ -2194,6 +2214,11 @@ void vitastorkv_dbw_t::del(const std::string & key, std::function } void* vitastorkv_dbw_t::list_start(const std::string & start) +{ + return list_start(start, ""); +} + +void* vitastorkv_dbw_t::list_start(const std::string & start, const std::string & end) { if (!db->inode_id || db->closing) return NULL; @@ -2201,6 +2226,7 @@ void* vitastorkv_dbw_t::list_start(const std::string & start) op->db = db; op->opcode = KV_LIST; op->key = start; + op->list_end = end; op->callback = [](kv_op_t *){}; op->exec(); return op; diff --git a/src/kv/vitastor_kv.h b/src/kv/vitastor_kv.h index fe4c570a..9efd078f 100644 --- a/src/kv/vitastor_kv.h +++ b/src/kv/vitastor_kv.h @@ -40,6 +40,8 @@ struct __attribute__((visibility("default"))) vitastorkv_dbw_t std::function cas_compare = NULL); void* list_start(const std::string & start); + // list keys >= start and < end (all keys >= start if end is empty) + void* list_start(const std::string & start, const std::string & end); void list_next(void *handle, std::function cb); void list_close(void *handle); diff --git a/tests/run_tests.sh b/tests/run_tests.sh index c29ef29b..d3a07ba9 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -85,6 +85,7 @@ OLD=1 SCHEME=ec ./test_snapshot_down.sh ./test_kv_stress.sh IMMEDIATE_COMMIT=1 ./test_kv_stress.sh +./test_kv_list_bounds.sh ./test_splitbrain.sh diff --git a/tests/test_kv_list_bounds.sh b/tests/test_kv_list_bounds.sh new file mode 100755 index 00000000..7603f458 --- /dev/null +++ b/tests/test_kv_list_bounds.sh @@ -0,0 +1,45 @@ +#!/bin/bash -ex +# Test K/V listings with an end key: they must return exactly the keys of their range, +# also when the range spans several blocks or ends in the middle of a block. + +PG_COUNT=16 +. `dirname $0`/run_3osds.sh + +$VITASTOR_CLI create -s 1G kvlist +KV="build/src/kv/vitastor-kv --config_path $VITASTOR_CFG kvlist" + +# 1000 keys with 100-byte values span several 4 KB blocks +VALUE=$(printf 'v%.0s' $(seq 1 100)) +( + for i in $(seq -f '%03g' 0 999); do + echo "set t/a$i $VALUE" + done + echo "set t/b0 v" +) | $KV > ./testdata/kv_list_set.log 2>&1 +if [ "$(grep -c '^OK' ./testdata/kv_list_set.log)" -ne 1001 ]; then + format_error "Failed to set the keys" +fi + +list_keys() +{ + $KV list "$@" | sed 's/ = .*//' +} + +if [ "$(list_keys t/a t/b)" != "$(seq -f 't/a%03g' 0 999)" ]; then + format_error "list t/a t/b did not return exactly t/a000..t/a999" +fi +if [ "$(list_keys t/a5 t/a6)" != "$(seq -f 't/a%03g' 500 599)" ]; then + format_error "list t/a5 t/a6 did not return exactly t/a500..t/a599" +fi +if [ -n "$(list_keys t/c t/d)" ]; then + format_error "list t/c t/d returned keys" +fi +if [ "$($KV dump t/a5 t/a6 | grep -c '^set ')" -ne 100 ]; then + format_error "dump t/a5 t/a6 did not return 100 keys" +fi +# Without an end, the listing goes on past the range +if [ "$(list_keys t/a | head -n 1001)" != "$(seq -f 't/a%03g' 0 999; echo t/b0)" ]; then + format_error "list t/a did not return t/a000..t/a999, then t/b0" +fi + +format_green OK -- 2.47.3 From c3303a4122786d06dfa88dd66c8f3f9cd549edf3 Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Sat, 10 Oct 2026 11:13:10 +0300 Subject: [PATCH 2/2] Stop NFS prefix scans at the end of the prefix The emptiness checks of RMDIR and of RENAME over a directory, READDIR and the defrag scans list a key prefix. They now stop at the end of the prefix instead of running on to the next existing key through blocks emptied by deletes, which made RMDIR slower with every deleted file. This also fixes RENAME of a directory over an empty directory failing with ENOTEMPTY: its check took the first key after the prefix, which belongs to another directory or inode, for an entry. --- .gitea/workflows/test.yml | 18 ++++++ src/nfs/nfs_kv.cpp | 10 ++++ src/nfs/nfs_kv.h | 2 + src/nfs/nfs_kv_defrag.cpp | 4 +- src/nfs/nfs_kv_readdir.cpp | 2 +- src/nfs/nfs_kv_remove.cpp | 2 +- src/nfs/nfs_kv_rename.cpp | 2 +- tests/run_tests.sh | 1 + tests/test_nfs_list_bounds.sh | 103 ++++++++++++++++++++++++++++++++++ 9 files changed, 139 insertions(+), 5 deletions(-) create mode 100755 tests/test_nfs_list_bounds.sh diff --git a/.gitea/workflows/test.yml b/.gitea/workflows/test.yml index 896c36ff..a04d91a8 100644 --- a/.gitea/workflows/test.yml +++ b/.gitea/workflows/test.yml @@ -2471,6 +2471,24 @@ jobs: echo "" done + test_nfs_list_bounds: + runs-on: ubuntu-latest + needs: build + container: ${{env.TEST_IMAGE}}:${{github.sha}} + steps: + - name: Run test + id: test + timeout-minutes: 3 + run: /root/vitastor/tests/test_nfs_list_bounds.sh + - name: Print logs + if: always() && steps.test.outcome == 'failure' + run: | + for i in /root/vitastor/testdata/*.log /root/vitastor/testdata/*.txt; do + echo "-------- $i --------" + cat $i + echo "" + done + test_write_encrypted: runs-on: ubuntu-latest needs: build diff --git a/src/nfs/nfs_kv.cpp b/src/nfs/nfs_kv.cpp index bd12ff5f..15ba6497 100644 --- a/src/nfs/nfs_kv.cpp +++ b/src/nfs/nfs_kv.cpp @@ -110,6 +110,16 @@ std::string kv_direntry_key(uint64_t dir_ino, const std::string & filename) return (char*)key + filename; } +std::string kv_prefix_end(const std::string & prefix) +{ + std::string end = prefix; + while (end.size() && (uint8_t)end.back() == 0xff) + end.pop_back(); + if (end.size()) + end.back()++; + return end; +} + std::string kv_direntry_filename(const std::string & key) { // decode as: d / diff --git a/src/nfs/nfs_kv.h b/src/nfs/nfs_kv.h index ec0c2d97..240de67f 100644 --- a/src/nfs/nfs_kv.h +++ b/src/nfs/nfs_kv.h @@ -130,6 +130,8 @@ int kv_map_type(const std::string & type); fattr3 get_kv_attributes(nfs_proxy_t *proxy, uint64_t ino, json11::Json attrs); std::string kv_direntry_key(uint64_t dir_ino, const std::string & filename); std::string kv_direntry_filename(const std::string & key); +// The first key after all keys starting with prefix, to stop a listing at the end of the prefix +std::string kv_prefix_end(const std::string & prefix); std::string kv_inode_prefix_key(uint64_t ino, const char *prefix); std::string kv_inode_key(uint64_t ino); uint64_t kv_key_inode(const std::string & key, int prefix_len = 1); diff --git a/src/nfs/nfs_kv_defrag.cpp b/src/nfs/nfs_kv_defrag.cpp index 86bc8c7a..1d1f36d6 100644 --- a/src/nfs/nfs_kv_defrag.cpp +++ b/src/nfs/nfs_kv_defrag.cpp @@ -317,7 +317,7 @@ void kv_fs_defrag_all_t::run(int st) else if (st == 6) goto resume_6; clock_gettime(CLOCK_REALTIME, &now); - list_shared = proxy->db->list_start("shared"); + list_shared = proxy->db->list_start("shared", kv_prefix_end("shared")); proxy->db->list_next(list_shared, [this](int res, const std::string & key, const std::string & value) { if (res == -ENOENT || key.substr(0, 6) != "shared") @@ -482,7 +482,7 @@ void kv_fs_state_t::upgrade_db(std::function cb) return; } // Create missing shared inode index keys - auto list_inodes = proxy->db->list_start("i"); + auto list_inodes = proxy->db->list_start("i", kv_prefix_end("i")); proxy->db->list_next(list_inodes, [=](int res, const std::string & key, const std::string & value) { if (res == -ENOENT || key.substr(0, 1) != "i" || key == "id") diff --git a/src/nfs/nfs_kv_readdir.cpp b/src/nfs/nfs_kv_readdir.cpp index e726e46f..02b9b384 100644 --- a/src/nfs/nfs_kv_readdir.cpp +++ b/src/nfs/nfs_kv_readdir.cpp @@ -241,7 +241,7 @@ resume_2: st->cookieverf = ((uint64_t)lrand48() | ((uint64_t)lrand48() << 31) | ((uint64_t)lrand48() << 62)); } st->getattr_cur = st->entries.size(); - st->list_handle = st->self->parent->db->list_start(st->start); + st->list_handle = st->self->parent->db->list_start(st->start, kv_prefix_end(st->prefix)); st->self->parent->db->list_next(st->list_handle, [=](int res, const std::string & key, const std::string & value) { st->res = res; diff --git a/src/nfs/nfs_kv_remove.cpp b/src/nfs/nfs_kv_remove.cpp index fa70dd96..69d83e15 100644 --- a/src/nfs/nfs_kv_remove.cpp +++ b/src/nfs/nfs_kv_remove.cpp @@ -181,7 +181,7 @@ resume_3: if (st->is_rmdir) { // (4) Check if the directory actually is not empty - st->list_handle = st->self->parent->db->list_start(kv_direntry_key(st->ino, "")); + st->list_handle = st->self->parent->db->list_start(kv_direntry_key(st->ino, ""), kv_prefix_end(kv_direntry_key(st->ino, ""))); st->self->parent->db->list_next(st->list_handle, [st](int res, const std::string & key, const std::string & value) { st->res = res; diff --git a/src/nfs/nfs_kv_rename.cpp b/src/nfs/nfs_kv_rename.cpp index 72b6236b..69c4437d 100644 --- a/src/nfs/nfs_kv_rename.cpp +++ b/src/nfs/nfs_kv_rename.cpp @@ -138,7 +138,7 @@ resume_2: { // Check that the destination directory is empty st->new_dir_prefix = kv_direntry_key(st->new_direntry["ino"].uint64_value(), ""); - st->list_handle = st->self->parent->db->list_start(st->new_dir_prefix); + st->list_handle = st->self->parent->db->list_start(st->new_dir_prefix, kv_prefix_end(st->new_dir_prefix)); st->self->parent->db->list_next(st->list_handle, [st](int res, const std::string & key, const std::string & value) { st->res = res; diff --git a/tests/run_tests.sh b/tests/run_tests.sh index d3a07ba9..9e7b6332 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -185,6 +185,7 @@ TEST_NAME=old_csum_4k OLD=1 OSD_ARGS="--data_csum_type crc32c" OFFSET_ARGS= ./test_nfs.sh ./test_nfs_unaligned_append.sh ./test_nfs_long_readdir.sh +./test_nfs_list_bounds.sh ./test_write_encrypted.sh SCHEME=ec ./test_write_encrypted.sh diff --git a/tests/test_nfs_list_bounds.sh b/tests/test_nfs_list_bounds.sh new file mode 100755 index 00000000..565aab3d --- /dev/null +++ b/tests/test_nfs_list_bounds.sh @@ -0,0 +1,103 @@ +#!/bin/bash -ex +# Test that RMDIR does not slow down after many K/V keys were created and deleted, +# and that RENAME of a directory over an empty directory succeeds. +# +# K/V blocks emptied by deletes are never freed. The check that a directory is empty lists +# its entries, and the listing used to go on past the end of the directory through all +# empty blocks up to the next existing key. The keys below are created and deleted right +# after the entries of the newest directories (dD... with 13 hex digits of inode number in +# pool 1), so the emptied blocks are exactly where these listings ran. + +PG_COUNT=16 +. `dirname $0`/run_3osds.sh + +DIR_COUNT=50 +KEY_COUNT=10000 + +$VITASTOR_CLI create -s 10G fsmeta +$VITASTOR_CLI modify-pool --used-for-app fs:fsmeta testpool + +start_nfs() +{ + build/src/nfs/vitastor-nfs --config_path $VITASTOR_CFG start --fs fsmeta \ + --portmap 0 --port 2050 --foreground 1 >>./testdata/nfs.log 2>&1 & + NFS_PID=$! + for i in $(seq 1 30); do + sudo mount localhost:/ ./testdata/nfs -o port=2050,mountport=2050,nfsvers=3,soft,nolock,tcp && return 0 + sleep 1 + done + format_error "Failed to mount vitastor-nfs" +} + +stop_nfs() +{ + sudo umount ./testdata/nfs/ + kill $NFS_PID + wait $NFS_PID || true +} + +# Time RMDIR of $DIR_COUNT empty directories, in milliseconds +rmdir_ms() +{ + local dir=$MNT/$1 start + mkdir $dir + for i in $(seq 1 $DIR_COUNT); do + mkdir $dir/d$i + done + start=$(date +%s%N) + for i in $(seq 1 $DIR_COUNT); do + rmdir $dir/d$i + done + echo $(( ($(date +%s%N) - start) / 1000000 )) + rmdir $dir +} + +mkdir -p testdata/nfs +MNT=$(pwd)/testdata/nfs +trap "sudo umount -f $MNT"' || true; kill -9 $(jobs -p)' EXIT + +start_nfs +BEFORE=$(rmdir_ms before) +stop_nfs + +# Create and delete keys with 400-byte values after the entries of all directories +VALUE=$(printf 'x%.0s' $(seq 1 400)) +( + for i in $(seq 1 $KEY_COUNT); do + echo "set dF100000000000000/k$i $VALUE" + done + for i in $(seq 1 $KEY_COUNT); do + echo "del dF100000000000000/k$i" + done +) | build/src/kv/vitastor-kv --config_path $VITASTOR_CFG fsmeta > ./testdata/kv_churn.log 2>&1 +if [ "$(grep -c '^OK' ./testdata/kv_churn.log)" -ne $((KEY_COUNT*2)) ]; then + format_error "Failed to create and delete $KEY_COUNT keys" +fi + +start_nfs +AFTER=$(rmdir_ms after) +if [ $AFTER -gt $((BEFORE*2)) ]; then + format_error "RMDIR of $DIR_COUNT directories took $AFTER ms after creating and deleting $KEY_COUNT keys, $BEFORE ms before" +fi +format_green "RMDIR of $DIR_COUNT directories: $BEFORE ms before, $AFTER ms after creating and deleting $KEY_COUNT keys" +# READDIR still returns exactly the entries of a directory +mkdir $MNT/list +for i in $(seq 1 20); do + mkdir $MNT/list/e$i +done +if [ "$(ls -f $MNT/list | grep -c '^e')" -ne 20 ]; then + format_error "READDIR returned a wrong number of entries" +fi +# RENAME of a directory over an empty directory succeeds (the emptiness check used to take +# the first key after the prefix, which belongs to another directory, for an entry) +mkdir $MNT/list/e1/x +if ! mv -T $MNT/list/e1 $MNT/list/e2; then + format_error "RENAME of a directory over an empty directory failed" +fi +if [ ! -d $MNT/list/e2/x ]; then + format_error "RENAME of a directory over an empty directory lost its contents" +fi +stop_nfs + + +format_green OK -- 2.47.3