rbd: use snapc->seq the way server does

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

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).

---
 drivers/block/rbd.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

index 4d3a1e0..8a46599 100644
@@ -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

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

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

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.

---
 drivers/block/rbd.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

index 8a46599..ac8a83f 100644
@@ -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

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.

---
 drivers/block/rbd.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

index ac8a83f..c299a55 100644
@@ -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

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.

---
 drivers/block/rbd.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index c299a55..6df8c62 100644
@@ -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

The rest of the series looks good too.