[PATCH 4/7] printing: remove pcap_cache_add()

[David Disseldorp]

All print list updates are now done via pcap_cache_replace(), which can
call into the print_list code directly.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---
source3/printing/pcap.c | 16 ++++++----------
source3/printing/pcap.h | 1 -
2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 0c4bf40..9c44584 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,15 +83,6 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

-static bool pcap_cache_add(const char *name, const char *comment, const char *location)
-{
- NTSTATUS status;
- time_t t = time_mono(NULL);
-
- status = printer_list_set_printer(talloc_tos(), name, comment, location, t);
- return NT_STATUS_IS_OK(status);
-}
-
bool pcap_cache_loaded(void)
{
NTSTATUS status;
@@ -105,6 +96,7 @@ bool pcap_cache_replace(const struct pcap_cache *pcache)
{
const struct pcap_cache *p;
NTSTATUS status;
+ time_t t = time_mono(NULL);

status = printer_list_mark_reload();
if (!NT_STATUS_IS_OK(status)) {
@@ -113,7 +105,11 @@ bool pcap_cache_replace(const struct pcap_cache *pcache)
}

for (p = pcache; p; p = p->next) {
- pcap_cache_add(p->name, p->comment, p->location);
+ status = printer_list_set_printer(talloc_tos(), p->name,
+ p->comment, p->location, t);
+ if (!NT_STATUS_IS_OK(status)) {
+ return false;
+ }
}

status = printer_list_clean_old();
diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index d388d7d..7dccf84 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -35,7 +35,6 @@ struct pcap_cache;

bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location);
void pcap_cache_destroy_specific(struct pcap_cache **ppcache);
-bool pcap_cache_add(const char *name, const char *comment, const char *location);
bool pcap_cache_loaded(void);
bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);
--
1.8.4.5

[David Disseldorp]

Currently, automatic printer share updates are handled in the following
way:
- Background printer process (BPP) forked on startup
- Parent smbd and per-client children await MSG_PRINTER_PCAP messages
- BPP periodically polls the printing backend for printcap data
- printcap data written to printer_list.tdb
- MSG_PRINTER_PCAP sent to all smbd processes following update
- smbd processes all read the latest printer_list.tdb data, and update
their share listings

This procedure is not scalable, as all smbd processes hit
printer_list.tdb in parallel, resulting in a large spike in CPU usage.

This change sees smbd processes only update their printer share lists
only when a client asks for this information, e.g. via NetShareEnum or
EnumPrinters.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Suggested-by: Volker Lendecke <v...@samba.org>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/spoolssd.c | 34 +++++------------------------
source3/rpc_server/spoolss/srv_spoolss_nt.c | 11 +++++++++-
source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 1 +
source3/smbd/server.c | 20 -----------------
4 files changed, 16 insertions(+), 50 deletions(-)

diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c
index f181fcc..afb8f4f 100644
--- a/source3/printing/spoolssd.c
+++ b/source3/printing/spoolssd.c
@@ -132,27 +132,6 @@ static void smb_conf_updated(struct messaging_context *msg,
update_conf(ev_ctx, msg);
}

-static void update_pcap(struct tevent_context *ev_ctx,
- struct messaging_context *msg_ctx)
-{
- change_to_root_user();
- delete_and_reload_printers(ev_ctx, msg_ctx);
-}
-
-static void pcap_updated(struct messaging_context *msg,
- void *private_data,
- uint32_t msg_type,
- struct server_id server_id,
- DATA_BLOB *data)
-{
- struct tevent_context *ev_ctx;
-
- ev_ctx = talloc_get_type_abort(private_data, struct tevent_context);
-
- DEBUG(10, ("Got message that pcap updated. Reloading.\n"));
- update_pcap(ev_ctx, msg);
-}
-
static void spoolss_sig_term_handler(struct tevent_context *ev,
struct tevent_signal *se,
int signum,
@@ -318,8 +297,6 @@ static bool spoolss_child_init(struct tevent_context *ev_ctx,

messaging_register(msg_ctx, ev_ctx,
MSG_SMB_CONF_UPDATED, smb_conf_updated);
- messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP,
- pcap_updated);
messaging_register(msg_ctx, ev_ctx,
MSG_PREFORK_PARENT_EVENT, parent_ping);

@@ -739,15 +716,14 @@ pid_t start_spoolssd(struct tevent_context *ev_ctx,
MSG_SMB_CONF_UPDATED, smb_conf_updated);
messaging_register(msg_ctx, NULL, MSG_PRINTER_UPDATE,
print_queue_forward);
- messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP,
- pcap_updated);
messaging_register(msg_ctx, ev_ctx,
MSG_PREFORK_CHILD_EVENT, child_ping);

- /* As soon as messaging is up check if pcap has been loaded already.
- * If so then we probably missed a message and should load_printers()
- * ourselves. If pcap has not been loaded yet, then ignore, we will get
- * a message as soon as the bq process completes the reload. */
+ /*
+ * As soon as messaging is up check if pcap has been loaded already.
+ * If pcap has not been loaded yet, then ignore, as we will reload on
+ * client enumeration anyway.
+ */
if (pcap_cache_loaded()) {
load_printers(ev_ctx, msg_ctx);
}
diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
index 760c924..b8cae89 100644
--- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
+++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
@@ -4312,7 +4312,7 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx,
uint32_t *count_p)
{
int snum;
- int n_services = lp_numservices();
+ int n_services;
union spoolss_PrinterInfo *info = NULL;
uint32_t count = 0;
WERROR result = WERR_OK;
@@ -4324,6 +4324,15 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx,
return WERR_NOMEM;
}

+ /*
+ * printer shares are only updated on client enumeration. The background
+ * printer process updates printer_list.tdb at regular intervals.
+ */
+ become_root();
+ delete_and_reload_printers(server_event_context(), msg_ctx);
+ unbecome_root();
+
+ n_services = lp_numservices();
*count_p = 0;
*info_p = NULL;

diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
index e030b98..11abc6c 100644
--- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
+++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
@@ -548,6 +548,7 @@ static WERROR init_srv_share_info_ctr(struct pipes_struct *p,

/* Ensure all the usershares are loaded. */
become_root();
+ delete_and_reload_printers(server_event_context(), p->msg_ctx);
load_usershare_shares(NULL, connections_snum_used);
load_registry_shares();
num_services = lp_numservices();
diff --git a/source3/smbd/server.c b/source3/smbd/server.c
index ec9348c..bc07c0f 100644
--- a/source3/smbd/server.c
+++ b/source3/smbd/server.c
@@ -110,24 +110,6 @@ static void smbd_parent_conf_updated(struct messaging_context *msg,
}

/*******************************************************************
- What to do when printcap is updated.
- ********************************************************************/
-
-static void smb_pcap_updated(struct messaging_context *msg,
- void *private_data,
- uint32_t msg_type,
- struct server_id server_id,
- DATA_BLOB *data)
-{
- struct tevent_context *ev_ctx =
- talloc_get_type_abort(private_data, struct tevent_context);
-
- DEBUG(10,("Got message saying pcap was updated. Reloading.\n"));
- change_to_root_user();
- delete_and_reload_printers(ev_ctx, msg);
-}
-
-/*******************************************************************
Delete a statcache entry.
********************************************************************/

@@ -894,8 +876,6 @@ static bool open_sockets_smbd(struct smbd_parent_context *parent,
messaging_register(msg_ctx, NULL, MSG_SMB_STAT_CACHE_DELETE,
smb_stat_cache_delete);
messaging_register(msg_ctx, NULL, MSG_DEBUG, smbd_msg_debug);
- messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP,
- smb_pcap_updated);
messaging_register(msg_ctx, NULL, MSG_SMB_BRL_VALIDATE,
brl_revalidate);
messaging_register(msg_ctx, NULL, MSG_SMB_FORCE_TDIS,
--
1.8.4.5

[David Disseldorp]

The printcap update procedure involves the background printer process
obtaining the printcap information from the printing backend, writing
this to printer_list.tdb, and then notifying all smbd processes of the
new list. The processes then all attempt to simultaneously traverse
printer_list.tdb, in order to update their local share lists.

With a large number of printers, and a large number of per-client smbd
processes, this traversal results in significant lock contention, mostly
due to the fact that the traversal is unnecessarily done with an
exclusive (write) lock on the printer_list.tdb database.

This commit changes the share update code path to perform a read-only
traversal.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Reported-by: Alex K <korobki...@gmail.com>
Reported-by: Franz Pförtsch <franz.p...@brose.com>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/load.c | 2 +-
source3/printing/pcap.c | 4 ++--
source3/printing/pcap.h | 2 +-
source3/printing/printer_list.c | 17 +++++++++++------
source3/printing/printer_list.h | 4 ++--
5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/source3/printing/load.c b/source3/printing/load.c
index 136d055..2ba3b2e 100644
--- a/source3/printing/load.c
+++ b/source3/printing/load.c
@@ -71,5 +71,5 @@ void load_printers(struct tevent_context *ev,

/* load all printcap printers */
if (lp_load_printers() && lp_servicenumber(PRINTERS_NAME) >= 0)
- pcap_printer_fn(lp_add_one_printer, NULL);
+ pcap_printer_read_fn(lp_add_one_printer, NULL);
}
diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index dd7ba62..25dd4c7 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -229,11 +229,11 @@ void pcap_printer_fn_specific(const struct pcap_cache *pc,
return;
}

-void pcap_printer_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata)
+void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata)
{
NTSTATUS status;

- status = printer_list_run_fn(fn, pdata);
+ status = printer_list_read_run_fn(fn, pdata);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(3, ("Failed to run fn for all printers!\n"));
}
diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 7056213..6c062c3 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -39,7 +39,7 @@ bool pcap_cache_add(const char *name, const char *comment, const char *location)

bool pcap_cache_loaded(void);
bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);

-void pcap_printer_fn(void (*fn)(const char *, const char *, const char *, void *), void *);
+void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *);

void pcap_cache_reload(struct tevent_context *ev,
struct messaging_context *msg_ctx,
diff --git a/source3/printing/printer_list.c b/source3/printing/printer_list.c
index 4a66b96..9a9fa0b 100644
--- a/source3/printing/printer_list.c
+++ b/source3/printing/printer_list.c
@@ -284,7 +284,8 @@ done:
typedef int (printer_list_trv_fn_t)(struct db_record *, void *);

static NTSTATUS printer_list_traverse(printer_list_trv_fn_t *fn,
- void *private_data)
+ void *private_data,
+ bool read_only)
{
struct db_context *db;
NTSTATUS status;
@@ -294,7 +295,11 @@ static NTSTATUS printer_list_traverse(printer_list_trv_fn_t *fn,
return NT_STATUS_INTERNAL_DB_CORRUPTION;
}

- status = dbwrap_traverse(db, fn, private_data, NULL);
+ if (read_only) {
+ status = dbwrap_traverse_read(db, fn, private_data, NULL);
+ } else {
+ status = dbwrap_traverse(db, fn, private_data, NULL);
+ }

return status;
}
@@ -364,7 +369,7 @@ NTSTATUS printer_list_clean_old(void)

state.status = NT_STATUS_OK;

- status = printer_list_traverse(printer_list_clean_fn, &state);
+ status = printer_list_traverse(printer_list_clean_fn, &state, false);
if (NT_STATUS_EQUAL(status, NT_STATUS_UNSUCCESSFUL) &&
!NT_STATUS_IS_OK(state.status)) {
status = state.status;
@@ -417,8 +422,8 @@ static int printer_list_exec_fn(struct db_record *rec, void *private_data)
return 0;
}

-NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char *, void *),
- void *private_data)
+NTSTATUS printer_list_read_run_fn(void (*fn)(const char *, const char *, const char *, void *),
+ void *private_data)
{
struct printer_list_exec_state state;
NTSTATUS status;
@@ -427,7 +432,7 @@ NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char *
state.private_data = private_data;
state.status = NT_STATUS_OK;

- status = printer_list_traverse(printer_list_exec_fn, &state);
+ status = printer_list_traverse(printer_list_exec_fn, &state, true);
if (NT_STATUS_EQUAL(status, NT_STATUS_UNSUCCESSFUL) &&
!NT_STATUS_IS_OK(state.status)) {
status = state.status;
diff --git a/source3/printing/printer_list.h b/source3/printing/printer_list.h
index fb2e007..b12c192 100644
--- a/source3/printing/printer_list.h
+++ b/source3/printing/printer_list.h
@@ -100,6 +100,6 @@ NTSTATUS printer_list_mark_reload(void);
*/
NTSTATUS printer_list_clean_old(void);

-NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char *, void *),
- void *private_data);
+NTSTATUS printer_list_read_run_fn(void (*fn)(const char *, const char *, const char *, void *),
+ void *private_data);
#endif /* _PRINTER_LIST_H_ */
--
1.8.4.5

[David Disseldorp]

This will allow in future for a single atomic printer_list.tdb update.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/pcap.c | 26 +++++++++++---------------
source3/printing/pcap.h | 8 ++++----
source3/printing/print_aix.c | 17 ++++++++++++++---
source3/printing/print_iprint.c | 16 ++++++++++------
source3/printing/print_standard.c | 8 ++++++--
source3/printing/print_svid.c | 11 +++++++----
6 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 25dd4c7..0c4bf40 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,7 +83,7 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

