rbd: snap context references

These two patches are related to how snapshot context reference counting is handled in rbd_do_op().

-Alex

[PATCH 1/2] rbd: fix reference leak in rbd_do_op() [PATCH 2/2] rbd: only get snap context for write requests

This commit introduced a bug:

    4634246d rbd: consolidate rbd_do_op() calls

When a read request is being issued, the snapshot context provided isn't needed. But that snap context pointer was acquired in rbd_rq_fn() and it carries with it a reference to that structure. So before discarding it, the reference needs to be dropped.

---
 drivers/block/rbd.c |    1 +
 1 file changed, 1 insertion(+)

index 835153e..b86f5e5 100644
@@ -1333,6 +1333,7 @@ static int rbd_do_op(struct request *rq,
 	} else {
 		opcode = CEPH_OSD_OP_READ;
 		flags = CEPH_OSD_FLAG_READ;
+		ceph_put_snap_context(snapc);
 		snapc = NULL;
 		snapid = rbd_dev->spec->snap_id;
 		payload_len = 0;

1.7.9.5

Right now we get and release the header semaphore every time we process a request for an rbd image. We do this because for write requests we need to supply the snapshot context, and we can't safely get a reference to it without holding that semaphore.

There's no need to get the snap context if we're doing a read, so avoid doing so in that case.

The rbd_device->exists field can be updated asynchronously, changing from set to clear if a mapped snapshot disappears from the base image's snapshot context. Although I don't think synchronizing access to this carefully is that critical, it is converted here to be an atomic variable so a request is aware the flag is clear as soon as that is known.

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

index b86f5e5..bac4304 100644
@@ -229,7 +229,7 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */
 
 	struct rbd_image_header	header;
-	bool                    exists;
+	atomic_t		exists;
 	struct rbd_spec		*spec;
 
 	char			*header_name;

@@ -753,7 +753,7 @@ static int rbd_dev_set_mapping(struct rbd_device

*rbd_dev)
goto done;
rbd_dev->mapping.read_only = true;
}
- rbd_dev->exists = true;
+ atomic_set(&rbd_dev->exists, 1);
done:
return ret;
}
@@ -1333,8 +1333,7 @@ static int rbd_do_op(struct request *rq,
} else {
opcode = CEPH_OSD_OP_READ;
flags = CEPH_OSD_FLAG_READ;
- ceph_put_snap_context(snapc);
- snapc = NULL;
+ rbd_assert(!snapc);
snapid = rbd_dev->spec->snap_id; payload_len = 0;
}
@@ -1652,6 +1651,7 @@ static void rbd_rq_fn(struct request_queue *q)

while ((rq = blk_fetch_request(q))) {
struct ceph_snap_context *snapc = NULL;
+ bool write_request = rq_data_dir(rq) == WRITE;
int result;

dout("fetched request\n");
@@ -1667,19 +1667,17 @@ static void rbd_rq_fn(struct request_queue *q)
/* Stop writes to a read-only device */

result = -EROFS;
- if (read_only && rq_data_dir(rq) == WRITE)
+ if (read_only && write_request)
goto out_end_request;

- /* Grab a reference to the snapshot context */
+ /* Grab a reference to the snapshot context if needed */

- down_read(&rbd_dev->header_rwsem);
- if (rbd_dev->exists) {
+ if (write_request) {
+ down_read(&rbd_dev->header_rwsem);
snapc = ceph_get_snap_context(rbd_dev->header.snapc); rbd_assert(snapc != NULL);
- }
- up_read(&rbd_dev->header_rwsem);
-
- if (!snapc) {
+ up_read(&rbd_dev->header_rwsem);
+ } else if (!atomic_read(&rbd_dev->exists)) {
rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP); dout("request for non-existent snapshot"); result = -ENXIO;
@@ -2295,6 +2293,7 @@ struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
return NULL;

spin_lock_init(&rbd_dev->lock);
+ atomic_set(&rbd_dev->exists, 0);
INIT_LIST_HEAD(&rbd_dev->node); INIT_LIST_HEAD(&rbd_dev->snaps); init_rwsem(&rbd_dev->header_rwsem);
@@ -2919,7 +2918,7 @@ static int rbd_dev_snaps_update(struct rbd_device *rbd_dev)
/* Existing snapshot not in the new snap context */

if (rbd_dev->spec->snap_id == snap->id)
- rbd_dev->exists = false;
+ atomic_set(&rbd_dev->exists, 0);
rbd_remove_snap_dev(snap); dout("%ssnap id %llu has been removed\n",
rbd_dev->spec->snap_id == snap->id ?

1.7.9.5