[PATCH 0/2] rbd: snap context references

[PATCH 0/2] rbd: snap context references

[Alex Elder]

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

[PATCH 1/2] rbd: fix reference leak in rbd_do_op()

[Alex Elder]

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.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 835153e..b86f5e5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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

[PATCH 2/2] rbd: only get snap context for write requests

[Alex Elder]

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.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b86f5e5..bac4304 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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