From patchwork Mon Aug 6 17:45:56 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Elder X-Patchwork-Id: 1280591 Return-Path: X-Original-To: patchwork-ceph-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 422423FC23 for ; Mon, 6 Aug 2012 17:46:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756409Ab2HFRqA (ORCPT ); Mon, 6 Aug 2012 13:46:00 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:49552 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754224Ab2HFRp7 (ORCPT ); Mon, 6 Aug 2012 13:45:59 -0400 Received: by pbbrr13 with SMTP id rr13so2884408pbb.19 for ; Mon, 06 Aug 2012 10:45:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject :content-type:content-transfer-encoding:x-gm-message-state; bh=M4agGglthRyod0aPGTPV3AcKVpxSsdz5IbppCUnZMhc=; b=iZxZTb6Gn2J3QaXUpL13XoXmzg8HlKpnyHJshqWlBIdtnfVsbGGbabvmubqU3nj2Hl 9rDqJt1RboVTb67CxjrHWx5iFruTB1t9oEbPMBiEqvkg7BAA1GGCazzWbQjfDtUQrbWv WYjnccPGA5L0odWtQo9BJ1XMQhN6HVsx2Fm4empaEVbYUTrWZ+pucIL4aCiR3d1aBKG/ XFajWHqpuUB+oR+nOhuV7i1/2lTDy+Nad+YBBUESZ7LFNjuNQgoZ8kDNzbKQndCtrY33 8QAbzRYwAfoNp4cOma7vuS68Umqc5lA4nuaNFeoZw//CJogP+cMIWA2oUMbiCvae8TnG h1jQ== Received: by 10.68.130.9 with SMTP id oa9mr20661344pbb.95.1344275158533; Mon, 06 Aug 2012 10:45:58 -0700 (PDT) Received: from ?IPv6:2607:f298:a:607:3c9c:52cb:843e:71a9? ([2607:f298:a:607:3c9c:52cb:843e:71a9]) by mx.google.com with ESMTPS id rp9sm1499560pbc.52.2012.08.06.10.45.57 (version=SSLv3 cipher=OTHER); Mon, 06 Aug 2012 10:45:57 -0700 (PDT) Message-ID: <502002D4.6070701@inktank.com> Date: Mon, 06 Aug 2012 10:45:56 -0700 From: Alex Elder User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: "ceph-devel@vger.kernel.org" Subject: [PATCH] rbd: simplify __rbd_init_snaps_header() X-Gm-Message-State: ALoCoQnrnXC59WSQRtd11zGpBnVUna/3i0r2lne7xbSFGEl2ouZoIBI9nDgPXIA82p/UzBik4LpK Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org 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 --- drivers/block/rbd.c | 166 +++++++++++++++++++++++++++------------------------- 1 file changed, 88 insertions(+), 78 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 */ + +/* 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)) + /* - * 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) { - 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; + const u32 snap_count = snapc->num_snaps; + char *snap_name = rbd_dev->header.snap_names; + struct list_head * const head = &rbd_dev->snaps; + 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; 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;