[PATCH 0/4] rbd: use snapc->seq the way server does

[PATCH 0/4] rbd: use snapc->seq the way server does

[Alex Elder]

This series of patches changes the way the snap context "seq" field
is used.  Currently it is used in a way that isn't really useful, and
as such is a bit confusing.  This behavior seems to be a hold over
from a time when there was no snap_id field maintained for an rbd_dev.

Summary:
[PATCH 1/4] rbd: don't use snapc->seq that way
    Removes special handling in __rbd_refresh_header() that ensured
    the seq field was updated to point to the head if it had been
    at the start of the function.
[PATCH 2/4] rbd: preserve snapc->seq in rbd_header_set_snap()
    Changes rbd_header_set_snap() so it doesn't set the seq field
    to the snapshot id (for a snapshot mapping) or the highest
    snapshot id (for the base image).
[PATCH 3/4] rbd: set snapc->seq only when refreshing header
    Assigns snapc->seq whenever an updated rbd image header is
    received rather than when a new snapshot id has been
    assigned.
[PATCH 4/4] rbd: kill rbd_image_header->snap_seq
    Gets rid of the rbd_image_header->snap_seq field, which
    previously kept the same information now maintained in
    the snapc->seq field.

					-Alex

[PATCH 1/4] rbd: don't use snapc->seq that way

[Alex Elder]

In what appears to be an artifact of a different way of encoding
whether an rbd image maps a snapshot, __rbd_refresh_header() has
code that arranges to update the seq value in an rbd image's
snapshot context to point to the first entry in its snapshot
array if that's where it was pointing initially.

We now use rbd_dev->snap_id to record the snapshot id--using
the special value SNAP_NONE to indicate the rbd_dev is not mapping
a snapshot at all.

There is therefore no need to check for this case, nor to update the
seq value, in __rbd_refresh_header().  Just preserve the seq value
that rbd_read_header() provides (which, at the moment, is nothing).

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4d3a1e0..8a46599 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1718,8 +1718,6 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev)
 {
 	int ret;
 	struct rbd_image_header h;
-	u64 snap_seq;
-	int follow_seq = 0;

 	ret = rbd_read_header(rbd_dev, &h);
 	if (ret < 0)
@@ -1735,13 +1733,6 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev)
 		set_capacity(rbd_dev->disk, size);
 	}

-	snap_seq = rbd_dev->header.snapc->seq;
-	if (rbd_dev->header.total_snaps &&
-	    rbd_dev->header.snapc->snaps[0] == snap_seq)
-		/* pointing at the head, will need to follow that
-		   if head moves */
-		follow_seq = 1;
-
 	/* rbd_dev->header.object_prefix shouldn't change */
 	kfree(rbd_dev->header.snap_sizes);
 	kfree(rbd_dev->header.snap_names);
@@ -1759,11 +1750,6 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev)
 	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
 	kfree(h.object_prefix);

-	if (follow_seq)
-		rbd_dev->header.snapc->seq = rbd_dev->header.snapc->snaps[0];
-	else
-		rbd_dev->header.snapc->seq = snap_seq;
-
 	ret = __rbd_init_snaps_header(rbd_dev);

 	up_write(&rbd_dev->header_rwsem);
-- 
1.7.5.4

Re: [PATCH 1/4] rbd: don't use snapc->seq that way

[Josh Durgin]

On 07/19/2012 10:11 AM, Alex Elder wrote:
> We now use rbd_dev->snap_id to record the snapshot id--using
> the special value SNAP_NONE to indicate the rbd_dev is not mapping
> a snapshot at all.

That's CEPH_NOSNAP, not SNAP_NONE, right? In any case,

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

Re: [PATCH 1/4] rbd: don't use snapc->seq that way

[Alex Elder]

On 07/19/2012 04:02 PM, Josh Durgin wrote:
> On 07/19/2012 10:11 AM, Alex Elder wrote:
>> We now use rbd_dev->snap_id to record the snapshot id--using
>> the special value SNAP_NONE to indicate the rbd_dev is not mapping
>> a snapshot at all.
> 
> That's CEPH_NOSNAP, not SNAP_NONE, right? In any case,

Yes.  I'll fix the description before I commit it...

> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
> 

[PATCH 2/4] rbd: preserve snapc->seq in rbd_header_set_snap()

[Alex Elder]

In rbd_header_set_snap(), there is logic to make the snap context's
seq field get set to a particular snapshot id, or 0 if there is no
snapshot for the rbd image.

This seems to be an artifact of how the current snapshot id for an
rbd_dev was recorded before the rbd_dev->snap_id field began to be
used for that purpose.

