rbd: simplify __rbd_init_snaps_header()

The purpose of __rbd_init_snaps_header() is to compare a new
snapshot context with an rbd device's list of existing snapshots.
It updates the list by adding any new snapshots or removing any
that are not present in the new snapshot context.

The code as written is a little confusing, because it traverses both
the existing snapshot list and the set of snapshots in the snapshot
context in reverse.  This was done based on an assumption about
snapshots that is not true--namely that a duplicate snapshot name
could cause an error in intepreting things if they were not
processed in ascending order.

These precautions are not necessary, because:
    - all snapshots are uniquely identified by their snapshot id
    - a new snapshot cannot be created if the rbd device has another
      snapshot with the same name
(It is furthermore not currently possible to rename a snapshot.)

This patch re-implements __rbd_init_snaps_header() so it passes
through both the existing snapshot list and the entries in the
snapshot context in forward order.  It still does the same thing
as before, but I find the logic considerably easier to understand.

By going forward through the names in the snapshot context, there
is no longer a need for the rbd_prev_snap_name() helper function.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 54a55f0..bf418a6 100644 (file)
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2066,97 +2066,82 @@ err:
 }
 
 /*
- * search for the previous snap in a null delimited string list
- */
-const char *rbd_prev_snap_name(const char *name, const char *start)
-{
-       if (name < start + 2)
-               return NULL;
-
-       name -= 2;
-       while (*name) {
-               if (name == start)
-                       return start;
-               name--;
-       }
-       return name + 1;
-}
-
-/*
- * compare the old list of snapshots that we have to what's in the header
- * and update it accordingly. Note that the header holds the snapshots
- * in a reverse order (from newest to oldest) and we need to go from
- * older to new so that we don't get a duplicate snap name when
- * doing the process (e.g., removed snapshot and recreated a new
- * one with the same name.
+ * Scan the rbd device's current snapshot list and compare it to the
+ * newly-received snapshot context.  Remove any existing snapshots
+ * not present in the new snapshot context.  Add a new snapshot for
+ * any snaphots in the snapshot context not in the current list.
+ * And verify there are no changes to snapshots we already know
+ * about.
+ *
+ * Assumes the snapshots in the snapshot context are sorted by
+ * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
+ * are also maintained in that order.)
  */
 static int __rbd_init_snaps_header(struct rbd_device *rbd_dev)
 {
-       const char *name, *first_name;
-       int i = rbd_dev->header.total_snaps;
-       struct rbd_snap *snap, *old_snap = NULL;
-       struct list_head *p, *n;
+       struct ceph_snap_context *snapc = rbd_dev->header.snapc;
+       const u32 snap_count = snapc->num_snaps;
+       char *snap_name = rbd_dev->header.snap_names;
+       struct list_head *head = &rbd_dev->snaps;
+       struct list_head *links = head->next;
+       u32 index = 0;
 
-       first_name = rbd_dev->header.snap_names;
-       name = first_name + rbd_dev->header.snap_names_len;
+       while (index < snap_count || links != head) {
+               u64 snap_id;
+               struct rbd_snap *snap;
 
-       list_for_each_prev_safe(p, n, &rbd_dev->snaps) {
-               u64 cur_id;
+               snap_id = index < snap_count ? snapc->snaps[index]
+                                            : CEPH_NOSNAP;
+               snap = links != head ? list_entry(links, struct rbd_snap, node)
+                                    : NULL;
+               BUG_ON(snap && snap->id == CEPH_NOSNAP);
 
-               old_snap = list_entry(p, struct rbd_snap, node);
+               if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
+                       struct list_head *next = links->next;
 
-               if (i)
-                       cur_id = rbd_dev->header.snapc->snaps[i - 1];
+                       /* Existing snapshot not in the new snap context */
 
-               if (!i || old_snap->id < cur_id) {
-                       /*
-                        * old_snap->id was skipped, thus was
-                        * removed.  If this rbd_dev is mapped to
-                        * the removed snapshot, record that it no
-                        * longer exists, to prevent further I/O.
-                        */
-                       if (rbd_dev->snap_id == old_snap->id)
+                       if (rbd_dev->snap_id == snap->id)
                                rbd_dev->snap_exists = false;
-                       __rbd_remove_snap_dev(old_snap);
-                       continue;
-               }
-               if (old_snap->id == cur_id) {
-                       /* we have this snapshot already */
-                       i--;
-                       name = rbd_prev_snap_name(name, first_name);
+                       __rbd_remove_snap_dev(snap);
+
+                       /* Done with this list entry; advance */
+
+                       links = next;
                        continue;
                }
-               for (; i > 0;
-                    i--, name = rbd_prev_snap_name(name, first_name)) {
-                       if (!name) {
-                               WARN_ON(1);
-                               return -EINVAL;
-                       }
-                       cur_id = rbd_dev->header.snapc->snaps[i];
-                       /* snapshot removal? handle it above */
-                       if (cur_id >= old_snap->id)
-                               break;
-                       /* a new snapshot */
-                       snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
-                       if (IS_ERR(snap))
-                               return PTR_ERR(snap);
-
-                       /* note that we add it backward so using n and not p */
-                       list_add(&snap->node, n);
-                       p = &snap->node;
-               }
-       }
-       /* we're done going over the old snap list, just add what's left */
-       for (; i > 0; i--) {
-               name = rbd_prev_snap_name(name, first_name);
-               if (!name) {
-                       WARN_ON(1);
-                       return -EINVAL;
+
+               if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
+                       struct rbd_snap *new_snap;
+
+                       /* We haven't seen this snapshot before */
+
+                       new_snap = __rbd_add_snap_dev(rbd_dev, index,
+                                                       snap_name);
+                       if (IS_ERR(new_snap))
+                               return PTR_ERR(new_snap);
+
+                       /* New goes before existing, or at end of list */
+
+                       if (snap)
+                               list_add_tail(&new_snap->node, &snap->node);
+                       else
+                               list_add(&new_snap->node, head);
+               } else {
+                       /* Already have this one */
+
+                       BUG_ON(snap->size != rbd_dev->header.snap_sizes[index]);
+                       BUG_ON(strcmp(snap->name, snap_name));
+
+                       /* Done with this list entry; advance */
+
+                       links = links->next;
                }
-               snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
-               if (IS_ERR(snap))
-                       return PTR_ERR(snap);
-               list_add(&snap->node, &rbd_dev->snaps);
+
+               /* Advance to the next entry in the snapshot context */
+
+               index++;
+               snap_name += strlen(snap_name) + 1;
        }
 
        return 0;