getncchanges.c: max_links calculation didn't work well in some cases

The max_links calculation didn't work particularly well if max_links was
set to a value lower than max_objects.

As soon as repl_chunk->object_count exceeded repl_chunk->max_links, the
chunk would be deemed full, even if there was only one link to send (or
even worse, no links to send). For example, if max_objects=100 and
max_links=10, then it would send back chunks of 10 objects (or 9 objects
and 1 link).

I believe the historic reason this logic exists is to avoid overfilling
the response message. It's hard to tell what the appropriate limit would
be because the total message size would depend on how many attributes
each object has.

I couldn't think of logic that would be suitable for all cases. I toyed
with the idea of working out a percentage of how full the message is.
However, adjusting the max_links doesn't really make sense when the
settings are small enough, e.g. max_objects=100 and max_links=100 is
never going to overfill the message, so there's no reason to alter the
values.

In the end I went with:
- If the user is using non-default values, just use those.
- In the default value case, just use the historic calculation

Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972
Reviewed-by: Garming Sam <garming@catalyst.net.nz>

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 4632a9ccf1a7910f77c6020db31c25108ef1cdb4..b48e9c7234d530eb2f41a5a279b88b2a57a417ee 100644 (file)
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -46,6 +46,8 @@
 #define DBGC_CLASS            DBGC_DRS_REPL
 
 #define DRS_GUID_SIZE       16
+#define DEFAULT_MAX_OBJECTS 1000
+#define DEFAULT_MAX_LINKS   1500
 
 /*
  * state of a partially-completed replication cycle. This state persists
@@ -2388,21 +2390,27 @@ static uint32_t getncchanges_chunk_links_pending(struct getncchanges_repl_chunk
  */
 static uint32_t getncchanges_chunk_max_links(struct getncchanges_repl_chunk *repl_chunk)
 {
-       uint32_t max_links;
+       uint32_t max_links = 0;
 
-       /*
-        * This is just an approximate guess to avoid overfilling the replication
-        * chunk. E.g. if we've already sent 1000 objects, then send 1000 fewer
-        * links. For comparison, the max that Windows seems to send is ~2700
-        * links and ~250 objects (although this may vary based on timeouts)
-        */
-       if (repl_chunk->object_count >= repl_chunk->max_links) {
+       if (repl_chunk->max_links != DEFAULT_MAX_LINKS ||
+           repl_chunk->max_objects != DEFAULT_MAX_OBJECTS) {
 
-               /* request is already full of objects - don't send any links */
-               max_links = 0;
-       } else {
+               /*
+                * We're using non-default settings, so don't try to adjust
+                * them, just trust the user has configured decent values
+                */
+               max_links = repl_chunk->max_links;
+
+       } else if (repl_chunk->max_links > repl_chunk->object_count) {
 
-               /* send fewer links if we're already sending a lot of objects */
+               /*
+                * This is just an approximate guess to avoid overfilling the
+                * replication chunk. It's the logic we've used historically.
+                * E.g. if we've already sent 1000 objects, then send 1000 fewer
+                * links. For comparison, the max that Windows seems to send is
+                * ~2700 links and ~250 objects (although this may vary based
+                * on timeouts)
+                */
                max_links = repl_chunk->max_links - repl_chunk->object_count;
        }
 
@@ -2454,7 +2462,9 @@ static bool getncchanges_chunk_is_full(struct getncchanges_repl_chunk *repl_chun
                 * The chunk is full if we've got more links to send than will
                 * fit in one chunk
                 */
-               chunk_full = (chunk_limit <= links_to_send);
+               if (links_to_send > 0 && chunk_limit <= links_to_send) {
+                       chunk_full = true;
+               }
        }
 
        return chunk_full;
@@ -2641,7 +2651,8 @@ static struct getncchanges_repl_chunk * getncchanges_chunk_new(TALLOC_CTX *mem_c
        repl_chunk->start = time(NULL);
 
        repl_chunk->max_objects = lpcfg_parm_int(dce_call->conn->dce_ctx->lp_ctx, NULL,
-                                                "drs", "max object sync", 1000);
+                                                "drs", "max object sync",
+                                                DEFAULT_MAX_OBJECTS);
 
        /*
         * The client control here only applies in normal replication, not extended
@@ -2661,7 +2672,8 @@ static struct getncchanges_repl_chunk * getncchanges_chunk_new(TALLOC_CTX *mem_c
 
        repl_chunk->max_links =
                        lpcfg_parm_int(dce_call->conn->dce_ctx->lp_ctx, NULL,
-                                      "drs", "max link sync", 1500);
+                                      "drs", "max link sync",
+                                       DEFAULT_MAX_LINKS);
 
        repl_chunk->immediate_link_sync =
                        lpcfg_parm_bool(dce_call->conn->dce_ctx->lp_ctx, NULL,