rbd: locking, snapshot registration, snap_by_name()

Move the calls to get the header semaphore out of
rbd_header_set_snap() and into its caller.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

drivers/block/rbd.c +0 -2

index 634a16c..214c937 100644
@@ -647,8 +647,6 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, char *snap_name)
  {
  	int ret;
 
-	down_write(&rbd_dev->header_rwsem);
-
  	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
  		    sizeof (RBD_SNAP_HEAD_NAME))) {

  		rbd_dev->mapping.snap_id = CEPH_NOSNAP;

drivers/block/rbd.c +2 -1

@@ -666,7 +664,6 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, char *snap_name)
 
  	ret = 0;
  done:
-	up_write(&rbd_dev->header_rwsem);
  	return ret;
  }

@@ -2608,7 +2605,9 @@ static ssize_t rbd_add(struct bus_type *bus,
  	if (rc)
  		goto err_out_bus;
 
+	down_write(&rbd_dev->header_rwsem);
  	rc = rbd_header_set_snap(rbd_dev, snap_name);
+	up_write(&rbd_dev->header_rwsem);
  	if (rc)
  		goto err_out_bus;
 

Expand the region of code in rbd_add() protected by the header semaphore to include the complete initialization sequence. It may not be strictly necessary, but it doesn't hurt. And with the upcoming changes to the order of steps here this offers easy protection.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   16 ++++++++++------
  1 file changed, 10 insertions(+), 6 deletions(-)

drivers/block/rbd.c +10 -6

index 214c937..6af09f1 100644
@@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  	INIT_LIST_HEAD(&rbd_dev->snaps);
  	init_rwsem(&rbd_dev->header_rwsem);
 
+	down_write(&rbd_dev->header_rwsem);
+
  	/* generate unique id: find highest unique id, add one */
  	rbd_dev_id_get(rbd_dev);
 

@@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
  	/* contact OSD, request size info about the object being mapped */
  	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
  	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;
 
-	/* no need to lock here, as rbd_dev is not registered yet */
  	rc = rbd_dev_snap_devs_update(rbd_dev);
  	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;
 
-	down_write(&rbd_dev->header_rwsem);
  	rc = rbd_header_set_snap(rbd_dev, snap_name);
-	up_write(&rbd_dev->header_rwsem);
  	if (rc)
-		goto err_out_bus;
+		goto err_out_unlock;
+
+	up_write(&rbd_dev->header_rwsem);
 
  	/* Set up the blkdev mapping. */
 

@@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 
  	return count;
 
+err_out_unlock:
+	up_write(&rbd_dev->header_rwsem);
  err_out_bus:
  	/* this will also clean up rest of rbd_dev stuff */
 

@@ -2649,6 +2652,7 @@ err_put_id:
  		kfree(rbd_dev->pool_name);
  	}
  	rbd_dev_id_put(rbd_dev);
+	up_write(&rbd_dev->header_rwsem);
  err_nomem:
  	kfree(rbd_dev);
  	kfree(options);

Looking closer, I don't think we need to protect this section at all. The device isn't initialized yet, so nothing else can access the rbd_dev->header. It'd be good to have a comment to that effect.

Josh

Quoted 66 lines

Expand the region of code in rbd_add() protected by the header semaphore to include the complete initialization sequence. It may not be strictly necessary, but it doesn't hurt. And with the upcoming changes to the order of steps here this offers easy protection.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   16 ++++++++++------
  1 file changed, 10 insertions(+), 6 deletions(-)

drivers/block/rbd.c +10 -6

index 214c937..6af09f1 100644
@@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
      INIT_LIST_HEAD(&rbd_dev->snaps);
      init_rwsem(&rbd_dev->header_rwsem);
 
+    down_write(&rbd_dev->header_rwsem);
+
      /* generate unique id: find highest unique id, add one */
      rbd_dev_id_get(rbd_dev);
 

@@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
      /* contact OSD, request size info about the object being mapped */
      rc = rbd_read_header(rbd_dev, &rbd_dev->header);
      if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
 
-    /* no need to lock here, as rbd_dev is not registered yet */
      rc = rbd_dev_snap_devs_update(rbd_dev);
      if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
 
-    down_write(&rbd_dev->header_rwsem);
      rc = rbd_header_set_snap(rbd_dev, snap_name);
-    up_write(&rbd_dev->header_rwsem);
      if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
+
+    up_write(&rbd_dev->header_rwsem);
 
      /* Set up the blkdev mapping. */
 

@@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 
      return count;
 
+err_out_unlock:
+    up_write(&rbd_dev->header_rwsem);
  err_out_bus:
      /* this will also clean up rest of rbd_dev stuff */
 

