[PATCHES BUG 13446] smbd: Cache dfree information based on query path
Jeremy Allison
jra at samba.org
Fri May 25 19:52:15 UTC 2018
On Thu, May 24, 2018 at 04:15:16PM -0700, Christof Schmitt wrote:
> Ok, thank you. See the attached updated patches.
LGTM Christof RB+ and pushed ! Thanks.
> 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