sys/log: Add bookmarks per sector to improve reading

vrahane · vrahane · commit efdadf6baad5 · 2025-01-30T14:44:57.000-08:00
- Add sector bookmarks for reading optimization
- Retain older absolute bookmarks behavior
- CLI: Add '-t' option in log_shell for measuring read time (cherry-picking Jerzy's
commit)
- CLI: Add '-i' option in log_shell for specifying index where log read
should start
- CLI: Add '-b' option in log_shell for reading bookmarks
diff --git a/fs/fcb/include/fcb/fcb.h b/fs/fcb/include/fcb/fcb.h
@@ -168,6 +168,28 @@ typedef int (*fcb_walk_cb)(struct fcb_entry *loc, void *arg);
 int fcb_walk(struct fcb *, struct flash_area *, fcb_walk_cb cb, void *cb_arg);
 int fcb_getnext(struct fcb *, struct fcb_entry *loc);
 
+/**
+ * Get first entry in the provided flash area
+ *
+ * @param fcb Pointer to FCB
+ * @param fa Optional pointer to flash area
+ * @param loc Pointer to first FCB entry in the provided flash area
+ *
+ * @return 0 on success, non-zero on failure
+ */
+int fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fa,
+                        struct fcb_entry *loc);
+
+/**
+ * Get next area pointer from the FCB pointer to by fcb pointer
+ *
+ * @param fcb Pointer to the FCB
+ * @param fap Pointer to the flash_area
+ *
+ * @return Pointer to the flash_area that comes next
+ */
+struct flash_area *fcb_getnext_area(struct fcb *fcb, struct flash_area *fap);
+
 #if MYNEWT_VAL_FCB_BIDIRECTIONAL
 /**
  * Call 'cb' for every element in flash circular buffer moving
diff --git a/fs/fcb/src/fcb.c b/fs/fcb/src/fcb.c
@@ -90,7 +90,7 @@ fcb_init(struct fcb *fcb)
     assert((fcb->f_align & (fcb->f_align - 1)) == 0);
 
     while (1) {
-        rc = fcb_getnext_in_area(fcb, &fcb->f_active);
+        rc = fcb_getnext_in_area(fcb, NULL, &fcb->f_active);
         if (rc == FCB_ERR_NOVAR) {
             rc = FCB_OK;
             break;
diff --git a/fs/fcb/src/fcb_getnext.c b/fs/fcb/src/fcb_getnext.c
@@ -63,10 +63,19 @@ fcb_start_offset(struct fcb *fcb)
 }
 
 int
-fcb_getnext_in_area(struct fcb *fcb, struct fcb_entry *loc)
+fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fa,
+                    struct fcb_entry *loc)
 {
     int rc;
 
+    /* If a flash area is specified, find first entry in that area */
+    if (fa) {
+        loc->fe_area = fa;
+        loc->fe_elem_off = fcb_len_in_flash(fcb, sizeof(struct fcb_disk_area));
+        loc->fe_elem_ix = 0;
+        loc->fe_data_len = 0;
+    }
+
     rc = fcb_elem_info(fcb, loc);
     if (rc == 0 || rc == FCB_ERR_CRC) {
         do {
diff --git a/fs/fcb/src/fcb_priv.h b/fs/fcb/src/fcb_priv.h
@@ -47,7 +47,6 @@ fcb_len_in_flash(struct fcb *fcb, uint16_t len)
     return (len + (fcb->f_align - 1)) & ~(fcb->f_align - 1);
 }
 
-int fcb_getnext_in_area(struct fcb *fcb, struct fcb_entry *loc);
 struct flash_area *fcb_getnext_area(struct fcb *fcb, struct flash_area *fap);
 int fcb_getnext_nolock(struct fcb *fcb, struct fcb_entry *loc);
 
diff --git a/sys/log/full/include/log/log_fcb.h b/sys/log/full/include/log/log_fcb.h
@@ -39,6 +39,8 @@ struct log_fcb_bmark {
 #endif
     /* The index of the log entry that the FCB entry contains. */
     uint32_t lfb_index;
+    /* Is this a sector boundary bookmark */
+    bool lfb_sect_bmark;
 };
 
 /** A set of fcb log bookmarks. */
@@ -49,11 +51,17 @@ struct log_fcb_bset {
     /** The maximum number of bookmarks. */
     int lfs_cap;
 
+    /** The number of currently used non-sector bookmarks. */
+    int lfs_non_s_size;
+
     /** The number of currently usable bookmarks. */
     int lfs_size;
 
     /** The index where the next bookmark will get written. */
-    int lfs_next;
+    uint32_t lfs_next;
+
+    /** The index where the next non-sector bmark will get written */
+    uint32_t lfs_next_non_s;
 };
 
 /**
@@ -72,6 +80,7 @@ struct fcb_log {
 #if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
     struct log_fcb_bset fl_bset;
 #endif
+    struct log *fl_log;
 };
 
 #elif MYNEWT_VAL(LOG_FCB2)
@@ -113,9 +122,18 @@ struct fcb_log {
  * @param fcb_log               The log to configure.
  * @param buf                   The buffer to use for bookmarks.
  * @param bmark_count           The bookmark capacity of the supplied buffer.
+ *
+ * @return 0 on success, non-zero on failure
  */
-void log_fcb_init_bmarks(struct fcb_log *fcb_log,
-                         struct log_fcb_bmark *buf, int bmark_count);
+int log_fcb_init_bmarks(struct fcb_log *fcb_log,
+                        struct log_fcb_bmark *buf, int bmark_count);
+
+/** @brief Remove bookmarks which point to oldest FCB/FCB2 area. This is
+ * meant to get called just before the area is rotated out.
+ *
+ * @param fcb_log               The fcb_log to operate on.
+ */
+void log_fcb_rotate_bmarks(struct fcb_log *fcb_log);
 
 /**
  * @brief Erases all bookmarks from the supplied fcb_log.
@@ -125,39 +143,44 @@ void log_fcb_init_bmarks(struct fcb_log *fcb_log,
 void log_fcb_clear_bmarks(struct fcb_log *fcb_log);
 
 /**
- * @brief Remove bookmarks which point to oldest FCB/FCB2 area. This is
- * meant to get called just before the area is rotated out.
+ * @brief Get bookmarks for a particular log
  *
- * @param fcb_log               The fcb_log to operate on.
+ * @param log Pointer to the log we want to read bookmarks from
+ * @param bmarks_size Pointer to the variable we want to read bookmarks into
+ *
+ * @return Pointer to the bookmarks array for the provided log
  */
-void log_fcb_rotate_bmarks(struct fcb_log *fcb_log);
+struct log_fcb_bmark *log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size);
 
 /**
  * @brief Searches an fcb_log for the closest bookmark that comes before or at
  * the specified index.
  *
  * @param fcb_log               The log to search.
  * @param index                 The index to look for.
+ * @param min_diff              If bookmark was found, fill it up with the difference
+ *                              else it will return -1.
  *
  * @return                      The closest bookmark on success;
  *                              NULL if the log has no applicable bookmarks.
  */
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index);
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index, int *min_diff);
 
 /**
  * Inserts a bookmark into the provided log.
  *
  * @param fcb_log               The log to insert a bookmark into.
  * @param entry                 The entry the bookmark should point to.
  * @param index                 The log entry index of the bookmark.
+ * @paran sect_bmark            Bool indicating it is a sector bookmark.
  */
 #if MYNEWT_VAL(LOG_FCB)