@@ -2649,6 +2652,7 @@ err_put_id:
          kfree(rbd_dev->pool_name);
      }
      rbd_dev_id_put(rbd_dev);
+    up_write(&rbd_dev->header_rwsem);
  err_nomem:
      kfree(rbd_dev);
      kfree(options);

Quoted 5 lines · Josh Durgin

Looking closer, I don't think we need to protect this section at all. The device isn't initialized yet, so nothing else can access the rbd_dev->header. It'd be good to have a comment to that effect.

Josh

For the most part that's true.

However as soon as it's possible to refresh the header, the lock should be held. And in the code as of this point that happens as soon as rbd_bus_add_dev() gets called. In that respect this change fixes a bug, because the header refresh could have been initiated before the header had even been read yet.

Upcoming patches rearrange the order of this stuff quite a bit though, which was why I just grabbed the lock immediately and held onto it until everything is set up, for simplicity.

After all of these are in place I can reduce the range over which the lock is held during initialization. But as I said in the commit commit, it doesn't really hurt to hold it. I will add a comment in the code to that effect before I commit.

-Alex

Quoted 71 lines · Alex Elder

Quoted 66 lines

Expand the region of code in rbd_add() protected by the header semaphore to include the complete initialization sequence. It may not be strictly necessary, but it doesn't hurt. And with the upcoming changes to the order of steps here this offers easy protection.

Signed-off-by Alex Elder <[email protected]>

---
   drivers/block/rbd.c |   16 ++++++++++------
   1 file changed, 10 insertions(+), 6 deletions(-)

drivers/block/rbd.c +10 -6

