[PATCHES BUG 13446] smbd: Cache dfree information based on query path
Christof Schmitt
cs at samba.org
Thu May 24 23:15:16 UTC 2018
On Wed, May 23, 2018 at 04:19:11PM -0700, Jeremy Allison wrote:
> On Wed, May 23, 2018 at 01:37:52PM -0700, Christof Schmitt wrote:
> > Here is the updated patch series including a test. The last patch should
> > not be backported as it would change the VFS ABI.
> >
> > Could you review this one?
>
> Firstly, you've got some trailing whitespace (git-hook caught that for me).
The same hook is active here, and i can trigger it by adding whitespace.
Not sure what slipped through. I reviewed the patches again and remove
one trailing whitespace at the end of selftest/knownfail, maybe that was
it.
>
> In:
>
> uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
> uint64_t *bsize, uint64_t *dfree, uint64_t *dsize)
> {
> int dfree_cache_time = lp_dfree_cache_time(SNUM(conn));
> - struct dfree_cached_info *dfc = conn->dfree_info;
> + struct dfree_cached_info *dfc = NULL, dfc_new;
> uint64_t dfree_ret;
> + char tmpbuf[PATH_MAX], *full_path, *to_free, *key_path;
> + size_t len;
> + DATA_BLOB key, value;
>
> ....
>
> Please split the new variables onto separate lines and initialize to NULL, e.g.
Done.
>
> + struct dfree_cached_info *dfc = NULL
> + struct dfree_cached_info dfc_new;
> + char tmpbuf[PATH_MAX];
> + char *full_path = NULL;
> + char *to_free = NULL;
> + char *key_path = NULL;
>
> Ensure dfc_new is zeroed before use:
Done, i added the C99 initializer instead of ZERO_STRUCT.
>
> + DBG_DEBUG("Creating dfree cache entry for %s\n", key_path);
> + ZERO_STRUCT(dfc_new); <-- add this.
> + dfc_new.bsize = *bsize;
> + dfc_new.dfree = *dfree;
> + dfc_new.dsize = *dsize;
>
> This section:
>
> + if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
> + /*
> + * In case of a file use the parent directory to reduce number
> + * of cache entries.
> + */
> + bool ok;
> +
> + ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
> + if (!ok) {
> + errno = ENOMEM;
> + return -1;
> + }
> +
> + } else {
> + key_path = full_path;
> + }
>
> Made me think. In the 'if (!ok)' error case you need to TALLOC_FREE(to_free),
> also, parent_dirname() will *always* allocate key_path, meaning on normal
> exit on this case you need to TALOC_FREE(to_free), but you're not freeing key_path.
>
> As you're done with 'to_free' if parent_dirname returns true, you might
> want to change this to:
>
> + if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
> + /*
> + * In case of a file use the parent directory to reduce number
> + * of cache entries.
> + */
> + bool ok;
> +
> + ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
> + if (!ok) {
> + TALLOC_FREE(to_free);
> + errno = ENOMEM;
> + return -1;
> + }
> + TALLOC_FREE(to_free); /* We're done with full_path */
> + /* key_path is always a talloced object. */
> + to_free = key_path;
> +
> + } else {
> + /* key_path might not be a talloced object. */
> + key_path = full_path;
> + }
>
> That way you always exit with only *one* thing to free -> 'to_free'
> and you note what paths talloc and what paths might not.
>
> The reason I bring this up is in one path, key_path is a talloced
> object, but in the other key_path could be pointing to the stack
> allocated char tmpbuf[PATH_MAX] (probably will in fact).
>
> I just worry in the future someone might add a TALLOC_FREE(key_path)
> without reading or understanding the code carefully enough.
Done, i changed the logic accordingly.
> In the last patch where you move the struct dfree_cached_info *dfree_info,
> add a comment to source3/include/vfs.h noting you've changed the VFS ABI,
> e.g.
>
> /* Version 39 - Remove SMB_VFS_FSYNC
> Only implement async versions. */
> /* Version 39 - Remove SMB_VFS_READ
> All users are now pread or async versions. */
> /* Version 39 - Remove SMB_VFS_WRITE
> All users are now pwrite or async versions. */
> + /* Version 39 - Remove struct dfree_cached_info pointer from
> + connection struct */
>
> #define SMB_VFS_INTERFACE_VERSION 39
Done, added for the last patch.
> Otherwise looks good ! Can you re-do those and resend to me,
> and I'll push if happy.
Ok, thank you. See the attached updated patches.
Christof
-------------- next part --------------
From 7d21d9755c5bd00ee4462cc8b2ebcbdc911cefc8 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 23 May 2018 11:07:54 -0700
Subject: [PATCH 1/6] selftest: Add dfq_cache share with 'dfree cache time' set
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13446
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/target/Samba3.pm | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 2b1752f..c6ae7ab 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2116,6 +2116,12 @@ sub provision($$$$$$$$$)
vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq
admin users = $unix_name
include = $dfqconffile
+[dfq_cache]
+ path = $shrdir/dfree
+ vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq
+ admin users = $unix_name
+ include = $dfqconffile
+ dfree cache time = 60
[dfq_owner]
path = $shrdir/dfree
vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq
--
1.8.3.1
From c2a991f13d80748df8f3c39e91744a3ee92576d8 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 23 May 2018 11:25:42 -0700
Subject: [PATCH 2/6] selftest: Add test for 'dfree cache'
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13446
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/knownfail | 1 +
source3/script/tests/test_dfree_quota.sh | 35 ++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index a2aeed2..b2839ae 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -343,3 +343,4 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
+^samba3.blackbox.dfree_quota \(fileserver\).Test dfree cache\(fileserver\)
diff --git a/source3/script/tests/test_dfree_quota.sh b/source3/script/tests/test_dfree_quota.sh
index 6e227c4..abd82b4 100755
--- a/source3/script/tests/test_dfree_quota.sh
+++ b/source3/script/tests/test_dfree_quota.sh
@@ -130,6 +130,35 @@ test_smbclient_dfree() {
return $status
}
+# Issue two queries to different directories in one session to test
+# caching effects
+test_smbclient_dfree_2() {
+ name="$1"
+ share="$2"
+ dir1="$3"
+ dir2="$4"
+ confs="$5"
+ expected="$6"
+ subunit_start_test "$name"
+ setup_conf $confs
+ output=$($VALGRIND $smbclient //$SERVER/$share \
+ -c "cd $dir1; du; cd ..; cd $dir2 ; du" $@ 2>&1)
+ status=$?
+ if [ "$status" = "0" ]; then
+ received=$(echo "$output" | \
+ awk '/blocks of size/ {print $1, $5, $6}' | \
+ tr '\n' ' ')
+ if [ "$expected" = "$received" ]; then
+ subunit_pass_test "$name"
+ else
+ echo "$output" | subunit_fail_test "$name"
+ fi
+ else
+ echo "$output" | subunit_fail_test "$name"
+ fi
+ return $status
+}
+
test_smbcquotas() {
name="$1"
conf="$2"
@@ -164,6 +193,12 @@ test_smbclient_dfree "Test large disk" dfq "." "conf3 ." "1125899906842624 1024.
#basic quota test (SMB1 only)
test_smbcquotas "Test user quota" confq1 $USERNAME "40960/4096000/3072000" -U$USERNAME%$PASSWORD --option=clientmaxprotocol=NT1 || failed=`expr $failed + 1`
+# Test dfree cache through queries in two different directories
+test_smbclient_dfree_2 "Test dfree cache" dfq_cache "." "subdir1" \
+ "conf1 . conf2 subdir1" "10 1024. 5 20 1024. 10 " \
+ -U$USERNAME%$PASSWORD --option=clientmaxprotocol=SMB3 \
+ || failed=`expr $failed + 1`
+
#quota limit > disk size, remaining quota > disk free
test_smbclient_dfree "Test dfree share root df vs quota case 1" dfq "." "confdfq1 ." "80 1024. 40" -U$USERNAME%$PASSWORD --option=clientmaxprotocol=SMB3 || failed=`expr $failed + 1`
#quota limit > disk size, remaining quota < disk free
--
1.8.3.1
From f5a5e151f38ed3e08e419ec6984b4db92b59b78a Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 16 May 2018 13:05:36 -0700
Subject: [PATCH 3/6] memcache: Add new cache type for dfree information
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13446
Signed-off-by: Christof Schmitt <cs at samba.org>
---
lib/util/memcache.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/util/memcache.h b/lib/util/memcache.h
index 670cbd6..4331c2f 100644
--- a/lib/util/memcache.h
+++ b/lib/util/memcache.h
@@ -45,7 +45,8 @@ enum memcache_number {
SINGLETON_CACHE,
SMB1_SEARCH_OFFSET_MAP,
SHARE_MODE_LOCK_CACHE, /* talloc */
- VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC /* talloc */
+ VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC, /* talloc */
+ DFREE_CACHE,
};
/*
--
1.8.3.1
From 7c056cb852158061599fc21c625d6296efbdfd57 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 16 May 2018 13:17:52 -0700
Subject: [PATCH 4/6] smbd: Cache dfree information based on query path
Sub directories in a SMB share can have different free space information
(e.g. when a different file system is mounted there). Caching the dfree
information per SMB share will return invalid data. Address this by
switching to memcache and store the cached data based on the query path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13446
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/knownfail | 1 -
source3/smbd/dfree.c | 99 ++++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 80 insertions(+), 20 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index b2839ae..a2aeed2 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -343,4 +343,3 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
-^samba3.blackbox.dfree_quota \(fileserver\).Test dfree cache\(fileserver\)
diff --git a/source3/smbd/dfree.c b/source3/smbd/dfree.c
index a702d08..5b20707 100644
--- a/source3/smbd/dfree.c
+++ b/source3/smbd/dfree.c
@@ -21,6 +21,7 @@
#include "smbd/smbd.h"
#include "smbd/globals.h"
#include "lib/util_file.h"
+#include "lib/util/memcache.h"
/****************************************************************************
Normalise for DOS usage.
@@ -167,48 +168,108 @@ dfree_done:
/****************************************************************************
Potentially returned cached dfree info.
+
+ Depending on the file system layout and file system features, the free space
+ information can be different for different sub directories underneath a SMB
+ share. Store the cache information in memcache using the query path as the
+ key to accomodate this.
****************************************************************************/
uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
uint64_t *bsize, uint64_t *dfree, uint64_t *dsize)
{
int dfree_cache_time = lp_dfree_cache_time(SNUM(conn));
- struct dfree_cached_info *dfc = conn->dfree_info;
+ struct dfree_cached_info *dfc = NULL;
+ struct dfree_cached_info dfc_new = { 0 };
uint64_t dfree_ret;
+ char tmpbuf[PATH_MAX];
+ char *full_path = NULL;
+ char *to_free = NULL;
+ char *key_path = NULL;
+ size_t len;
+ DATA_BLOB key, value;
+ bool found;
if (!dfree_cache_time) {
return sys_disk_free(conn, fname, bsize, dfree, dsize);
}
+ len = full_path_tos(conn->connectpath,
+ fname->base_name,
+ tmpbuf,
+ sizeof(tmpbuf),
+ &full_path,
+ &to_free);
+ if (len == -1) {
+ errno = ENOMEM;
+ return -1;
+ }
+
+ if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
+ /*
+ * In case of a file use the parent directory to reduce number
+ * of cache entries.
+ */
+ bool ok;
+
+ ok = parent_dirname(talloc_tos(),
+ full_path,
+ &key_path,
+ NULL);
+ TALLOC_FREE(to_free); /* We're done with full_path */
+
+ if (!ok) {
+ errno = ENOMEM;
+ return -1;
+ }
+
+ /*
+ * key_path is always a talloced object.
+ */
+ to_free = key_path;
+ } else {
+ /*
+ * key_path might not be a talloced object; rely on
+ * to_free set from full_path_tos.
+ */
+ key_path = full_path;
+ }
+
+ key = data_blob_const(key_path, strlen(key_path));
+ found = memcache_lookup(smbd_memcache(),
+ DFREE_CACHE,
+ key,
+ &value);
+ dfc = found ? (struct dfree_cached_info *)value.data : NULL;
+
if (dfc && (conn->lastused - dfc->last_dfree_time < dfree_cache_time)) {
- /* Return cached info. */
+ DBG_DEBUG("Returning dfree cache entry for %s\n", key_path);
*bsize = dfc->bsize;
*dfree = dfc->dfree;
*dsize = dfc->dsize;
- return dfc->dfree_ret;
+ dfree_ret = dfc->dfree_ret;
+ goto out;
}
dfree_ret = sys_disk_free(conn, fname, bsize, dfree, dsize);
if (dfree_ret == (uint64_t)-1) {
/* Don't cache bad data. */
- return dfree_ret;
- }
-
- /* No cached info or time to refresh. */
- if (!dfc) {
- dfc = talloc(conn, struct dfree_cached_info);
- if (!dfc) {
- return dfree_ret;
- }
- conn->dfree_info = dfc;
+ goto out;
}
- dfc->bsize = *bsize;
- dfc->dfree = *dfree;
- dfc->dsize = *dsize;
- dfc->dfree_ret = dfree_ret;
- dfc->last_dfree_time = conn->lastused;
-
+ DBG_DEBUG("Creating dfree cache entry for %s\n", key_path);
+ dfc_new.bsize = *bsize;
+ dfc_new.dfree = *dfree;
+ dfc_new.dsize = *dsize;
+ dfc_new.dfree_ret = dfree_ret;
+ dfc_new.last_dfree_time = conn->lastused;
+ memcache_add(smbd_memcache(),
+ DFREE_CACHE,
+ key,
+ data_blob_const(&dfc_new, sizeof(dfc_new)));
+
+out:
+ TALLOC_FREE(to_free);
return dfree_ret;
}
--
1.8.3.1
From 89396139d07f12aa430368b39b0197fa54fa2124 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 18 May 2018 20:51:58 -0700
Subject: [PATCH 5/6] smbd: Flush dfree memcache on service reload
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13446
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/smbd/dfree.c | 5 +++++
source3/smbd/proto.h | 1 +
source3/smbd/server_reload.c | 1 +
3 files changed, 7 insertions(+)
diff --git a/source3/smbd/dfree.c b/source3/smbd/dfree.c
index 5b20707..d280e1e 100644
--- a/source3/smbd/dfree.c
+++ b/source3/smbd/dfree.c
@@ -273,3 +273,8 @@ out:
TALLOC_FREE(to_free);
return dfree_ret;
}
+
+void flush_dfree_cache(void)
+{
+ memcache_flush(smbd_memcache(), DFREE_CACHE);
+}
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 778561c..399be83 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -173,6 +173,7 @@ uint64_t sys_disk_free(connection_struct *conn, struct smb_filename *fname,
uint64_t *bsize, uint64_t *dfree, uint64_t *dsize);
uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
uint64_t *bsize, uint64_t *dfree, uint64_t *dsize);
+void flush_dfree_cache(void);
/* The following definitions come from smbd/dir.c */
diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index c93c077..9b62096 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c
@@ -164,6 +164,7 @@ bool reload_services(struct smbd_server_connection *sconn,
mangle_reset_cache();
reset_stat_cache();
+ flush_dfree_cache();
/* this forces service parameters to be flushed */
set_current_service(NULL,0,True);
--
1.8.3.1
From 06462fa19b522ab38587af3506d094aef0adb447 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 16 May 2018 13:25:54 -0700
Subject: [PATCH 6/6] smbd: Move dfree_info struct
As the struct is no longer used as part of connection_struct, move it to
dfree.c.
This is not backported, as it would change the VFS ABI.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/include/vfs.h | 11 ++---------
source3/smbd/dfree.c | 8 ++++++++
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/source3/include/vfs.h b/source3/include/vfs.h
index cc7948a..3eba79c 100644
--- a/source3/include/vfs.h
+++ b/source3/include/vfs.h
@@ -251,6 +251,8 @@
All users are now pread or async versions. */
/* Version 39 - Remove SMB_VFS_WRITE
All users are now pwrite or async versions. */
+/* Version 39 - Remove struct dfree_cached_info pointer from
+ connection struct */
#define SMB_VFS_INTERFACE_VERSION 39
@@ -409,14 +411,6 @@ typedef struct {
bool is_wild;
} name_compare_entry;
-struct dfree_cached_info {
- time_t last_dfree_time;
- uint64_t dfree_ret;
- uint64_t bsize;
- uint64_t dfree;
- uint64_t dsize;
-};
-
struct share_params {
int service;
};
@@ -479,7 +473,6 @@ typedef struct connection_struct {
name_compare_entry *veto_list; /* Per-share list of files to veto (never show). */
name_compare_entry *veto_oplock_list; /* Per-share list of files to refuse oplocks on. */
name_compare_entry *aio_write_behind_list; /* Per-share list of files to use aio write behind on. */
- struct dfree_cached_info *dfree_info;
struct trans_state *pending_trans;
struct rpc_pipe_client *spoolss_pipe;
diff --git a/source3/smbd/dfree.c b/source3/smbd/dfree.c
index d280e1e..05f6d69 100644
--- a/source3/smbd/dfree.c
+++ b/source3/smbd/dfree.c
@@ -175,6 +175,14 @@ dfree_done:
key to accomodate this.
****************************************************************************/
+struct dfree_cached_info {
+ time_t last_dfree_time;
+ uint64_t dfree_ret;
+ uint64_t bsize;
+ uint64_t dfree;
+ uint64_t dsize;
+};
+
uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
uint64_t *bsize, uint64_t *dfree, uint64_t *dsize)
{
--
1.8.3.1
More information about the samba-technical
mailing list