[PATCHES BUG 13446] smbd: Cache dfree information based on query path
Christof Schmitt
cs at samba.org
Tue May 22 23:10:58 UTC 2018
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 dfb466ef2d647c5436d32379e0f36f6779255a44 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 1/4] 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 f6d953c33c8e0cdecb729d8ba9d5a25cdde4fef5 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 2/4] 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>
---
source3/smbd/dfree.c | 74 ++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 55 insertions(+), 19 deletions(-)
diff --git a/source3/smbd/dfree.c b/source3/smbd/dfree.c
index a702d08..2b9aee0 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,83 @@ 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;
+ 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;
+ char *parent_dir;
+
+ ok = parent_dirname(talloc_tos(), full_path, &parent_dir, NULL);
+ if (!ok) {
+ errno = ENOMEM;
+ return -1;
+ }
+
+ key = data_blob_const(parent_dir, strlen(parent_dir));
+ } else {
+ key = data_blob_const(full_path, strlen(full_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", full_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", full_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 e63e8df3b32c30050e03cae6ada2909d80b585ce 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 3/4] 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 2b9aee0..7994e16 100644
--- a/source3/smbd/dfree.c
+++ b/source3/smbd/dfree.c
@@ -248,3 +248,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 d662593505da135620b2412fef7e320f6ac3638c 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 4/4] 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 7994e16..ec680be 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