On 08/07/2012 04:27 PM, Josh Durgin wrote:
On 08/06/2012 10:45 AM, Alex Elder wrote:
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@xxxxxxxxxxx>
This is more readable to me as well, but a few nits below. Other than
those, Reviewed-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
See responses below.
---
drivers/block/rbd.c | 166
+++++++++++++++++++++++++++-------------------------
1 file changed, 88 insertions(+), 78 deletions(-)
Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2066,98 +2066,108 @@ err:
return ERR_PTR(ret);
}
+/* These might otherwise belong in <linux/list.h> */
+
+/* Return the next list entry, or a null pointer if there are no more */
+
+#define list_next_entry(list, head, type, member) \
+ (list_is_last(list, head) ? NULL : list_entry(list, type, member))
+
This isn't used in this patch, is it used later?
I guess not. I did need it at one time but I apparently did
away with that approach. I'll delete this.
/*
- * search for the previous snap in a null delimited string list
+ * Insert a new entry before the given next one in the list. Adjust
+ * the list header if appropriate.
*/
-const char *rbd_prev_snap_name(const char *name, const char *start)
+static inline void list_insert(struct list_head *new,
+ struct list_head *next,
+ struct list_head *head)
It looks like list_add_tail(new, next) does what we need in the one
place that uses this.
You know, I looked hard at that and convinced myself that I
needed to update the head pointer if inserting something
at the front of the list. And although I my gut sort of
rejected that I still managed to remain confused.
You are right though, and I'll use that and get rid of this
definition.
{
- if (name < start + 2)
- return NULL;
+ struct list_head *prev = next->prev;
- name -= 2;
- while (*name) {
- if (name == start)
- return start;
- name--;
- }
- return name + 1;
+ new->prev = prev;
+ prev->next = new;
+ new->next = next;
+ next->prev = new;
+ if (next == head)
+ head->next = new;
}
/*
- * 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;
-
- first_name = rbd_dev->header.snap_names;
- name = first_name + rbd_dev->header.snap_names_len;
-
- list_for_each_prev_safe(p, n, &rbd_dev->snaps) {
- u64 cur_id;
-
- old_snap = list_entry(p, struct rbd_snap, node);
-
- if (i)
- cur_id = rbd_dev->header.snapc->snaps[i - 1];
-
- 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)
+ struct ceph_snap_context * const snapc = rbd_dev->header.snapc;
Putting the const after the type looks odd to me, especially when it's
the opposite on the next line.
It is correct, but in this small function it's not really
necessary. The two mean different things.
struct foo *
modifiable pointer to modifiable data of type struct foo
struct foo * const
modifiable pointer to constant data of type struct foo
struct foo const *
or const struct foo *
constant pointer to modifiable data of type struct foo
struct foo const * const
or const struct foo * const
constant pointer to constant data of type struct foo
So the placement is significant, and it has to do with whether
the pointer itself can be modified versus whether what it points
to can be modified. The "const" can go before or after the
portion of the type specification it modifies. You almost never
see the "struct foo const *" form in Linux though.
+ const u32 snap_count = snapc->num_snaps;
+ char *snap_name = rbd_dev->header.snap_names;
+ struct list_head * const head = &rbd_dev->snaps;
same here
Same here.
When this is used inside a function (as opposed to use in
a function prototype) it conveys some information but it's
not that important, so I'll just drop them.
+ struct list_head *links = head->next;
+ u32 index = 0;
+
+ while (index < snap_count || links != head) {
+ u64 snap_id;
+ struct rbd_snap *snap;
+
+ 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);
+
+ if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
+ struct list_head *next = links->next;
+
+ /* Existing snapshot not in the new snap context */
+
+ 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;
This could just be links = links->next like you have later.
I just used "next" because I had it available here but not
below. I do like consistency though so I'll make your
suggested change.
Thanks a lot for your review.
-Alex
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_insert(&new_snap->node, &snap->node, head);
+ 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;
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
