[PATCHES BUG 13446] smbd: Cache dfree information based on query path
Christof Schmitt
cs at samba.org
Wed May 23 20:37:52 UTC 2018
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?
Christof
On Tue, May 22, 2018 at 04:10:58PM -0700, Christof Schmitt via samba-technical wrote:
> On Mon, May 21, 2018 at 04:25:40PM -0700, Jeremy Allison wrote:
> > On Mon, May 21, 2018 at 04:20:49PM -0700, Christof Schmitt wrote:
> > > On Mon, May 21, 2018 at 12:46:33PM -0700, Jeremy Allison wrote:
> > > > On Fri, May 18, 2018 at 08:54:26PM -0700, Christof Schmitt via samba-technical wrote:
> > > > > Forgot to include one patch to flush the cache on service reload. See
> > > > > the last patch in this series.
> > > >
> > > > Couple of comments. The key for the cache is incorrect, you're
> > > > only using fname->base_name - this can easily be duplicated across
> > > > different shares (for example, multiple shares all with a "TEMP"
> > > > sub directory).
> > > >
> > > > The cache key needs to be the absolute pathname, not the share
> > > > relative one.
> > >
> > > Right, i missed that. I will update this to get_full_smb_filename.
> >
> > Nope, that call doesn't do what you think it does :-).
> >
> > It only appends a stream name if present, not the full pathname.
> > You need to talloc_asprintf containing the vfs_GetWd() return.
>
> Of course. I copied that from vfs_gpfs, but the two falls to
> get_full_smb_filename are also wrong there. That will be a separate
> patch series to address.
>
> > > > Secondly, removing:
> > > >
> > > > - struct dfree_cached_info *dfree_info;
> > > >
> > > > from the typedef struct connection_struct { is an ABI
> > > > change for the VFS. Many VFS modules indirect into
> > > > connection_struct *conn which will break if introduced
> > > > as a back-pot for a patch.
> > > >
> > > > So this change is OK for master (but you need a comment in
> > > > source3/include/vfs.h to say you're changing the VFS interface
> > > > ABI number, or at least logging the fact you're changing it
> > > > as the number is already incremented for 4.9.0), but
> > > > this part of the patch can't be back-ported for any
> > > > released version.
> > >
> > > Ok. It still makes sense to remove the unused part for master. Maybe i
> > > will just remove the BUG line from the description, and add a comment
> > > that this should not get backported.
> >
> > In which case it doesn't really need a bug number and
> > you can just close the bug :-).
>
> I meant to not move the struct in the backport, but still fix the dfree
> cache. See attached patches. This is not tested yet; i will try to add
> some tests based on the vfs_fake_dfq module next.
>
> Christof
-------------- next part --------------
From a5633d78041bf5b6dcfc514ea76cdbc7b724da9d 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 c6bc1ebaf6d022a9b566519efd8778449a5463c4 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..2812d17 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 15a3733a96bd07f09da49676c09236a0d21c0288 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 429cef4b9fa7becc7bdac57e6d82a3caf9b9d059 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 | 73 ++++++++++++++++++++++++++++++++++++++--------------
2 files changed, 54 insertions(+), 20 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 2812d17..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..e8198c0 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,82 @@ 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, dfc_new;
uint64_t dfree_ret;
+ char tmpbuf[PATH_MAX], *full_path, *to_free, *key_path;
+ 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);
+ if (!ok) {
+ errno = ENOMEM;
+ return -1;
+ }
+
+ } else {
+ 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 9fbc7678f6259ff2e12934687cebae693f7ee085 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 e8198c0..dbc9f7d 100644
--- a/source3/smbd/dfree.c
+++ b/source3/smbd/dfree.c
@@ -247,3 +247,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 1ea7676eb68269388887c143d83040c4fb5df8f8 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 | 9 ---------
source3/smbd/dfree.c | 8 ++++++++
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/source3/include/vfs.h b/source3/include/vfs.h
index cc7948a..cdf6fb3 100644
--- a/source3/include/vfs.h
+++ b/source3/include/vfs.h
@@ -409,14 +409,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 +471,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 dbc9f7d..12bf814 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