-bool pcap_cache_add(const char *name, const char *comment, const char *location)
+static bool pcap_cache_add(const char *name, const char *comment, const char *location)
{
NTSTATUS status;
time_t t = time_mono(NULL);
@@ -132,8 +132,8 @@ void pcap_cache_reload(struct tevent_context *ev,
{
const char *pcap_name = lp_printcapname();
bool pcap_reloaded = False;
- NTSTATUS status;
bool post_cache_fill_fn_handled = false;
+ struct pcap_cache *pcache = NULL;

DEBUG(3, ("reloading printcap cache\n"));

@@ -143,12 +143,6 @@ void pcap_cache_reload(struct tevent_context *ev,
return;
}

- status = printer_list_mark_reload();
- if (!NT_STATUS_IS_OK(status)) {
- DEBUG(0, ("Failed to mark printer list for reload!\n"));
- return;
- }
-
#ifdef HAVE_CUPS
if (strequal(pcap_name, "cups")) {
pcap_reloaded = cups_cache_reload(ev, msg_ctx,
@@ -164,26 +158,26 @@ void pcap_cache_reload(struct tevent_context *ev,

#ifdef HAVE_IPRINT
if (strequal(pcap_name, "iprint")) {
- pcap_reloaded = iprint_cache_reload();
+ pcap_reloaded = iprint_cache_reload(&pcache);
goto done;
}
#endif

#if defined(SYSV) || defined(HPUX)
if (strequal(pcap_name, "lpstat")) {
- pcap_reloaded = sysv_cache_reload();
+ pcap_reloaded = sysv_cache_reload(&pcache);
goto done;
}
#endif

#ifdef AIX
if (strstr_m(pcap_name, "/qconfig") != NULL) {
- pcap_reloaded = aix_cache_reload();
+ pcap_reloaded = aix_cache_reload(&pcache);
goto done;
}
#endif

- pcap_reloaded = std_pcap_cache_reload(pcap_name);
+ pcap_reloaded = std_pcap_cache_reload(pcap_name, &pcache);

done:
DEBUG(3, ("reload status: %s\n", (pcap_reloaded) ? "ok" : "error"));
@@ -192,14 +186,16 @@ done:
/* cleanup old entries only if the operation was successful,
* otherwise keep around the old entries until we can
* successfully reload */
- status = printer_list_clean_old();
- if (!NT_STATUS_IS_OK(status)) {
- DEBUG(0, ("Failed to cleanup printer list!\n"));
+
+ if (!pcap_cache_replace(pcache)) {
+ DEBUG(0, ("Failed to replace printer list!\n"));
}
+
if (post_cache_fill_fn != NULL) {
post_cache_fill_fn(ev, msg_ctx);
}
}
+ pcap_cache_destroy_specific(&pcache);

return;
}
diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 6c062c3..d388d7d 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -49,7 +49,7 @@ bool pcap_printername_ok(const char *printername);

/* The following definitions come from printing/print_aix.c */

-bool aix_cache_reload(void);
+bool aix_cache_reload(struct pcap_cache **_pcache);

/* The following definitions come from printing/print_cups.c */

@@ -60,13 +60,13 @@ bool cups_cache_reload(struct tevent_context *ev,

/* The following definitions come from printing/print_iprint.c */

-bool iprint_cache_reload(void);
+bool iprint_cache_reload(struct pcap_cache **_pcache);

/* The following definitions come from printing/print_svid.c */

-bool sysv_cache_reload(void);
+bool sysv_cache_reload(struct pcap_cache **_pcache);

/* The following definitions come from printing/print_standard.c */
-bool std_pcap_cache_reload(const char *pcap_name);
+bool std_pcap_cache_reload(const char *pcap_name, struct pcap_cache **_pcache);

#endif /* _PRINTING_PCAP_H_ */
diff --git a/source3/printing/print_aix.c b/source3/printing/print_aix.c
index 23d9a86..927a71b 100644
--- a/source3/printing/print_aix.c
+++ b/source3/printing/print_aix.c
@@ -29,12 +29,13 @@
#include "printing/pcap.h"

#ifdef AIX
-bool aix_cache_reload(void)
+bool aix_cache_reload(struct pcap_cache **_pcache)
{
int iEtat;
XFILE *pfile;
char *line = NULL, *p;
char *name = NULL;
+ struct pcap_cache *pcache = NULL;
TALLOC_CTX *ctx = talloc_init("aix_cache_reload");

if (!ctx) {
@@ -52,6 +53,8 @@ bool aix_cache_reload(void)
iEtat = 0;
/* scan qconfig file for searching <printername>: */
for (;(line = fgets_slash(NULL, 1024, pfile)); free(line)) {
+ bool ok;
+
if (*line == '*' || *line == 0)
continue;

@@ -67,6 +70,7 @@ bool aix_cache_reload(void)
if (strcmp(p, "bsh") != 0) {
name = talloc_strdup(ctx, p);
if (!name) {
+ pcap_cache_destroy_specific(&pcache);
SAFE_FREE(line);
x_fclose(pfile);
TALLOC_FREE(ctx);
@@ -86,7 +90,10 @@ bool aix_cache_reload(void)
/* name is found without stanza device */
/* probably a good printer ??? */
iEtat = 0;
- if (!pcap_cache_add(name, NULL, NULL)) {
+ ok = pcap_cache_add_specific(&pcache,
+ name, NULL, NULL);
+ if (!ok) {
+ pcap_cache_destroy_specific(&pcache);
SAFE_FREE(line);
x_fclose(pfile);
TALLOC_FREE(ctx);
@@ -101,7 +108,10 @@ bool aix_cache_reload(void)
} else if (strstr_m(line, "device")) {
/* it's a good virtual printer */
iEtat = 0;
- if (!pcap_cache_add(name, NULL, NULL)) {
+ ok = pcap_cache_add_specific(&pcache,
+ name, NULL, NULL);
+ if (!ok) {
+ pcap_cache_destroy_specific(&pcache);
SAFE_FREE(line);
x_fclose(pfile);
TALLOC_FREE(ctx);
@@ -113,6 +123,7 @@ bool aix_cache_reload(void)
}
}

+ *_pcache = pcache;
x_fclose(pfile);
TALLOC_FREE(ctx);
return true;
diff --git a/source3/printing/print_iprint.c b/source3/printing/print_iprint.c
index ad61a0a..eeb193c 100644
--- a/source3/printing/print_iprint.c
+++ b/source3/printing/print_iprint.c
@@ -206,7 +206,8 @@ static int iprint_get_server_version(http_t *http, char* serviceUri)

static int iprint_cache_add_printer(http_t *http,
int reqId,
- char* url)
+ char *url,
+ struct pcap_cache **pcache)
{
ipp_t *request = NULL, /* IPP Request */
*response = NULL; /* IPP Response */
@@ -342,7 +343,7 @@ static int iprint_cache_add_printer(http_t *http,
*/

if (name != NULL && !secure && smb_enabled)
- pcap_cache_add(name, info, NULL);
+ pcap_cache_add_specific(pcache, name, info, NULL);
}

out:
@@ -351,7 +352,7 @@ static int iprint_cache_add_printer(http_t *http,
return(0);
}

-bool iprint_cache_reload(void)
+bool iprint_cache_reload(struct pcap_cache **_pcache)
{
http_t *http = NULL; /* HTTP connection to server */
ipp_t *request = NULL, /* IPP Request */
@@ -359,7 +360,8 @@ bool iprint_cache_reload(void)
ipp_attribute_t *attr; /* Current attribute */
cups_lang_t *language = NULL; /* Default language */
int i;
- bool ret = False;
+ bool ret = false;
+ struct pcap_cache *pcache = NULL;

DEBUG(5, ("reloading iprint printcap cache\n"));

@@ -441,14 +443,16 @@ bool iprint_cache_reload(void)
char *url = ippGetString(attr, i, NULL);
if (!url || !strlen(url))
continue;
- iprint_cache_add_printer(http, i+2, url);
+ iprint_cache_add_printer(http, i+2, url,
+ &pcache);
}
}
attr = ippNextAttribute(response);
}
}

- ret = True;
+ ret = true;
+ *_pcache = pcache;

out:
if (response)
diff --git a/source3/printing/print_standard.c b/source3/printing/print_standard.c
index c4f9c5b..b5f1056 100644
--- a/source3/printing/print_standard.c
+++ b/source3/printing/print_standard.c
@@ -59,10 +59,11 @@
#include "printing/pcap.h"

/* handle standard printcap - moved from pcap_printer_fn() */
-bool std_pcap_cache_reload(const char *pcap_name)
+bool std_pcap_cache_reload(const char *pcap_name, struct pcap_cache **_pcache)
{
XFILE *pcap_file;
char *pcap_line;
+ struct pcap_cache *pcache = NULL;

if ((pcap_file = x_fopen(pcap_name, O_RDONLY, 0)) == NULL) {
DEBUG(0, ("Unable to open printcap file %s for read!\n", pcap_name));
@@ -117,12 +118,15 @@ bool std_pcap_cache_reload(const char *pcap_name)
}
}

- if (*name && !pcap_cache_add(name, comment, NULL)) {
+ if ((*name != '\0')
+ && !pcap_cache_add_specific(&pcache, name, comment, NULL)) {
x_fclose(pcap_file);
+ pcap_cache_destroy_specific(&pcache);
return false;
}
}

x_fclose(pcap_file);
+ *_pcache = pcache;
return true;
}
diff --git a/source3/printing/print_svid.c b/source3/printing/print_svid.c
index 2226493..879661b 100644
--- a/source3/printing/print_svid.c
+++ b/source3/printing/print_svid.c
@@ -35,10 +35,11 @@
#include "printing/pcap.h"

#if defined(SYSV) || defined(HPUX)
-bool sysv_cache_reload(void)
+bool sysv_cache_reload(struct pcap_cache **_pcache)
{
char **lines;
int i;
+ struct pcap_cache *pcache = NULL;

#if defined(HPUX)
DEBUG(5, ("reloading hpux printcap cache\n"));
@@ -111,14 +112,16 @@ bool sysv_cache_reload(void)
*tmp = '\0';

/* add it to the cache */
- if (!pcap_cache_add(name, NULL, NULL)) {
+ if (!pcap_cache_add_specific(&pcache, name, NULL, NULL)) {
TALLOC_FREE(lines);
- return False;
+ pcap_cache_destroy_specific(&pcache);
+ return false;
}
}

TALLOC_FREE(lines);
- return True;
+ *_pcache = pcache;
+ return true;
}

#else
--
1.8.4.5

[David Disseldorp]

The per-client smbd printer share inventory is currently updated from
printer_list.tdb when a client enumerates printers, via EnumPrinters or
NetShareEnum.
printer_list.tdb is populated by the background print process, based on
the latest printcap values retrieved from the printing backend (e.g.
CUPS) at regular intervals.
This change ensures that per-client smbd processes don't reparse
printer_list.tdb if it hasn't been updated since the last enumeration.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Suggested-by: Volker Lendecke <v...@samba.org>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/smbd/server_reload.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index e1e3f9a..ad148e1 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c
@@ -31,6 +31,13 @@
#include "messages.h"
#include "lib/param/loadparm.h"

+/*
+ * The persistent pcap cache is populated by the background print process. Per
+ * client smbds should only reload their printer share inventories if this
+ * information has changed. Use reload_last_pcap_time to detect this.
+ */
+static time_t reload_last_pcap_time = 0;
+
static bool snum_is_shared_printer(int snum)
{
return (lp_browseable(snum) && lp_snum_ok(snum) && lp_printable(snum));
@@ -61,6 +68,20 @@ void delete_and_reload_printers(struct tevent_context *ev,
const char *pname;
const char *sname;
NTSTATUS status;
+ bool ok;
+ time_t pcap_last_update;
+
+ ok = pcap_cache_loaded(&pcap_last_update);
+ if (!ok) {
+ DEBUG(1, ("pcap cache not loaded\n"));
+ return;
+ }
+
+ if (reload_last_pcap_time == pcap_last_update) {
+ DEBUG(5, ("skipping printer reload, already up to date.\n"));
+ return;
+ }
+ reload_last_pcap_time = pcap_last_update;

/* Get pcap printers updated */
load_printers(ev, msg_ctx);
--
1.8.4.5

[David Disseldorp]

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/load.c | 2 +-
source3/printing/pcap.c | 10 ++++++++--
source3/printing/pcap.h | 2 +-
source3/printing/queue_process.c | 2 +-
source3/printing/spoolssd.c | 4 ++--
5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/source3/printing/load.c b/source3/printing/load.c
index 2ba3b2e..238998d 100644
--- a/source3/printing/load.c
+++ b/source3/printing/load.c
@@ -65,7 +65,7 @@ load automatic printer services from pre-populated pcap cache
void load_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx)
{
- SMB_ASSERT(pcap_cache_loaded());
+ SMB_ASSERT(pcap_cache_loaded(NULL));

add_auto_printers();

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 9c44584..c5524ad 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,13 +83,19 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

-bool pcap_cache_loaded(void)
+bool pcap_cache_loaded(time_t *_last_change)
{
NTSTATUS status;
time_t last;

status = printer_list_get_last_refresh(&last);
- return NT_STATUS_IS_OK(status);

+ if (!NT_STATUS_IS_OK(status)) {
+ return false;
+ }

+ if (_last_change != NULL) {
+ *_last_change = last;
+ }
+ return true;

}

bool pcap_cache_replace(const struct pcap_cache *pcache)

diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 7dccf84..8fc9e9d 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -35,7 +35,7 @@ struct pcap_cache;

bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location);
void pcap_cache_destroy_specific(struct pcap_cache **ppcache);

-bool pcap_cache_loaded(void);
+bool pcap_cache_loaded(time_t *_last_change);

bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);

void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *);

diff --git a/source3/printing/queue_process.c b/source3/printing/queue_process.c
index aa0d0fb..24d361c 100644
--- a/source3/printing/queue_process.c
+++ b/source3/printing/queue_process.c
@@ -390,7 +390,7 @@ void printing_subsystem_update(struct tevent_context *ev_ctx,
bool force)
{
if (background_lpq_updater_pid != -1) {
- if (pcap_cache_loaded()) {
+ if (pcap_cache_loaded(NULL)) {
load_printers(ev_ctx, msg_ctx);
}
if (force) {
diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c
index afb8f4f..b850578 100644
--- a/source3/printing/spoolssd.c
+++ b/source3/printing/spoolssd.c
@@ -304,7 +304,7 @@ static bool spoolss_child_init(struct tevent_context *ev_ctx,

* If so then we probably missed a message and should load_printers()

* ourselves. If pcap has not been loaded yet, then ignore, we will get

* a message as soon as the bq process completes the reload. */

- if (pcap_cache_loaded()) {
+ if (pcap_cache_loaded(NULL)) {
load_printers(ev_ctx, msg_ctx);
}

@@ -724,7 +724,7 @@ pid_t start_spoolssd(struct tevent_context *ev_ctx,

* If pcap has not been loaded yet, then ignore, as we will reload on

* client enumeration anyway.
*/
- if (pcap_cache_loaded()) {
+ if (pcap_cache_loaded(NULL)) {
load_printers(ev_ctx, msg_ctx);
}

--
1.8.4.5

[David Disseldorp]

All printer inventory updates are currently done via
delete_and_reload_printers(), which handles registry.tdb updates for
added or removed printers, AD printer unpublishing on removal, as well
as share service creation and deletion.

This change splits this functionality into two functions such that
per-client smbd processes do not perform registry.tdb updates or printer
unpublishing. This is now only performed by the process that performs
the printcap cache update.

This change is similar to ac6604868d1325dd4c872dc0f6ab056d10ebaecf from
the 3.6 branch.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/queue_process.c | 8 +++--
source3/smbd/proto.h | 2 ++
source3/smbd/server_reload.c | 78 +++++++++++++++++++++++++++++++++-------
3 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/source3/printing/queue_process.c b/source3/printing/queue_process.c
index 24d361c..7c097cd 100644
--- a/source3/printing/queue_process.c
+++ b/source3/printing/queue_process.c
@@ -50,7 +50,7 @@ static void reload_pcap_change_notify(struct tevent_context *ev,
* This will block the process for some time (~1 sec per printer), but
* it doesn't block smbd's servering clients.
*/
- delete_and_reload_printers(ev, msg_ctx);
+ delete_and_reload_printers_full(ev, msg_ctx);

message_send_all(msg_ctx, MSG_PRINTER_PCAP, NULL, 0, NULL);
}
@@ -372,7 +372,8 @@ bool printing_subsystem_init(struct tevent_context *ev_ctx,
ret = printing_subsystem_queue_tasks(ev_ctx, msg_ctx);

/* Publish nt printers, this requires a working winreg pipe */
- pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers);
+ pcap_cache_reload(ev_ctx, msg_ctx,
+ &delete_and_reload_printers_full);

return ret;
}
@@ -401,5 +402,6 @@ void printing_subsystem_update(struct tevent_context *ev_ctx,
return;
}

- pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers);
+ pcap_cache_reload(ev_ctx, msg_ctx,
+ &delete_and_reload_printers_full);
}
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index eab05c2..eb529e2 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -954,6 +954,8 @@ const struct security_token *sec_ctx_active_token(void);
struct memcache *smbd_memcache(void);
void delete_and_reload_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx);
+void delete_and_reload_printers_full(struct tevent_context *ev,
+ struct messaging_context *msg_ctx);
bool reload_services(struct smbd_server_connection *sconn,
bool (*snumused) (struct smbd_server_connection *, int),
bool test);
diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index ad148e1..e6006bc 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c
@@ -44,14 +44,10 @@ static bool snum_is_shared_printer(int snum)
}

/**
- * @brief Purge stale printers and reload from pre-populated pcap cache.
+ * @brief Purge stale printer shares and reload from pre-populated pcap cache.
*
* This function should normally only be called as a callback on a successful
- * pcap_cache_reload() or after a MSG_PRINTER_CAP message is received.
- *
- * This function can cause DELETION of printers and drivers from our registry,
- * so calling it on a failed pcap reload may REMOVE permanently all printers
- * and drivers.
+ * pcap_cache_reload(), or on client enumeration.
*
* @param[in] ev The event context.
*
@@ -60,25 +56,24 @@ static bool snum_is_shared_printer(int snum)
void delete_and_reload_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx)
{
- struct auth_session_info *session_info = NULL;
- struct spoolss_PrinterInfo2 *pinfo2 = NULL;
int n_services;
int pnum;
int snum;
const char *pname;
- const char *sname;
- NTSTATUS status;
bool ok;
time_t pcap_last_update;
+ TALLOC_CTX *frame = talloc_stackframe();

ok = pcap_cache_loaded(&pcap_last_update);
if (!ok) {

DEBUG(1, ("pcap cache not loaded\n"));

+ talloc_free(frame);
return;
}

if (reload_last_pcap_time == pcap_last_update) {

DEBUG(5, ("skipping printer reload, already up to date.\n"));

+ talloc_free(frame);
return;
}
reload_last_pcap_time = pcap_last_update;
@@ -91,6 +86,64 @@ void delete_and_reload_printers(struct tevent_context *ev,

DEBUG(10, ("reloading printer services from pcap cache\n"));

+ /*
+ * Add default config for printers added to smb.conf file and remove
+ * stale printers
+ */
+ for (snum = 0; snum < n_services; snum++) {
+ /* avoid removing PRINTERS_NAME */
+ if (snum == pnum) {
+ continue;
+ }
+
+ /* skip no-printer services */
+ if (!snum_is_shared_printer(snum)) {
+ continue;
+ }
+
+ pname = lp_printername(frame, snum);
+
+ /* check printer, but avoid removing non-autoloaded printers */
+ if (lp_autoloaded(snum) && !pcap_printername_ok(pname)) {
+ lp_killservice(snum);
+ }
+ }
+
+ /* Make sure deleted printers are gone */
+ load_printers(ev, msg_ctx);
+
+ talloc_free(frame);
+}
+
+/**
+ * @brief Purge stale printers and reload from pre-populated pcap cache.
+ *
+ * This function should normally only be called as a callback on a successful
+ * pcap_cache_reload().
+ *
+ * This function can cause DELETION of printers and drivers from our registry,
+ * so calling it on a failed pcap reload may REMOVE permanently all printers
+ * and drivers.
+ *
+ * @param[in] ev The event context.
+ *
+ * @param[in] msg_ctx The messaging context.
+ */
+void delete_and_reload_printers_full(struct tevent_context *ev,
+ struct messaging_context *msg_ctx)
+{
+ struct auth_session_info *session_info = NULL;
+ struct spoolss_PrinterInfo2 *pinfo2 = NULL;
+ int n_services;
+ int pnum;
+ int snum;
+ const char *pname;
+ const char *sname;
+ NTSTATUS status;

+
+ n_services = lp_numservices();

+ pnum = lp_servicenumber(PRINTERS_NAME);
+
status = make_session_info_system(talloc_tos(), &session_info);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(3, ("reload_printers: "
@@ -137,7 +190,6 @@ void delete_and_reload_printers(struct tevent_context *ev,
}
nt_printer_remove(session_info, session_info, msg_ctx,
pname);
- lp_killservice(snum);
} else {
DEBUG(8, ("Adding default registry entry for printer "
"[%s], if it doesn't exist.\n", sname));
@@ -146,8 +198,8 @@ void delete_and_reload_printers(struct tevent_context *ev,
}
}

- /* Make sure deleted printers are gone */
- load_printers(ev, msg_ctx);
+ /* finally, purge old snums */
+ delete_and_reload_printers(ev, msg_ctx);

TALLOC_FREE(session_info);
}
--
1.8.4.5

[David Disseldorp]

On Mon, 4 Aug 2014 15:58:00 +0200, David Disseldorp wrote:

> This patch-set attempts to address bug 10652: Samba consumes a lot of
> CPU when re-reading printcap info.
>
> It does so with the following changes to the printcap update procedure:
> - Use traverse_read for printer list share updates, instead of exclusive
> (write locked) traversals. This significantly reduces printer_list.tdb
> lock contention.
> - As suggested by Volker, reload per-client smbd printer share
> inventories on client enumeration, rather than immediately after every
> printer list update.
> - If enumeration is requested but the printer list has not changed,
> then skip the printer share inventory update.

Looks like one further change is needed here to handle clients that do
not enumerate printers prior to open, such as cupsaddsmb.

Cheers, David

[Andreas Schneider]

On Monday 04 August 2014 15:58:00 David Disseldorp wrote:
> This patch-set attempts to address bug 10652: Samba consumes a lot of
> CPU when re-reading printcap info.
>
> It does so with the following changes to the printcap update procedure:
> - Use traverse_read for printer list share updates, instead of exclusive
> (write locked) traversals. This significantly reduces printer_list.tdb
> lock contention.
> - As suggested by Volker, reload per-client smbd printer share
> inventories on client enumeration, rather than immediately after every
> printer list update.
> - If enumeration is requested but the printer list has not changed,
> then skip the printer share inventory update.

> - Split delete_and_reload_printers() functionality into two functions

> such that per-client smbd processes do not perform registry.tdb
> updates or printer unpublishing. This is now only performed by the

> single process that performs the printer list update.
>
> Feedback appreciated.


FYI. I've reviewed it and provided feedback via a private chat.


printing: only reload printer shares on client enum


This misses a few spots and David also found an issue so we have a NAK on this
patchset and David is preparing another round.



-- andreas

--
Andreas Schneider GPG-ID: CC014E3D
Samba Team a...@samba.org
www.samba.org

[David Disseldorp]

All print list updates are now done via pcap_cache_replace(), which can
call into the print_list code directly.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/pcap.c | 16 ++++++----------
source3/printing/pcap.h | 1 -
2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 0c4bf40..9c44584 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,15 +83,6 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

-static bool pcap_cache_add(const char *name, const char *comment, const char *location)
-{
- NTSTATUS status;
- time_t t = time_mono(NULL);
-
- status = printer_list_set_printer(talloc_tos(), name, comment, location, t);
- return NT_STATUS_IS_OK(status);
-}
-
bool pcap_cache_loaded(void)
{
NTSTATUS status;
@@ -105,6 +96,7 @@ bool pcap_cache_replace(const struct pcap_cache *pcache)
{
const struct pcap_cache *p;
NTSTATUS status;
+ time_t t = time_mono(NULL);

status = printer_list_mark_reload();
if (!NT_STATUS_IS_OK(status)) {
@@ -113,7 +105,11 @@ bool pcap_cache_replace(const struct pcap_cache *pcache)
}

for (p = pcache; p; p = p->next) {
- pcap_cache_add(p->name, p->comment, p->location);
+ status = printer_list_set_printer(talloc_tos(), p->name,
+ p->comment, p->location, t);

+ if (!NT_STATUS_IS_OK(status)) {
+ return false;
+ }
}

status = printer_list_clean_old();
diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index d388d7d..7dccf84 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -35,7 +35,6 @@ struct pcap_cache;

bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location);
void pcap_cache_destroy_specific(struct pcap_cache **ppcache);

-bool pcap_cache_add(const char *name, const char *comment, const char *location);
bool pcap_cache_loaded(void);

bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);

--
1.8.4.5

[David Disseldorp]

The printcap update procedure involves the background printer process
obtaining the printcap information from the printing backend, writing
this to printer_list.tdb, and then notifying all smbd processes of the
new list. The processes then all attempt to simultaneously traverse
printer_list.tdb, in order to update their local share lists.

With a large number of printers, and a large number of per-client smbd
processes, this traversal results in significant lock contention, mostly
due to the fact that the traversal is unnecessarily done with an
exclusive (write) lock on the printer_list.tdb database.

This commit changes the share update code path to perform a read-only
traversal.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Reported-by: Alex K <korobki...@gmail.com>
Reported-by: Franz Pförtsch <franz.p...@brose.com>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/load.c | 2 +-
source3/printing/pcap.c | 4 ++--
source3/printing/pcap.h | 2 +-
source3/printing/printer_list.c | 17 +++++++++++------

source3/printing/printer_list.h | 4 ++--
5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/source3/printing/load.c b/source3/printing/load.c
index 136d055..2ba3b2e 100644
--- a/source3/printing/load.c
+++ b/source3/printing/load.c

@@ -71,5 +71,5 @@ void load_printers(struct tevent_context *ev,

/* load all printcap printers */
if (lp_load_printers() && lp_servicenumber(PRINTERS_NAME) >= 0)
- pcap_printer_fn(lp_add_one_printer, NULL);
+ pcap_printer_read_fn(lp_add_one_printer, NULL);
}

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index dd7ba62..25dd4c7 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c

@@ -229,11 +229,11 @@ void pcap_printer_fn_specific(const struct pcap_cache *pc,
return;
}

-void pcap_printer_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata)
+void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata)
{
NTSTATUS status;

- status = printer_list_run_fn(fn, pdata);
+ status = printer_list_read_run_fn(fn, pdata);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(3, ("Failed to run fn for all printers!\n"));
}

diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 7056213..6c062c3 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -39,7 +39,7 @@ bool pcap_cache_add(const char *name, const char *comment, const char *location)

bool pcap_cache_loaded(void);
bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);

[David Disseldorp]

Currently, automatic printer share updates are handled in the following
way:
- Background printer process (BPP) forked on startup
- Parent smbd and per-client children await MSG_PRINTER_PCAP messages
- BPP periodically polls the printing backend for printcap data
- printcap data written to printer_list.tdb
- MSG_PRINTER_PCAP sent to all smbd processes following update
- smbd processes all read the latest printer_list.tdb data, and update
their share listings

This procedure is not scalable, as all smbd processes hit
printer_list.tdb in parallel, resulting in a large spike in CPU usage.

This change sees smbd processes only update their printer share lists
only when a client asks for this information, e.g. via NetShareEnum or
EnumPrinters.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Suggested-by: Volker Lendecke <v...@samba.org>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/spoolssd.c | 34 +++++------------------------
source3/rpc_server/spoolss/srv_spoolss_nt.c | 11 +++++++++-
source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 1 +

source3/smbd/lanman.c | 1 +
source3/smbd/server.c | 20 -----------------
5 files changed, 17 insertions(+), 50 deletions(-)

diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c
index f181fcc..afb8f4f 100644
--- a/source3/printing/spoolssd.c
+++ b/source3/printing/spoolssd.c

+
+ n_services = lp_numservices();

*count_p = 0;
*info_p = NULL;

diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
index e030b98..11abc6c 100644
--- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
+++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
@@ -548,6 +548,7 @@ static WERROR init_srv_share_info_ctr(struct pipes_struct *p,

/* Ensure all the usershares are loaded. */
become_root();
+ delete_and_reload_printers(server_event_context(), p->msg_ctx);
load_usershare_shares(NULL, connections_snum_used);
load_registry_shares();
num_services = lp_numservices();

diff --git a/source3/smbd/lanman.c b/source3/smbd/lanman.c
index 66ab8a2..b7c74e9 100644
--- a/source3/smbd/lanman.c
+++ b/source3/smbd/lanman.c
@@ -2091,6 +2091,7 @@ static bool api_RNetShareEnum(struct smbd_server_connection *sconn,

/* Ensure all the usershares are loaded. */
become_root();

+ delete_and_reload_printers(sconn->ev_ctx, sconn->msg_ctx);
load_registry_shares();
count = load_usershare_shares(NULL, connections_snum_used);
unbecome_root();

[David Disseldorp]

This will allow in future for a single atomic printer_list.tdb update.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/pcap.c | 26 +++++++++++---------------
source3/printing/pcap.h | 8 ++++----
source3/printing/print_aix.c | 17 ++++++++++++++---
source3/printing/print_iprint.c | 16 ++++++++++------
source3/printing/print_standard.c | 8 ++++++--
source3/printing/print_svid.c | 11 +++++++----
6 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 25dd4c7..0c4bf40 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,7 +83,7 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 6c062c3..d388d7d 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h

[David Disseldorp]

The per-client smbd printer share inventory is currently updated from
printer_list.tdb when a client enumerates printers, via EnumPrinters or
NetShareEnum.
printer_list.tdb is populated by the background print process, based on
the latest printcap values retrieved from the printing backend (e.g.
CUPS) at regular intervals.
This change ensures that per-client smbd processes don't reparse
printer_list.tdb if it hasn't been updated since the last enumeration.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Suggested-by: Volker Lendecke <v...@samba.org>

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/smbd/server_reload.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index 627ad8b..1477f00 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c

[David Disseldorp]

Only keep a single definition in server_reload.c

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/rpc_server/spoolss/srv_spoolss_nt.c | 9 ---------
source3/smbd/proto.h | 1 +
source3/smbd/server_reload.c | 2 +-
3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
index b8cae89..391bb01 100644
--- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
+++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
@@ -4289,15 +4289,6 @@ static WERROR construct_printer_info8(TALLOC_CTX *mem_ctx,
return WERR_OK;
}

-
-/********************************************************************
-********************************************************************/
-
-static bool snum_is_shared_printer(int snum)
-{
- return (lp_browseable(snum) && lp_snum_ok(snum) && lp_printable(snum));
-}
-
/********************************************************************
Spoolss_enumprinters.
********************************************************************/
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 22cd921..3905a74 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -955,6 +955,7 @@ const struct security_token *sec_ctx_active_token(void);
/* The following definitions come from smbd/server.c */

struct memcache *smbd_memcache(void);
+bool snum_is_shared_printer(int snum);
void delete_and_reload_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx);
bool reload_services(struct smbd_server_connection *sconn,
diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index 1477f00..efcf294 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c
@@ -38,7 +38,7 @@
*/
static time_t reload_last_pcap_time = 0;

-static bool snum_is_shared_printer(int snum)
+bool snum_is_shared_printer(int snum)

{
return (lp_browseable(snum) && lp_snum_ok(snum) && lp_printable(snum));
}

--
1.8.4.5

[David Disseldorp]

The printer share inventory should be reloaded on open _and_
enumeration, as there are some clients, such as cupsaddsmb, that do not
perform an enumeration prior to access.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/rpc_server/spoolss/srv_spoolss_nt.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
index 391bb01..b8a6d77 100644
--- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
+++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
@@ -1720,6 +1720,16 @@ WERROR _spoolss_OpenPrinterEx(struct pipes_struct *p,
return WERR_INVALID_PARAM;
}

+ /*
+ * The printcap printer share inventory is updated on client
+ * enumeration. For clients that do not perform enumeration prior to
+ * access, such as cupssmbadd, we reinitialise the printer share
+ * inventory on open as well.
+ */
+ become_root();
+ delete_and_reload_printers(server_event_context(), p->msg_ctx);
+ unbecome_root();
+
/* some sanity check because you can open a printer or a print server */
/* aka: \\server\printer or \\server */

@@ -4316,7 +4326,7 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx,
}

/*
- * printer shares are only updated on client enumeration. The background
+ * printer shares are updated on client enumeration. The background

* printer process updates printer_list.tdb at regular intervals.

*/
become_root();
--
1.8.4.5

[David Disseldorp]

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/load.c | 2 +-
source3/printing/pcap.c | 10 ++++++++--
source3/printing/pcap.h | 2 +-
source3/printing/queue_process.c | 2 +-

source3/printing/spoolssd.c | 4 ++--
5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/source3/printing/load.c b/source3/printing/load.c
index 2ba3b2e..238998d 100644
--- a/source3/printing/load.c
+++ b/source3/printing/load.c

@@ -65,7 +65,7 @@ load automatic printer services from pre-populated pcap cache

void load_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx)

{
- SMB_ASSERT(pcap_cache_loaded());
+ SMB_ASSERT(pcap_cache_loaded(NULL));

add_auto_printers();

diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c
index 9c44584..c5524ad 100644
--- a/source3/printing/pcap.c
+++ b/source3/printing/pcap.c
@@ -83,13 +83,19 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache)
*pp_cache = NULL;
}

-bool pcap_cache_loaded(void)
+bool pcap_cache_loaded(time_t *_last_change)
{
NTSTATUS status;
time_t last;

status = printer_list_get_last_refresh(&last);
- return NT_STATUS_IS_OK(status);

+ if (!NT_STATUS_IS_OK(status)) {
+ return false;
+ }

+ if (_last_change != NULL) {
+ *_last_change = last;
+ }
+ return true;
}

bool pcap_cache_replace(const struct pcap_cache *pcache)

diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h
index 7dccf84..8fc9e9d 100644
--- a/source3/printing/pcap.h
+++ b/source3/printing/pcap.h
@@ -35,7 +35,7 @@ struct pcap_cache;

bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location);
void pcap_cache_destroy_specific(struct pcap_cache **ppcache);

-bool pcap_cache_loaded(void);
+bool pcap_cache_loaded(time_t *_last_change);

bool pcap_cache_replace(const struct pcap_cache *cache);
void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *);

void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *);

diff --git a/source3/printing/queue_process.c b/source3/printing/queue_process.c
index aa0d0fb..24d361c 100644
--- a/source3/printing/queue_process.c
+++ b/source3/printing/queue_process.c

@@ -390,7 +390,7 @@ void printing_subsystem_update(struct tevent_context *ev_ctx,
bool force)
{
if (background_lpq_updater_pid != -1) {
- if (pcap_cache_loaded()) {
+ if (pcap_cache_loaded(NULL)) {
load_printers(ev_ctx, msg_ctx);
}
if (force) {

diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c
index afb8f4f..b850578 100644
--- a/source3/printing/spoolssd.c
+++ b/source3/printing/spoolssd.c
@@ -304,7 +304,7 @@ static bool spoolss_child_init(struct tevent_context *ev_ctx,

* If so then we probably missed a message and should load_printers()

* ourselves. If pcap has not been loaded yet, then ignore, we will get

* a message as soon as the bq process completes the reload. */

- if (pcap_cache_loaded()) {
+ if (pcap_cache_loaded(NULL)) {
load_printers(ev_ctx, msg_ctx);
}

@@ -724,7 +724,7 @@ pid_t start_spoolssd(struct tevent_context *ev_ctx,

* If pcap has not been loaded yet, then ignore, as we will reload on

[David Disseldorp]

All printer inventory updates are currently done via
delete_and_reload_printers(), which handles registry.tdb updates for
added or removed printers, AD printer unpublishing on removal, as well
as share service creation and deletion.

This change splits this functionality into two functions such that

per-client smbd processes do not perform registry.tdb updates or printer

unpublishing. This is now only performed by the process that performs
the printcap cache update.

This change is similar to ac6604868d1325dd4c872dc0f6ab056d10ebaecf from
the 3.6 branch.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652

Signed-off-by: David Disseldorp <dd...@samba.org>
---

source3/printing/queue_process.c | 100 +++++++++++++++++++++++++++++++++++++--
source3/smbd/server_reload.c | 51 +++-----------------
2 files changed, 104 insertions(+), 47 deletions(-)

diff --git a/source3/printing/queue_process.c b/source3/printing/queue_process.c
index 24d361c..88196b4 100644
--- a/source3/printing/queue_process.c
+++ b/source3/printing/queue_process.c
@@ -33,10 +33,102 @@
#include "rpc_server/rpc_config.h"
#include "printing/load.h"
#include "rpc_server/spoolss/srv_spoolss_nt.h"
+#include "auth.h"
+#include "nt_printing.h"

extern pid_t start_spoolssd(struct tevent_context *ev_ctx,
struct messaging_context *msg_ctx);

+/**
+ * @brief Purge stale printers and reload from pre-populated pcap cache.
+ *
+ * This function should normally only be called as a callback on a successful
+ * pcap_cache_reload().
+ *
+ * This function can cause DELETION of printers and drivers from our registry,
+ * so calling it on a failed pcap reload may REMOVE permanently all printers
+ * and drivers.
+ *
+ * @param[in] ev The event context.
+ *
+ * @param[in] msg_ctx The messaging context.
+ */

+static void delete_and_reload_printers_full(struct tevent_context *ev,

+ struct messaging_context *msg_ctx)
+{
+ struct auth_session_info *session_info = NULL;
+ struct spoolss_PrinterInfo2 *pinfo2 = NULL;
+ int n_services;
+ int pnum;
+ int snum;
+ const char *pname;
+ const char *sname;
+ NTSTATUS status;

+
+ n_services = lp_numservices();

+ pnum = lp_servicenumber(PRINTERS_NAME);
+
+ status = make_session_info_system(talloc_tos(), &session_info);

+ if (!NT_STATUS_IS_OK(status)) {
+ DEBUG(3, ("reload_printers: "
+ "Could not create system session_info\n"));
+ /* can't remove stale printers before we
+ * are fully initilized */
+ return;
+ }
+

+ /*
+ * Add default config for printers added to smb.conf file and remove
+ * stale printers
+ */
+ for (snum = 0; snum < n_services; snum++) {
+ /* avoid removing PRINTERS_NAME */
+ if (snum == pnum) {
+ continue;
+ }
+
+ /* skip no-printer services */
+ if (!snum_is_shared_printer(snum)) {
+ continue;
+ }
+

+ sname = lp_const_servicename(snum);
+ pname = lp_printername(session_info, snum);

+
+ /* check printer, but avoid removing non-autoloaded printers */
+ if (lp_autoloaded(snum) && !pcap_printername_ok(pname)) {

+ DEBUG(3, ("removing stale printer %s\n", pname));
+
+ if (is_printer_published(session_info, session_info,
+ msg_ctx,
+ NULL,
+ lp_servicename(session_info,
+ snum),
+ &pinfo2)) {
+ nt_printer_publish(session_info,
+ session_info,
+ msg_ctx,
+ pinfo2,
+ DSPRINT_UNPUBLISH);
+ TALLOC_FREE(pinfo2);
+ }
+ nt_printer_remove(session_info, session_info, msg_ctx,
+ pname);
+ } else {
+ DEBUG(8, ("Adding default registry entry for printer "
+ "[%s], if it doesn't exist.\n", sname));
+ nt_printer_add(session_info, session_info, msg_ctx,
+ sname);
+ }
+ }
+

+ /* finally, purge old snums */
+ delete_and_reload_printers(ev, msg_ctx);

+
+ TALLOC_FREE(session_info);
+}
+
+
/****************************************************************************
Notify smbds of new printcap data
**************************************************************************/
@@ -50,7 +142,7 @@ static void reload_pcap_change_notify(struct tevent_context *ev,

* This will block the process for some time (~1 sec per printer), but
* it doesn't block smbd's servering clients.
*/
- delete_and_reload_printers(ev, msg_ctx);
+ delete_and_reload_printers_full(ev, msg_ctx);

message_send_all(msg_ctx, MSG_PRINTER_PCAP, NULL, 0, NULL);
}

@@ -372,7 +464,8 @@ bool printing_subsystem_init(struct tevent_context *ev_ctx,

ret = printing_subsystem_queue_tasks(ev_ctx, msg_ctx);

/* Publish nt printers, this requires a working winreg pipe */
- pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers);
+ pcap_cache_reload(ev_ctx, msg_ctx,
+ &delete_and_reload_printers_full);

return ret;
}

@@ -401,5 +494,6 @@ void printing_subsystem_update(struct tevent_context *ev_ctx,

return;
}

- pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers);
+ pcap_cache_reload(ev_ctx, msg_ctx,
+ &delete_and_reload_printers_full);
}

diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
index efcf294..317a250 100644
--- a/source3/smbd/server_reload.c
+++ b/source3/smbd/server_reload.c
@@ -44,14 +44,10 @@ bool snum_is_shared_printer(int snum)

}

/**
- * @brief Purge stale printers and reload from pre-populated pcap cache.
+ * @brief Purge stale printer shares and reload from pre-populated pcap cache.
*
* This function should normally only be called as a callback on a successful
- * pcap_cache_reload() or after a MSG_PRINTER_CAP message is received.
- *
- * This function can cause DELETION of printers and drivers from our registry,
- * so calling it on a failed pcap reload may REMOVE permanently all printers
- * and drivers.
+ * pcap_cache_reload(), or on client enumeration.
*
* @param[in] ev The event context.
*

@@ -60,25 +56,24 @@ bool snum_is_shared_printer(int snum)
void delete_and_reload_printers(struct tevent_context *ev,
struct messaging_context *msg_ctx)
{

- struct auth_session_info *session_info = NULL;
- struct spoolss_PrinterInfo2 *pinfo2 = NULL;
int n_services;
int pnum;
int snum;
const char *pname;
- const char *sname;
- NTSTATUS status;
bool ok;
time_t pcap_last_update;
+ TALLOC_CTX *frame = talloc_stackframe();

ok = pcap_cache_loaded(&pcap_last_update);
if (!ok) {

DEBUG(1, ("pcap cache not loaded\n"));

+ talloc_free(frame);
return;
}

if (reload_last_pcap_time == pcap_last_update) {

DEBUG(5, ("skipping printer reload, already up to date.\n"));

+ talloc_free(frame);
return;
}
reload_last_pcap_time = pcap_last_update;

@@ -91,15 +86,6 @@ void delete_and_reload_printers(struct tevent_context *ev,

DEBUG(10, ("reloading printer services from pcap cache\n"));

- status = make_session_info_system(talloc_tos(), &session_info);
- if (!NT_STATUS_IS_OK(status)) {
- DEBUG(3, ("reload_printers: "
- "Could not create system session_info\n"));
- /* can't remove stale printers before we
- * are fully initilized */
- return;
- }
-
/*

* Add default config for printers added to smb.conf file and remove

* stale printers
@@ -115,41 +101,18 @@ void delete_and_reload_printers(struct tevent_context *ev,
continue;
}

- sname = lp_const_servicename(snum);
- pname = lp_printername(session_info, snum);

+ pname = lp_printername(frame, snum);

/* check printer, but avoid removing non-autoloaded printers */

if (lp_autoloaded(snum) && !pcap_printername_ok(pname)) {
- DEBUG(3, ("removing stale printer %s\n", pname));
-
- if (is_printer_published(session_info, session_info,
- msg_ctx,
- NULL,
- lp_servicename(session_info,
- snum),
- &pinfo2)) {
- nt_printer_publish(session_info,
- session_info,
- msg_ctx,
- pinfo2,
- DSPRINT_UNPUBLISH);
- TALLOC_FREE(pinfo2);
- }
- nt_printer_remove(session_info, session_info, msg_ctx,
- pname);
lp_killservice(snum);
- } else {
- DEBUG(8, ("Adding default registry entry for printer "
- "[%s], if it doesn't exist.\n", sname));
- nt_printer_add(session_info, session_info, msg_ctx,
- sname);

}
}

/* Make sure deleted printers are gone */

load_printers(ev, msg_ctx);

- TALLOC_FREE(session_info);
+ talloc_free(frame);
}

/****************************************************************************
--
1.8.4.5

[Andreas Schneider]

On Thursday 07 August 2014 17:40:31 David Disseldorp wrote:
> This patch-set attempts to address bug 10652: Samba consumes a lot of
> CPU when re-reading printcap info.
>
> It does so with the following changes to the printcap update procedure:
> - Use traverse_read for printer list share updates, instead of exclusive
> (write locked) traversals. This significantly reduces printer_list.tdb
> lock contention.
> - As suggested by Volker, reload per-client smbd printer share

> inventories when needed, rather than immediately after every printer
> list update.
> - If the printer list has not changed since the previous reload, then

> skip the printer share inventory update.

> - Split delete_and_reload_printers() functionality into two functions

> such that per-client smbd processes do not perform registry.tdb
> updates or printer unpublishing. This is now only performed by the

> single process that performs the printer list update.
>
> Feedback appreciated.

Great work! Thanks for working on that.

Pushed to autobuild.