There's no need to update the value of snapc->seq here any more, so
stop doing it.  Tidy up a few local variables in that function
while we're at it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8a46599..ac8a83f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -588,29 +588,25 @@ static int snap_by_name(struct rbd_image_header
*header, const char *snap_name,

 static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
 {
-	struct rbd_image_header *header = &rbd_dev->header;
-	struct ceph_snap_context *snapc = header->snapc;
-	int ret = -ENOENT;
+	int ret;

 	down_write(&rbd_dev->header_rwsem);

 	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
-		if (header->total_snaps)
-			snapc->seq = header->snap_seq;
-		else
-			snapc->seq = 0;
 		rbd_dev->snap_id = CEPH_NOSNAP;
 		rbd_dev->snap_exists = false;
 		rbd_dev->read_only = 0;
 		if (size)
-			*size = header->image_size;
+			*size = rbd_dev->header.image_size;
 	} else {
-		ret = snap_by_name(header, rbd_dev->snap_name,
-					&snapc->seq, size);
+		u64 snap_id = 0;
+
+		ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
+					&snap_id, size);
 		if (ret < 0)
 			goto done;
-		rbd_dev->snap_id = snapc->seq;
+		rbd_dev->snap_id = snap_id;
 		rbd_dev->snap_exists = true;
 		rbd_dev->read_only = 1;
 	}
-- 
1.7.5.4

[PATCH 3/4] rbd: set snapc->seq only when refreshing header

[Alex Elder]

In rbd_header_add_snap() there is code to set snapc->seq to the
just-added snapshot id.  This is the only remnant left of the
use of that field for recording which snapshot an rbd_dev was
associated with.  That functionality is no longer supported,
so get rid of that final bit of code.

Doing so means we never actually set snapc->seq any more.  On the
server, the snapshot context's sequence value represents the highest
snapshot id ever issued for a particular rbd image.  So we'll make
it have that meaning here as well.  To do so, set this value
whenever the rbd header is (re-)read.  That way it will always be
consistent with the rest of the snapshot context we maintain.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ac8a83f..c299a55 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -537,6 +537,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,

 	atomic_set(&header->snapc->nref, 1);
 	header->snap_seq = le64_to_cpu(ondisk->snap_seq);
+	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
 	header->snapc->num_snaps = snap_count;
 	header->total_snaps = snap_count;

@@ -1685,14 +1686,7 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,

 	kfree(data);

-	if (ret < 0)
-		return ret;
-
-	down_write(&rbd_dev->header_rwsem);
-	rbd_dev->header.snapc->seq = new_snapid;
-	up_write(&rbd_dev->header_rwsem);
-
-	return 0;
+	return ret < 0 ? ret : 0;
 bad:
 	return -ERANGE;
 }
-- 
1.7.5.4

[PATCH 4/4] rbd: kill rbd_image_header->snap_seq

[Alex Elder]

The snap_seq field in an rbd_image_header structure held the value
from the rbd image header when it was last refreshed.  We now
maintain this value in the snapc->seq field.  So get rid of the
other one.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c299a55..6df8c62 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -82,7 +82,6 @@ struct rbd_image_header {
 	__u8 comp_type;
 	struct ceph_snap_context *snapc;
 	size_t snap_names_len;
-	u64 snap_seq;
 	u32 total_snaps;

 	char *snap_names;
@@ -536,7 +535,6 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	header->comp_type = ondisk->options.comp_type;

 	atomic_set(&header->snapc->nref, 1);
-	header->snap_seq = le64_to_cpu(ondisk->snap_seq);
 	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
 	header->snapc->num_snaps = snap_count;
 	header->total_snaps = snap_count;
-- 
1.7.5.4

Re: [PATCH 0/4] rbd: use snapc->seq the way server does

[Josh Durgin]

On 07/19/2012 10:09 AM, Alex Elder wrote:
> This series of patches changes the way the snap context "seq" field
> is used.  Currently it is used in a way that isn't really useful, and
> as such is a bit confusing.  This behavior seems to be a hold over
> from a time when there was no snap_id field maintained for an rbd_dev.
>
> Summary:
> [PATCH 1/4] rbd: don't use snapc->seq that way
>      Removes special handling in __rbd_refresh_header() that ensured
>      the seq field was updated to point to the head if it had been
>      at the start of the function.
> [PATCH 2/4] rbd: preserve snapc->seq in rbd_header_set_snap()
>      Changes rbd_header_set_snap() so it doesn't set the seq field
>      to the snapshot id (for a snapshot mapping) or the highest
>      snapshot id (for the base image).
> [PATCH 3/4] rbd: set snapc->seq only when refreshing header
>      Assigns snapc->seq whenever an updated rbd image header is
>      received rather than when a new snapshot id has been
>      assigned.
> [PATCH 4/4] rbd: kill rbd_image_header->snap_seq
>      Gets rid of the rbd_image_header->snap_seq field, which
>      previously kept the same information now maintained in
>      the snapc->seq field.
>
> 					-Alex

The rest of the series looks good too.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>