Issue proftpd#1484: Update scoreboard at regular time intervals

This commit removes the previously used blocksize or filesize percentage based
scoreboard updates and starts using elapsed time based scoreboard updates.

Percent based scoreboard updates could have been too infrequent for big files
and slow link speeds, blocksize based scoreboard updates could have been too
frequent for fast links.

Also this changes the meaning of PR_TUNABLE_XFER_SCOREBOARD_UPDATES, previously
it meant the number of transferred blocks to wait until a scoreboard update
happened, now it means the number of deciseconds to wait between subsequent
scoreboard updates.

Deciseconds as the unit were chosen so the default of
PR_TUNABLE_XFER_SCOREBOARD_UPDATES of 10 is unchanged and will mean a
scoreboard update every second.

NEWS

@@ -36,6 +36,7 @@
 - Issue 1476 - Reload after omitting explicit ModulePath value causes fatal
   module load failures.
 - Issue 1433 - Update PR_SCORE_XFER_DONE in pr_throttle_pause
+- Issue 1484 - Scoreboard updates should happen at regular time intervals
 
 1.3.8rc3 - Released 23-Apr-2022
 --------------------------------

configure.in

@@ -1328,9 +1328,9 @@ AC_ARG_ENABLE(scoreboard-updates,
   [
     if test x"$enableval" = xyes || test x"$enableval" = xno ; then
       AC_MSG_WARN(scoreboard updates defaulting to 10)
-      AC_DEFINE_UNQUOTED(PR_TUNABLE_XFER_SCOREBOARD_UPDATES, 10, [Define the scoreboard update count])
+      AC_DEFINE_UNQUOTED(PR_TUNABLE_XFER_SCOREBOARD_UPDATES, 10, [Define the scoreboard update interval in deciseconds])
     else
-      AC_DEFINE_UNQUOTED(PR_TUNABLE_XFER_SCOREBOARD_UPDATES, $enableval, [Define the scoreboard update count])
+      AC_DEFINE_UNQUOTED(PR_TUNABLE_XFER_SCOREBOARD_UPDATES, $enableval, [Define the scoreboard update interval in deciseconds])
     fi
   ])
 

modules/mod_xfer.c

@@ -2678,7 +2678,7 @@ MODRET xfer_retr(cmd_rec *cmd) {
   unsigned char have_limit = FALSE;
   long bufsz, len = 0;
   off_t start_offset = 0, download_len = 0;
-  off_t curr_offset, curr_pos = 0, nbytes_sent = 0, cnt_steps = 0, cnt_next = 0;
+  off_t curr_offset, curr_pos = 0, nbytes_sent = 0;
   pr_error_t *err = NULL;
 
   /* Prepare for any potential throttling. */
@@ -2853,11 +2853,6 @@ MODRET xfer_retr(cmd_rec *cmd) {
 
   pr_alarms_unblock();
 
-  cnt_steps = session.xfer.file_size / 100;
-  if (cnt_steps == 0) {
-    cnt_steps = 1;
-  }
-
   if (session.range_len > 0) {
     if (curr_pos + session.range_len > st.st_size) {
       /* If the RANG end point is past the end of our file, ignore it and
@@ -2932,8 +2927,6 @@ MODRET xfer_retr(cmd_rec *cmd) {
   }
 
   while (nbytes_sent != download_len) {
-    int force_scoreboard_update = FALSE;
-
     pr_signals_handle();
 
     if (XFER_ABORTED) {
@@ -2996,19 +2989,13 @@ MODRET xfer_retr(cmd_rec *cmd) {
     nbytes_sent += len;
     curr_offset += len;
 
-    if ((nbytes_sent / cnt_steps) != cnt_next) {
-      cnt_next = nbytes_sent / cnt_steps;
-      force_scoreboard_update = TRUE;
-    }
-
     /* If no throttling is configured, this simply updates the scoreboard.
      * In this case, we want to use session.xfer.total_bytes, rather than
      * nbytes_sent, as the latter incorporates a REST position and the
      * former does not.  (When handling STOR, this is not an issue: different
      * end-of-loop conditions).
      */
-    pr_throttle_pause(session.xfer.total_bytes, force_scoreboard_update,
-      nbytes_sent);
+    pr_throttle_pause(session.xfer.total_bytes, FALSE, nbytes_sent);
   }
 
   if (XFER_ABORTED) {

src/throttle.c

@@ -30,7 +30,7 @@
 static long double xfer_rate_kbps = 0.0, xfer_rate_bps = 0.0;
 static off_t xfer_rate_freebytes = 0.0;
 static int have_xfer_rate = FALSE;
-static unsigned int xfer_rate_scoreboard_updates = 0;
+static long xfer_rate_next_scoreboard_update = 0;
 
 /* Very similar to the {block,unblock}_signals() function, this masks most
  * of the same signals -- except for TERM.  This allows a throttling process
@@ -100,7 +100,7 @@ void pr_throttle_init(cmd_rec *cmd) {
   /* Make sure the variables are (re)initialized */
   xfer_rate_kbps = xfer_rate_bps = 0.0;
   xfer_rate_freebytes = 0;
-  xfer_rate_scoreboard_updates = 0;
+  xfer_rate_next_scoreboard_update = 0;
   have_xfer_rate = FALSE;
 
   c = find_config(CURRENT_CONF, CONF_PARAM, "TransferRate", FALSE);
@@ -222,18 +222,17 @@ void pr_throttle_pause(off_t xferlen, int force_scoreboard_update, off_t xfer_do
 
   /* Perform no throttling if no throttling has been configured. */
   if (!have_xfer_rate) {
-    xfer_rate_scoreboard_updates++;
 
     if (force_scoreboard_update ||
-        xfer_rate_scoreboard_updates % PR_TUNABLE_XFER_SCOREBOARD_UPDATES == 0) {
+        elapsed > xfer_rate_next_scoreboard_update) {
       /* Update the scoreboard. */
       pr_scoreboard_entry_update(session.pid,
         PR_SCORE_XFER_LEN, orig_xferlen,
         PR_SCORE_XFER_DONE, xfer_done,
         PR_SCORE_XFER_ELAPSED, (unsigned long) elapsed,
         NULL);
 
-      xfer_rate_scoreboard_updates = 0;
+      xfer_rate_next_scoreboard_update = elapsed + PR_TUNABLE_XFER_SCOREBOARD_UPDATES * 100;
     }
 
     return;
@@ -250,21 +249,19 @@ void pr_throttle_pause(off_t xferlen, int force_scoreboard_update, off_t xfer_do
       xferlen -= xfer_rate_freebytes;
 
     } else {
-      xfer_rate_scoreboard_updates++;
-
       /* The number of bytes transferred is less than the freebytes.  Just
        * update the scoreboard -- no throttling needed.
        */
 
       if (force_scoreboard_update ||
-          xfer_rate_scoreboard_updates % PR_TUNABLE_XFER_SCOREBOARD_UPDATES == 0) {
+          elapsed > xfer_rate_next_scoreboard_update) {
         pr_scoreboard_entry_update(session.pid,
           PR_SCORE_XFER_LEN, orig_xferlen,
           PR_SCORE_XFER_DONE, xfer_done,
           PR_SCORE_XFER_ELAPSED, (unsigned long) elapsed,
           NULL);
 
-        xfer_rate_scoreboard_updates = 0;
+        xfer_rate_next_scoreboard_update = elapsed + PR_TUNABLE_XFER_SCOREBOARD_UPDATES * 100;
       }
 
       return;