* [PATCH REPOST 0/2] rbd: only get snap context for write requests @ 2013-01-03 22:53 Alex Elder 2013-01-03 22:53 ` [PATCH REPOST 1/2] rbd: make exists flag atomic Alex Elder 2013-01-03 22:54 ` [PATCH REPOST 2/2] rbd: only get snap context for write requests Alex Elder 0 siblings, 2 replies; 5+ messages in thread From: Alex Elder @ 2013-01-03 22:53 UTC (permalink / raw) To: ceph-devel@vger.kernel.org This series stops acquiring the snap context for read requests. -Alex [PATCH REPOST 1/2] rbd: make exists flag atomic [PATCH REPOST 2/2] rbd: only get snap context for write requests ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH REPOST 1/2] rbd: make exists flag atomic 2013-01-03 22:53 [PATCH REPOST 0/2] rbd: only get snap context for write requests Alex Elder @ 2013-01-03 22:53 ` Alex Elder 2013-01-16 0:29 ` Josh Durgin 2013-01-03 22:54 ` [PATCH REPOST 2/2] rbd: only get snap context for write requests Alex Elder 1 sibling, 1 reply; 5+ messages in thread From: Alex Elder @ 2013-01-03 22:53 UTC (permalink / raw) To: ceph-devel@vger.kernel.org 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. Currently, value of the "exists" flag is only read and modified under protection of the header semaphore, but that will change with the next patch. Making it atomic ensures this won't be a problem because the a the non-existence of device will be immediately known. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 1348825..a3b0d43 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; @@ -751,7 +751,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; } @@ -1670,7 +1670,7 @@ static void rbd_rq_fn(struct request_queue *q) /* Grab a reference to the snapshot context */ down_read(&rbd_dev->header_rwsem); - if (rbd_dev->exists) { + if (atomic_read(&rbd_dev->exists)) { snapc = ceph_get_snap_context(rbd_dev->header.snapc); rbd_assert(snapc != NULL); } @@ -2292,6 +2292,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); @@ -2916,7 +2917,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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST 1/2] rbd: make exists flag atomic 2013-01-03 22:53 ` [PATCH REPOST 1/2] rbd: make exists flag atomic Alex Elder @ 2013-01-16 0:29 ` Josh Durgin 0 siblings, 0 replies; 5+ messages in thread From: Josh Durgin @ 2013-01-16 0:29 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel@vger.kernel.org On 01/03/2013 02:53 PM, Alex Elder wrote: > 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. > > Currently, value of the "exists" flag is only read and modified > under protection of the header semaphore, but that will change with > the next patch. Making it atomic ensures this won't be a problem > because the a the non-existence of device will be immediately known. > > Signed-off-by: Alex Elder <elder@inktank.com> > --- Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > drivers/block/rbd.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 1348825..a3b0d43 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; > @@ -751,7 +751,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; > } > @@ -1670,7 +1670,7 @@ static void rbd_rq_fn(struct request_queue *q) > /* Grab a reference to the snapshot context */ > > down_read(&rbd_dev->header_rwsem); > - if (rbd_dev->exists) { > + if (atomic_read(&rbd_dev->exists)) { > snapc = ceph_get_snap_context(rbd_dev->header.snapc); > rbd_assert(snapc != NULL); > } > @@ -2292,6 +2292,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); > @@ -2916,7 +2917,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 ? > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH REPOST 2/2] rbd: only get snap context for write requests 2013-01-03 22:53 [PATCH REPOST 0/2] rbd: only get snap context for write requests Alex Elder 2013-01-03 22:53 ` [PATCH REPOST 1/2] rbd: make exists flag atomic Alex Elder @ 2013-01-03 22:54 ` Alex Elder 2013-01-16 1:23 ` Josh Durgin 1 sibling, 1 reply; 5+ messages in thread From: Alex Elder @ 2013-01-03 22:54 UTC (permalink / raw) To: ceph-devel@vger.kernel.org Right now we get the snapshot context for an rbd image (under protection of the header semaphore) for every request processed. There's no need to get the snap context if we're doing a read, so avoid doing so in that case. Note that we no longer need to hold the header semaphore to check the rbd_dev's existence flag. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index a3b0d43..fd6a708 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1331,7 +1331,7 @@ static int rbd_do_op(struct request *rq, } else { opcode = CEPH_OSD_OP_READ; flags = CEPH_OSD_FLAG_READ; - snapc = NULL; + rbd_assert(!snapc); snapid = rbd_dev->spec->snap_id; payload_len = 0; } @@ -1661,22 +1661,25 @@ static void rbd_rq_fn(struct request_queue *q) } spin_unlock_irq(q->queue_lock); - /* Stop writes to a read-only device */ - - result = -EROFS; - if (read_only && rq_data_dir(rq) == WRITE) - goto out_end_request; - - /* Grab a reference to the snapshot context */ - - down_read(&rbd_dev->header_rwsem); - if (atomic_read(&rbd_dev->exists)) { + /* Write requests need a reference to the snapshot context */ + + if (rq_data_dir(rq) == WRITE) { + result = -EROFS; + if (read_only) /* Can't write to a read-only device */ + goto out_end_request; + + /* + * Note that each osd request will take its + * own reference to the snapshot context + * supplied. The reference we take here + * just guarantees the one we provide stays + * valid. + */ + down_read(&rbd_dev->header_rwsem); snapc = ceph_get_snap_context(rbd_dev->header.snapc); + up_read(&rbd_dev->header_rwsem); rbd_assert(snapc != NULL); - } - up_read(&rbd_dev->header_rwsem); - - if (!snapc) { + } else if (!atomic_read(&rbd_dev->exists)) { rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP); dout("request for non-existent snapshot"); result = -ENXIO; @@ -1687,7 +1690,8 @@ static void rbd_rq_fn(struct request_queue *q) blk_rq_pos(rq) * SECTOR_SIZE, blk_rq_bytes(rq), rq->bio); out_end_request: - ceph_put_snap_context(snapc); + if (snapc) + ceph_put_snap_context(snapc); spin_lock_irq(q->queue_lock); if (result < 0) __blk_end_request_all(rq, result); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH REPOST 2/2] rbd: only get snap context for write requests 2013-01-03 22:54 ` [PATCH REPOST 2/2] rbd: only get snap context for write requests Alex Elder @ 2013-01-16 1:23 ` Josh Durgin 0 siblings, 0 replies; 5+ messages in thread From: Josh Durgin @ 2013-01-16 1:23 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel@vger.kernel.org On 01/03/2013 02:54 PM, Alex Elder wrote: > Right now we get the snapshot context for an rbd image (under > protection of the header semaphore) for every request processed. > > There's no need to get the snap context if we're doing a read, > so avoid doing so in that case. > > Note that we no longer need to hold the header semaphore to > check the rbd_dev's existence flag. > > Signed-off-by: Alex Elder <elder@inktank.com> > --- Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > drivers/block/rbd.c | 36 ++++++++++++++++++++---------------- > 1 file changed, 20 insertions(+), 16 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index a3b0d43..fd6a708 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -1331,7 +1331,7 @@ static int rbd_do_op(struct request *rq, > } else { > opcode = CEPH_OSD_OP_READ; > flags = CEPH_OSD_FLAG_READ; > - snapc = NULL; > + rbd_assert(!snapc); > snapid = rbd_dev->spec->snap_id; > payload_len = 0; > } > @@ -1661,22 +1661,25 @@ static void rbd_rq_fn(struct request_queue *q) > } > spin_unlock_irq(q->queue_lock); > > - /* Stop writes to a read-only device */ > - > - result = -EROFS; > - if (read_only && rq_data_dir(rq) == WRITE) > - goto out_end_request; > - > - /* Grab a reference to the snapshot context */ > - > - down_read(&rbd_dev->header_rwsem); > - if (atomic_read(&rbd_dev->exists)) { > + /* Write requests need a reference to the snapshot context */ > + > + if (rq_data_dir(rq) == WRITE) { > + result = -EROFS; > + if (read_only) /* Can't write to a read-only device */ > + goto out_end_request; > + > + /* > + * Note that each osd request will take its > + * own reference to the snapshot context > + * supplied. The reference we take here > + * just guarantees the one we provide stays > + * valid. > + */ > + down_read(&rbd_dev->header_rwsem); > snapc = ceph_get_snap_context(rbd_dev->header.snapc); > + up_read(&rbd_dev->header_rwsem); > rbd_assert(snapc != NULL); > - } > - up_read(&rbd_dev->header_rwsem); > - > - if (!snapc) { > + } else if (!atomic_read(&rbd_dev->exists)) { > rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP); > dout("request for non-existent snapshot"); > result = -ENXIO; > @@ -1687,7 +1690,8 @@ static void rbd_rq_fn(struct request_queue *q) > blk_rq_pos(rq) * SECTOR_SIZE, > blk_rq_bytes(rq), rq->bio); > out_end_request: > - ceph_put_snap_context(snapc); > + if (snapc) > + ceph_put_snap_context(snapc); > spin_lock_irq(q->queue_lock); > if (result < 0) > __blk_end_request_all(rq, result); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-16 1:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-03 22:53 [PATCH REPOST 0/2] rbd: only get snap context for write requests Alex Elder 2013-01-03 22:53 ` [PATCH REPOST 1/2] rbd: make exists flag atomic Alex Elder 2013-01-16 0:29 ` Josh Durgin 2013-01-03 22:54 ` [PATCH REPOST 2/2] rbd: only get snap context for write requests Alex Elder 2013-01-16 1:23 ` Josh Durgin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.