| Message ID | 50762CCB.5040007@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
This cleanup makes sense. We should probably check the return code now as well, as I note below. Reviewed-by: Josh Durgin <josh.durgin@inktank.com> On 10/10/2012 07:19 PM, Alex Elder wrote: > The two calls to rbd_do_op() from rbd_rq_fn() differ only in the > value passed for the snapshot id and the snapshot context. > > For reads the snapshot always comes from the mapping, and for writes > the snapshot id is always CEPH_NOSNAP. > > The snapshot context is always null for reads. For writes, the > snapshot context always comes from the rbd header, but it is > acquired under protection of header semaphore and could change > thereafter, so we can't simply use what's available inside > rbd_do_op(). > > Eliminate the snapid parameter from rbd_do_op(), and set it > based on the I/O direction inside that function instead. Always > pass the snapshot context acquired in the caller, but reset it > to a null pointer inside rbd_do_op() if the operation is a read. > > As a result, there is no difference in the read and write calls > to rbd_do_op() made in rbd_rq_fn(), so just call it unconditionally. > > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 396af14..ca28036 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -1163,7 +1163,6 @@ done: > static int rbd_do_op(struct request *rq, > struct rbd_device *rbd_dev, > struct ceph_snap_context *snapc, > - u64 snapid, > u64 ofs, u64 len, > struct bio *bio, > struct rbd_req_coll *coll, > @@ -1177,6 +1176,7 @@ static int rbd_do_op(struct request *rq, > u32 payload_len; > int opcode; > int flags; > + u64 snapid; > > seg_name = rbd_segment_name(rbd_dev, ofs); > if (!seg_name) > @@ -1187,10 +1187,13 @@ static int rbd_do_op(struct request *rq, > if (rq_data_dir(rq) == WRITE) { > opcode = CEPH_OSD_OP_WRITE; > flags = CEPH_OSD_FLAG_WRITE|CEPH_OSD_FLAG_ONDISK; > + snapid = CEPH_NOSNAP; > payload_len = seg_len; > } else { > opcode = CEPH_OSD_OP_READ; > flags = CEPH_OSD_FLAG_READ; > + snapc = NULL; > + snapid = rbd_dev->mapping.snap_id; > payload_len = 0; > } > > @@ -1518,24 +1521,13 @@ static void rbd_rq_fn(struct request_queue *q) > kref_get(&coll->kref); > bio = bio_chain_clone(&rq_bio, &next_bio, &bp, > op_size, GFP_ATOMIC); > - if (!bio) { > + if (bio) > + (void) rbd_do_op(rq, rbd_dev, snapc, > + ofs, op_size, > + bio, coll, cur_seg); We could check the error code here pretty easily now. > + else > rbd_coll_end_req_index(rq, coll, cur_seg, > -ENOMEM, op_size); > - goto next_seg; > - } > - > - /* init OSD command: write or read */ > - if (do_write) > - (void) rbd_do_op(rq, rbd_dev, > - snapc, CEPH_NOSNAP, > - ofs, op_size, bio, > - coll, cur_seg); > - else > - (void) rbd_do_op(rq, rbd_dev, > - NULL, rbd_dev->mapping.snap_id, > - ofs, op_size, bio, > - coll, cur_seg); > -next_seg: > size -= op_size; > ofs += op_size; > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/24/2012 12:31 PM, Josh Durgin wrote: > This cleanup makes sense. We should probably check > the return code now as well, as I note below. > > Reviewed-by: Josh Durgin <josh.durgin@inktank.com> I'm going to do this as a followup patch, next week. -Alex > > On 10/10/2012 07:19 PM, Alex Elder wrote: >> The two calls to rbd_do_op() from rbd_rq_fn() differ only in the >> value passed for the snapshot id and the snapshot context. >> >> For reads the snapshot always comes from the mapping, and for writes >> the snapshot id is always CEPH_NOSNAP. >> >> The snapshot context is always null for reads. For writes, the >> snapshot context always comes from the rbd header, but it is >> acquired under protection of header semaphore and could change >> thereafter, so we can't simply use what's available inside >> rbd_do_op(). >> >> Eliminate the snapid parameter from rbd_do_op(), and set it >> based on the I/O direction inside that function instead. Always >> pass the snapshot context acquired in the caller, but reset it >> to a null pointer inside rbd_do_op() if the operation is a read. >> >> As a result, there is no difference in the read and write calls >> to rbd_do_op() made in rbd_rq_fn(), so just call it unconditionally. >> >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- >> drivers/block/rbd.c | 26 +++++++++----------------- >> 1 file changed, 9 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index 396af14..ca28036 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -1163,7 +1163,6 @@ done: >> static int rbd_do_op(struct request *rq, >> struct rbd_device *rbd_dev, >> struct ceph_snap_context *snapc, >> - u64 snapid, >> u64 ofs, u64 len, >> struct bio *bio, >> struct rbd_req_coll *coll, >> @@ -1177,6 +1176,7 @@ static int rbd_do_op(struct request *rq, >> u32 payload_len; >> int opcode; >> int flags; >> + u64 snapid; >> >> seg_name = rbd_segment_name(rbd_dev, ofs); >> if (!seg_name) >> @@ -1187,10 +1187,13 @@ static int rbd_do_op(struct request *rq, >> if (rq_data_dir(rq) == WRITE) { >> opcode = CEPH_OSD_OP_WRITE; >> flags = CEPH_OSD_FLAG_WRITE|CEPH_OSD_FLAG_ONDISK; >> + snapid = CEPH_NOSNAP; >> payload_len = seg_len; >> } else { >> opcode = CEPH_OSD_OP_READ; >> flags = CEPH_OSD_FLAG_READ; >> + snapc = NULL; >> + snapid = rbd_dev->mapping.snap_id; >> payload_len = 0; >> } >> >> @@ -1518,24 +1521,13 @@ static void rbd_rq_fn(struct request_queue *q) >> kref_get(&coll->kref); >> bio = bio_chain_clone(&rq_bio, &next_bio, &bp, >> op_size, GFP_ATOMIC); >> - if (!bio) { >> + if (bio) >> + (void) rbd_do_op(rq, rbd_dev, snapc, >> + ofs, op_size, >> + bio, coll, cur_seg); > > We could check the error code here pretty easily now. > >> + else >> rbd_coll_end_req_index(rq, coll, cur_seg, >> -ENOMEM, op_size); >> - goto next_seg; >> - } >> - >> - /* init OSD command: write or read */ >> - if (do_write) >> - (void) rbd_do_op(rq, rbd_dev, >> - snapc, CEPH_NOSNAP, >> - ofs, op_size, bio, >> - coll, cur_seg); >> - else >> - (void) rbd_do_op(rq, rbd_dev, >> - NULL, rbd_dev->mapping.snap_id, >> - ofs, op_size, bio, >> - coll, cur_seg); >> -next_seg: >> size -= op_size; >> ofs += op_size; >> > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 396af14..ca28036 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1163,7 +1163,6 @@ done: static int rbd_do_op(struct request *rq, struct rbd_device *rbd_dev, struct ceph_snap_context *snapc, - u64 snapid, u64 ofs, u64 len, struct bio *bio, struct rbd_req_coll *coll, @@ -1177,6 +1176,7 @@ static int rbd_do_op(struct request *rq, u32 payload_len; int opcode; int flags; + u64 snapid; seg_name = rbd_segment_name(rbd_dev, ofs); if (!seg_name) @@ -1187,10 +1187,13 @@ static int rbd_do_op(struct request *rq, if (rq_data_dir(rq) == WRITE) { opcode = CEPH_OSD_OP_WRITE; flags = CEPH_OSD_FLAG_WRITE|CEPH_OSD_FLAG_ONDISK; + snapid = CEPH_NOSNAP; payload_len = seg_len; } else { opcode = CEPH_OSD_OP_READ; flags = CEPH_OSD_FLAG_READ; + snapc = NULL; + snapid = rbd_dev->mapping.snap_id; payload_len = 0; } @@ -1518,24 +1521,13 @@ static void rbd_rq_fn(struct request_queue *q) kref_get(&coll->kref); bio = bio_chain_clone(&rq_bio, &next_bio, &bp, op_size, GFP_ATOMIC); - if (!bio) { + if (bio) + (void) rbd_do_op(rq, rbd_dev, snapc, + ofs, op_size, + bio, coll, cur_seg); + else rbd_coll_end_req_index(rq, coll, cur_seg, -ENOMEM, op_size); - goto next_seg; - } - - /* init OSD command: write or read */ - if (do_write) - (void) rbd_do_op(rq, rbd_dev, - snapc, CEPH_NOSNAP, - ofs, op_size, bio, - coll, cur_seg); - else - (void) rbd_do_op(rq, rbd_dev, - NULL, rbd_dev->mapping.snap_id, - ofs, op_size, bio, - coll, cur_seg); -next_seg: size -= op_size; ofs += op_size;
The two calls to rbd_do_op() from rbd_rq_fn() differ only in the value passed for the snapshot id and the snapshot context. For reads the snapshot always comes from the mapping, and for writes the snapshot id is always CEPH_NOSNAP. The snapshot context is always null for reads. For writes, the snapshot context always comes from the rbd header, but it is acquired under protection of header semaphore and could change thereafter, so we can't simply use what's available inside rbd_do_op(). Eliminate the snapid parameter from rbd_do_op(), and set it based on the I/O direction inside that function instead. Always pass the snapshot context acquired in the caller, but reset it to a null pointer inside rbd_do_op() if the operation is a read. As a result, there is no difference in the read and write calls to rbd_do_op() made in rbd_rq_fn(), so just call it unconditionally. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)