From e00bfe2d2c3f124b2883e3c2d169a2ce506a1e98 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 8 Mar 2024 09:50:20 +1300 Subject: [PATCH] Fix performance regression in storage list. storageListP() returns a list of entries in a path and should not need to stat/head, etc. in order to get more detailed info. This was broken by 75623d4 which failed to set the level correctly. Set the correct level and update tests. There's no easy way to directly test for a regression here but the SFTP tests will fail if more detailed info is requested since it would require script changes. --- doc/xml/release/2024/2.51.xml | 13 +++++++++++++ doc/xml/release/contributor.xml | 5 +++++ src/storage/storage.c | 4 ++-- test/src/module/storage/posixTest.c | 17 +++++++++++++++++ test/src/module/storage/sftpTest.c | 14 -------------- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/doc/xml/release/2024/2.51.xml b/doc/xml/release/2024/2.51.xml index 43ae80a2a..222f22f30 100644 --- a/doc/xml/release/2024/2.51.xml +++ b/doc/xml/release/2024/2.51.xml @@ -14,6 +14,19 @@

Skip zero-length files for block incremental delta restore.

+ + + + + + + + + + + +

Fix performance regression in storage list.

+ diff --git a/doc/xml/release/contributor.xml b/doc/xml/release/contributor.xml index fe9edaf1a..489470f5d 100644 --- a/doc/xml/release/contributor.xml +++ b/doc/xml/release/contributor.xml @@ -670,6 +670,11 @@ mahomedh + + Maksym Boguk + MaximBoguk + + Oscar MannerMan diff --git a/src/storage/storage.c b/src/storage/storage.c index cc5c95be2..543845dbf 100644 --- a/src/storage/storage.c +++ b/src/storage/storage.c @@ -378,8 +378,8 @@ storageList(const Storage *this, const String *pathExp, StorageListParam param) MEM_CONTEXT_TEMP_BEGIN() { StorageIterator *const storageItr = storageNewItrP( - this, pathExp, .errorOnMissing = param.errorOnMissing, .nullOnMissing = param.nullOnMissing, - .expression = param.expression); + this, pathExp, .level = storageInfoLevelExists, .errorOnMissing = param.errorOnMissing, + .nullOnMissing = param.nullOnMissing, .expression = param.expression); if (storageItr != NULL) { diff --git a/test/src/module/storage/posixTest.c b/test/src/module/storage/posixTest.c index db4fd5837..38ed41414 100644 --- a/test/src/module/storage/posixTest.c +++ b/test/src/module/storage/posixTest.c @@ -486,6 +486,23 @@ testRun(void) storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail; + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("path basic info - no recurse"); + + storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail; + + TEST_STORAGE_LIST( + storageTest, "pg", + "zzz/\n" + "pipe*\n" + "path/\n" + "link> {d=../file}\n" + "file {s=8, t=1656433838}\n" + "./\n", + .levelForce = true, .includeDot = true, .noRecurse = true, .sortOrder = sortOrderDesc); + + storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail; + // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("empty path - filter"); diff --git a/test/src/module/storage/sftpTest.c b/test/src/module/storage/sftpTest.c index 4846e417b..7f9008e3d 100644 --- a/test/src/module/storage/sftpTest.c +++ b/test/src/module/storage/sftpTest.c @@ -3776,15 +3776,8 @@ testRun(void) .resultInt = 8, .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG, .flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID, .mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID}, - {.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/.aaa.txt\",1]", .fileName = STRDEF(".aaa.txt"), - .resultInt = LIBSSH2_ERROR_NONE, - .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG, - .flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID, - .mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID}, {.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\".aaa.txt\",4095,null,0]", .fileName = STRDEF("noperm"), .resultInt = 6, .uid = 0, .gid = 0}, - {.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/noperm\",1]", .fileName = STRDEF("noperm"), - .resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0}, {.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"noperm\",4095,null,0]", .fileName = STRDEF(""), .resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0}, {.function = HRNLIBSSH2_SFTP_CLOSE_HANDLE, .resultInt = LIBSSH2_ERROR_NONE}, @@ -3801,15 +3794,8 @@ testRun(void) .resultInt = 7, .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG, .flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID, .mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID}, - {.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/bbb.txt\",1]", .fileName = STRDEF("bbb.txt"), - .resultInt = LIBSSH2_ERROR_NONE, - .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG, - .flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID, - .mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID}, {.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"bbb.txt\",4095,null,0]", .fileName = STRDEF("noperm"), .resultInt = 6, .uid = 0, .gid = 0}, - {.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/noperm\",1]", .fileName = STRDEF("noperm"), - .resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0}, {.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"noperm\",4095,null,0]", .fileName = STRDEF(""), .resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0}, {.function = HRNLIBSSH2_SFTP_CLOSE_HANDLE, .resultInt = LIBSSH2_ERROR_EAGAIN},