index 214c937..6af09f1 100644
@@ -2553,6 +2553,8 @@ static ssize_t rbd_add(struct bus_type *bus,
       INIT_LIST_HEAD(&rbd_dev->snaps);
       init_rwsem(&rbd_dev->header_rwsem);
 
+    down_write(&rbd_dev->header_rwsem);
+
       /* generate unique id: find highest unique id, add one */
       rbd_dev_id_get(rbd_dev);
 

@@ -2598,18 +2600,17 @@ static ssize_t rbd_add(struct bus_type *bus,
       /* contact OSD, request size info about the object being mapped */
       rc = rbd_read_header(rbd_dev, &rbd_dev->header);
       if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
 
-    /* no need to lock here, as rbd_dev is not registered yet */
       rc = rbd_dev_snap_devs_update(rbd_dev);
       if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
 
-    down_write(&rbd_dev->header_rwsem);
       rc = rbd_header_set_snap(rbd_dev, snap_name);
-    up_write(&rbd_dev->header_rwsem);
       if (rc)
-        goto err_out_bus;
+        goto err_out_unlock;
+
+    up_write(&rbd_dev->header_rwsem);
 
       /* Set up the blkdev mapping. */
 

@@ -2630,6 +2631,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 
       return count;
 
+err_out_unlock:
+    up_write(&rbd_dev->header_rwsem);
   err_out_bus:
       /* this will also clean up rest of rbd_dev stuff */
 

@@ -2649,6 +2652,7 @@ err_put_id:
           kfree(rbd_dev->pool_name);
       }
       rbd_dev_id_put(rbd_dev);
+    up_write(&rbd_dev->header_rwsem);
   err_nomem:
       kfree(rbd_dev);
       kfree(options);

When rbd_bus_add_dev() is called (one spot--in rbd_add()), the rbd image header has not even been read yet. This means that the list of snapshots will be empty at the time of the call. As a result, there is no need for the code that calls rbd_register_snap_dev() for each entry in that list--so get rid of it.

Once the header has been read (just after returning), a call will be made to rbd_dev_snap_devs_update(), which will then find every snapshot in the context to be new and will therefore call rbd_register_snap_dev() via __rbd_add_snap_dev() accomplishing the same thing.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   14 +++-----------
  1 file changed, 3 insertions(+), 11 deletions(-)

drivers/block/rbd.c +3 -11

index 6af09f1..e922989 100644
@@ -2244,29 +2244,21 @@ static int rbd_dev_snap_devs_update(struct
 rbd_device *rbd_dev)
 
  static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
  {
-	int ret;
  	struct device *dev;
-	struct rbd_snap *snap;
+	int ret;
 
  	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	dev = &rbd_dev->dev;
 
+	dev = &rbd_dev->dev;
  	dev->bus = &rbd_bus_type;
  	dev->type = &rbd_device_type;
  	dev->parent = &rbd_root_dev;
  	dev->release = rbd_dev_release;
  	dev_set_name(dev, "%d", rbd_dev->dev_id);
  	ret = device_register(dev);
-	if (ret < 0)
-		goto out;
 
-	list_for_each_entry(snap, &rbd_dev->snaps, node) {
-		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
-		if (ret < 0)
-			break;
-	}
-out:
  	mutex_unlock(&ctl_mutex);
+
  	return ret;
  }

An rbd_dev structure maintains a list of current snapshots that have already been fully initialized. The entries on the list have type struct rbd_snap, and each entry contains a copy of information that's found in the rbd_dev's snapshot context and header.

The only caller of snap_by_name() is rbd_header_set_snap(). In that call site any positive return value (the index in the snapshot array) is ignored.

rbd_header_set_snap() also has only one caller--rbd_add()--and that call is made after a call to rbd_dev_snap_devs_update(). Because the rbd_snap structures are initialized in that function, the current snapshot list can be used instead of the snapshot context to look up a snapshot's information by name.

Change snap_by_name() so it uses the snapshot list rather than the rbd_dev's snapshot context in looking up snapshot information. Return 0 if it's found rather than the snapshot id. To do this, change rbd_snap_by_name() to take an rbd_dev rather than rbd_image_header structure pointer as its first argument.

No caller ever passes a null pointer to snap_by_name() for the snapshot id or the size, so just assign using those pointers unconditionally.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   19 +++++++------------
  1 file changed, 7 insertions(+), 12 deletions(-)

drivers/block/rbd.c +7 -12

index e922989..4dff92f 100644
@@ -623,23 +623,18 @@ out_err:
 
  static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
  {
-	int i;
-	struct rbd_image_header *header = &rbd_dev->header;
-	char *p = header->snap_names;
-
-	rbd_assert(header->snapc != NULL);
-	for (i = 0; i < header->snapc->num_snaps; i++) {
-		if (!strcmp(snap_name, p)) {
 
-			/* Found it.  Pass back its id and/or size */
+	struct rbd_snap *snap;
 
-			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
-			rbd_dev->mapping.size = header->snap_sizes[i];
+	list_for_each_entry(snap, &rbd_dev->snaps, node) {
+		if (!strcmp(snap_name, snap->name)) {
+			rbd_dev->mapping.snap_id = snap->id;
+			rbd_dev->mapping.size = snap->size;
 
-			return i;
+			return 0;
  		}
-		p += strlen(p) + 1;	/* Skip ahead to the next name */
  	}
+
  	return -ENOENT;
  }
 

Quoted 15 lines · Alex Elder

An rbd_dev structure maintains a list of current snapshots that have already been fully initialized. The entries on the list have type struct rbd_snap, and each entry contains a copy of information that's found in the rbd_dev's snapshot context and header.

The only caller of snap_by_name() is rbd_header_set_snap(). In that call site any positive return value (the index in the snapshot array) is ignored.

rbd_header_set_snap() also has only one caller--rbd_add()--and that call is made after a call to rbd_dev_snap_devs_update(). Because the rbd_snap structures are initialized in that function, the current snapshot list can be used instead of the snapshot context to look up a snapshot's information by name.

Quoted 5 lines

Change snap_by_name() so it uses the snapshot list rather than the rbd_dev's snapshot context in looking up snapshot information. Return 0 if it's found rather than the snapshot id. To do this, change rbd_snap_by_name() to take an rbd_dev rather than rbd_image_header structure pointer as its first argument.

This argument change was an earlier commit now.

Quoted 3 lines

No caller ever passes a null pointer to snap_by_name() for the snapshot id or the size, so just assign using those pointers unconditionally.

No caller passes id or size now either.

With the commit message fixed:

Reviewed-by Josh Durgin <[email protected]>

Quoted 41 lines

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   19 +++++++------------
  1 file changed, 7 insertions(+), 12 deletions(-)

drivers/block/rbd.c +7 -12

index e922989..4dff92f 100644
@@ -623,23 +623,18 @@ out_err:
 
  static int snap_by_name(struct rbd_device *rbd_dev, const char
 *snap_name)
  {
-    int i;
-    struct rbd_image_header *header = &rbd_dev->header;
-    char *p = header->snap_names;
-
-    rbd_assert(header->snapc != NULL);
-    for (i = 0; i < header->snapc->num_snaps; i++) {
-        if (!strcmp(snap_name, p)) {
 
-            /* Found it.  Pass back its id and/or size */
+    struct rbd_snap *snap;
 
-            rbd_dev->mapping.snap_id = header->snapc->snaps[i];
-            rbd_dev->mapping.size = header->snap_sizes[i];
+    list_for_each_entry(snap, &rbd_dev->snaps, node) {
+        if (!strcmp(snap_name, snap->name)) {
+            rbd_dev->mapping.snap_id = snap->id;
+            rbd_dev->mapping.size = snap->size;
 
-            return i;
+            return 0;
          }
-        p += strlen(p) + 1;    /* Skip ahead to the next name */
      }
+
      return -ENOENT;
  }