-void log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
-                       uint32_t index);
+void log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+                       uint32_t index, bool sect_bmark);
 #elif MYNEWT_VAL(LOG_FCB2)
-void log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
-                       uint32_t index);
+void log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+                       uint32_t index, bool sect_bmark);
 #endif
 #endif
 
diff --git a/sys/log/full/src/log_fcb.c b/sys/log/full/src/log_fcb.c
@@ -53,7 +53,9 @@ fcb_get_fa_hdr(struct fcb *fcb, struct log *log, struct fcb_entry *fcb_entry, st
  * given offset is found, start walking from there.
  */
 static int
-fcb_walk_back_find_start(struct fcb *fcb, struct log *log, struct log_offset *log_offset, struct fcb_entry *fcb_entry)
+fcb_walk_back_find_start(struct fcb *fcb, struct log *log,
+                         struct log_offset *log_offset,
+                         struct fcb_entry *fcb_entry)
 {
     struct flash_area *fap;
     struct log_entry_hdr hdr;
@@ -108,6 +110,7 @@ fcb_walk_back_find_start(struct fcb *fcb, struct log *log, struct log_offset *lo
  * ignored; the "index" field is used instead.
  *
  * XXX: We should rename "timestamp" or make it an actual timestamp.
+ * If bmark is found, fill up min_diff.
  *
  * The "index" field corresponds to a log entry index.
  *
@@ -119,10 +122,10 @@ fcb_walk_back_find_start(struct fcb *fcb, struct log *log, struct log_offset *lo
  */
 static int
 log_fcb_find_gte(struct log *log, struct log_offset *log_offset,
-                 struct fcb_entry *out_entry)
+                 struct fcb_entry *out_entry, int *min_diff)
 {
 #if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-    const struct log_fcb_bmark *bmark;
+    struct log_fcb_bmark *bmark;
 #endif
     struct log_entry_hdr hdr;
     struct fcb_log *fcb_log;
@@ -166,7 +169,7 @@ log_fcb_find_gte(struct log *log, struct log_offset *log_offset,
     }
 
 #if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-    bmark = log_fcb_closest_bmark(fcb_log, log_offset->lo_index);
+    bmark = log_fcb_closest_bmark(fcb_log, log_offset->lo_index, min_diff);
     if (bmark != NULL) {
         *out_entry = bmark->lfb_entry;
         bmark_found = true;
@@ -209,13 +212,21 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
     struct fcb_log *fcb_log;
     struct flash_area *old_fa;
     int rc = 0;
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+    int active_sector_cnt = 0;
+    uint32_t idx = 0;
+#endif
 #if MYNEWT_VAL(LOG_STATS)
     int cnt;
 #endif
 
     fcb_log = (struct fcb_log *)log->l_arg;
     fcb = &fcb_log->fl_fcb;
 
+    /* Cache sector count before appending */
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+    active_sector_cnt = fcb->f_active_sector_entry_count;
+#endif
     while (1) {
         rc = fcb_append(fcb, len, loc);
         if (rc == 0) {
@@ -250,8 +261,10 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
             log->l_rotate_notify_cb(log);
         }
 
-#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-        /* The FCB needs to be rotated. */
+#if MYNEWT_VAL(LOG_FCB_BOOKMARKS) && !MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+        /* The FCB needs to be rotated. For sector bookmarks
+         * we just re-initialize the bookmarks
+         */
         log_fcb_rotate_bmarks(fcb_log);
 #endif
 
@@ -260,6 +273,14 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
             goto err;
         }
 
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+        /* The FCB needs to be rotated, reinit previously allocated
+         * bookmarks
+         */
+        log_fcb_init_bmarks(fcb_log, fcb_log->fl_bset.lfs_bmarks,
+                            fcb_log->fl_bset.lfs_cap);
+#endif
+
 #if MYNEWT_VAL(LOG_STORAGE_WATERMARK)
         /*
          * FCB was rotated successfully so let's check if watermark was within
@@ -273,6 +294,20 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
 #endif
     }
 
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+    /* Add bookmark if entry is added to a new sector */
+    if (!rc && log->l_log->log_type != LOG_TYPE_STREAM) {
+        if (active_sector_cnt > fcb->f_active_sector_entry_count) {
+#if MYNEWT_VAL(LOG_GLOBAL_IDX)
+            idx = g_log_info.li_next_index;
+#else
+            idx = log->l_idx;
+#endif
+            log_fcb_add_bmark(fcb_log, loc, idx, true);
+        }
+    }
+#endif
+
 err:
     return (rc);
 }
@@ -582,12 +617,13 @@ log_fcb_walk_impl(struct log *log, log_walk_func_t walk_func,
     struct flash_area *fap;
     int rc;
     struct fcb_entry_cache cache;
+    int min_diff = -1;
 
     fcb_log = log->l_arg;
     fcb = &fcb_log->fl_fcb;
 
     /* Locate the starting point of the walk. */
-    rc = log_fcb_find_gte(log, log_offset, &loc);
+    rc = log_fcb_find_gte(log, log_offset, &loc, &min_diff);
     switch (rc) {
     case 0:
         /* Found a starting point. */
@@ -611,9 +647,12 @@ log_fcb_walk_impl(struct log *log, log_walk_func_t walk_func,
 #if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
     /* If a minimum index was specified (i.e., we are not just retrieving the
      * last entry), add a bookmark pointing to this walk's start location.
+     * Only add a bmark if the index is non-zero and an exactly matching bmark
+     * was not found. If an exactly matching bmark was found, min_diff is 0,
+     * else it stays -1 or is great than 0.
      */
-    if (log_offset->lo_ts >= 0) {
-        log_fcb_add_bmark(fcb_log, &loc, log_offset->lo_index);
+    if ((log_offset->lo_ts >= 0 && log_offset->lo_index > 0) && min_diff != 0) {
+        log_fcb_add_bmark(fcb_log, &loc, log_offset->lo_index, false);
     }
 #endif
 
@@ -674,15 +713,15 @@ log_fcb_flush(struct log *log)
 static int
 log_fcb_registered(struct log *log)
 {
+    struct fcb_log *fl = (struct fcb_log *)log->l_arg;
+
+    fl->fl_log = log;
 #if MYNEWT_VAL(LOG_STORAGE_WATERMARK)
-    struct fcb_log *fl;
 #if MYNEWT_VAL(LOG_PERSIST_WATERMARK)
     struct fcb *fcb;
     struct fcb_entry loc;
 #endif
 
-    fl = (struct fcb_log *)log->l_arg;
-
 #if MYNEWT_VAL(LOG_PERSIST_WATERMARK)
     fcb = &fl->fl_fcb;
 
diff --git a/sys/log/full/src/log_fcb2.c b/sys/log/full/src/log_fcb2.c
@@ -63,6 +63,7 @@ log_fcb2_find_gte(struct log *log, struct log_offset *log_offset,
     struct fcb_log *fcb_log;
     struct fcb2 *fcb;
     int rc;
+    int min_diff = -1;
 
     fcb_log = log->l_arg;
     fcb = &fcb_log->fl_fcb;
@@ -103,7 +104,7 @@ log_fcb2_find_gte(struct log *log, struct log_offset *log_offset,
         return SYS_EUNKNOWN;
     }
 #if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-    bmark = log_fcb_closest_bmark(fcb_log, log_offset->lo_index);
+    bmark = log_fcb_closest_bmark(fcb_log, log_offset->lo_index, &min_diff);
     if (bmark != NULL) {
         *out_entry = bmark->lfb_entry;
     }
@@ -169,7 +170,7 @@ log_fcb2_start_append(struct log *log, int len, struct fcb2_entry *loc)
         }
 #endif
 
-#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
+#if MYNEWT_VAL(LOG_FCB_BOOKMARKS) && !MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
         /* The FCB needs to be rotated. */
         log_fcb_rotate_bmarks(fcb_log);
 #endif
@@ -179,6 +180,14 @@ log_fcb2_start_append(struct log *log, int len, struct fcb2_entry *loc)
             goto err;
         }
 
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+        /* The FCB needs to be rotated, reinit previously allocated
+         * bookmarks
+         */
+        log_fcb_init_bmarks(fcb_log, fcb_log->fl_bset.lfs_bmarks,
+                            fcb_log->fl_bset.lfs_cap);
+#endif
+
 #if MYNEWT_VAL(LOG_STORAGE_WATERMARK)
         /*
          * FCB was rotated successfully so let's check if watermark was within
@@ -497,8 +506,8 @@ log_fcb2_walk(struct log *log, log_walk_func_t walk_func,
     /* If a minimum index was specified (i.e., we are not just retrieving the
      * last entry), add a bookmark pointing to this walk's start location.
      */
-    if (log_off->lo_ts >= 0) {
-        log_fcb_add_bmark(fcb_log, &loc, log_off->lo_index);
+    if ((log_off->lo_ts >= 0 && log_off->lo_index > 0) && min_diff != 0) {
+        log_fcb_add_bmark(fcb_log, &loc, log_off->lo_index, false);
     }
 #endif
 
diff --git a/sys/log/full/src/log_fcb_bmark.c b/sys/log/full/src/log_fcb_bmark.c
diff --git a/sys/log/full/src/log_shell.c b/sys/log/full/src/log_shell.c
diff --git a/sys/log/full/syscfg.yml b/sys/log/full/syscfg.yml

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,6 @@ fcb_len_in_flash(struct fcb *fcb, uint16_t len)`
`47`	`47`	`return (len + (fcb->f_align - 1)) & ~(fcb->f_align - 1);`
`48`	`48`	`}`
`49`	`49`
`50`		`-int fcb_getnext_in_area(struct fcb fcb, struct fcb_entry loc);`
`51`	`50`	`struct flash_area fcb_getnext_area(struct fcb fcb, struct flash_area *fap);`
`52`	`51`	`int fcb_getnext_nolock(struct fcb fcb, struct fcb_entry loc);`
`53`	`52`