| Message ID | 50E6F028.6040904@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/04/2013 07:07 AM, Alex Elder wrote: > The two remaining osd ops used by rbd are CEPH_OSD_OP_WATCH and > CEPH_OSD_OP_NOTIFY_ACK. Move the setup of those operations into > rbd_osd_req_op_create(), and get rid of rbd_create_rw_op() and > rbd_destroy_op(). > > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 68 > ++++++++++++++++++++------------------------------- > 1 file changed, 27 insertions(+), 41 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 9f41c32..21fbf82 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -1027,24 +1027,6 @@ out_err: > return NULL; > } > > -static struct ceph_osd_req_op *rbd_create_rw_op(int opcode, u64 ofs, > u64 len) > -{ > - struct ceph_osd_req_op *op; > - > - op = kzalloc(sizeof (*op), GFP_NOIO); > - if (!op) > - return NULL; > - > - op->op = opcode; > - > - return op; > -} > - > -static void rbd_destroy_op(struct ceph_osd_req_op *op) > -{ > - kfree(op); > -} > - > struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...) > { > struct ceph_osd_req_op *op; > @@ -1087,6 +1069,16 @@ struct ceph_osd_req_op *rbd_osd_req_op_create(u16 > opcode, ...) > op->cls.indata_len = (u32) size; > op->payload_len += size; > break; > + case CEPH_OSD_OP_NOTIFY_ACK: > + case CEPH_OSD_OP_WATCH: > + /* rbd_osd_req_op_create(NOTIFY_ACK, cookie, version) */ > + /* rbd_osd_req_op_create(WATCH, cookie, version, flag) */ > + op->watch.cookie = va_arg(args, u64); > + op->watch.ver = va_arg(args, u64); > + op->watch.ver = cpu_to_le64(op->watch.ver); /* XXX */ why the /* XXX */ comment? > + if (opcode == CEPH_OSD_OP_WATCH && va_arg(args, int)) > + op->watch.flag = (u8) 1; > + break; > default: > rbd_warn(NULL, "unsupported opcode %hu\n", opcode); > kfree(op); > @@ -1434,14 +1426,10 @@ static int rbd_req_sync_notify_ack(struct > rbd_device *rbd_dev, > struct ceph_osd_req_op *op; > int ret; > > - op = rbd_create_rw_op(CEPH_OSD_OP_NOTIFY_ACK, 0, 0); > + op = rbd_osd_req_op_create(CEPH_OSD_OP_NOTIFY_ACK, notify_id, ver); > if (!op) > return -ENOMEM; > > - op->watch.ver = cpu_to_le64(ver); > - op->watch.cookie = notify_id; > - op->watch.flag = 0; > - > ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP, > rbd_dev->header_name, 0, 0, NULL, > NULL, 0, > @@ -1450,7 +1438,8 @@ static int rbd_req_sync_notify_ack(struct > rbd_device *rbd_dev, > NULL, 0, > rbd_simple_req_cb, 0, NULL); > > - rbd_destroy_op(op); > + rbd_osd_req_op_destroy(op); > + > return ret; > } > > @@ -1480,14 +1469,9 @@ static void rbd_watch_cb(u64 ver, u64 notify_id, > u8 opcode, void *data) > */ > static int rbd_req_sync_watch(struct rbd_device *rbd_dev, int start) > { > - struct ceph_osd_req_op *op; > struct ceph_osd_request **linger_req = NULL; > - __le64 version = 0; > - int ret; > - > - op = rbd_create_rw_op(CEPH_OSD_OP_WATCH, 0, 0); > - if (!op) > - return -ENOMEM; > + struct ceph_osd_req_op *op; > + int ret = 0; > > if (start) { > struct ceph_osd_client *osdc; > @@ -1496,26 +1480,28 @@ static int rbd_req_sync_watch(struct rbd_device > *rbd_dev, int start) > ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0, rbd_dev, > &rbd_dev->watch_event); > if (ret < 0) > - goto done; > - version = cpu_to_le64(rbd_dev->header.obj_version); > + return ret; > linger_req = &rbd_dev->watch_request; > + } else { > + rbd_assert(rbd_dev->watch_request != NULL); > } > > - op->watch.ver = version; > - op->watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie); > - op->watch.flag = (u8) start ? 1 : 0; > - > - ret = rbd_req_sync_op(rbd_dev, > + op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH, > + rbd_dev->watch_event->cookie, > + rbd_dev->header.obj_version, start); > + if (op) > + ret = rbd_req_sync_op(rbd_dev, > CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK, > op, rbd_dev->header_name, > 0, 0, NULL, linger_req, NULL); > > - if (!start || ret < 0) { > + /* Cancel the event if we're tearing down, or on error */ > + > + if (!start || !op || ret < 0) { > ceph_osdc_cancel_event(rbd_dev->watch_event); > rbd_dev->watch_event = NULL; > } > -done: > - rbd_destroy_op(op); > + rbd_osd_req_op_destroy(op); > > return ret; > } > -- 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 01/16/2013 10:23 PM, Josh Durgin wrote: > On 01/04/2013 07:07 AM, Alex Elder wrote: >> The two remaining osd ops used by rbd are CEPH_OSD_OP_WATCH and >> CEPH_OSD_OP_NOTIFY_ACK. Move the setup of those operations into >> rbd_osd_req_op_create(), and get rid of rbd_create_rw_op() and >> rbd_destroy_op(). >> >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- >> drivers/block/rbd.c | 68 >> ++++++++++++++++++++------------------------------- >> 1 file changed, 27 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index 9f41c32..21fbf82 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -1027,24 +1027,6 @@ out_err: >> return NULL; >> } >> >> -static struct ceph_osd_req_op *rbd_create_rw_op(int opcode, u64 ofs, >> u64 len) >> -{ >> - struct ceph_osd_req_op *op; >> - >> - op = kzalloc(sizeof (*op), GFP_NOIO); >> - if (!op) >> - return NULL; >> - >> - op->op = opcode; >> - >> - return op; >> -} >> - >> -static void rbd_destroy_op(struct ceph_osd_req_op *op) >> -{ >> - kfree(op); >> -} >> - >> struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...) >> { >> struct ceph_osd_req_op *op; >> @@ -1087,6 +1069,16 @@ struct ceph_osd_req_op *rbd_osd_req_op_create(u16 >> opcode, ...) >> op->cls.indata_len = (u32) size; >> op->payload_len += size; >> break; >> + case CEPH_OSD_OP_NOTIFY_ACK: >> + case CEPH_OSD_OP_WATCH: >> + /* rbd_osd_req_op_create(NOTIFY_ACK, cookie, version) */ >> + /* rbd_osd_req_op_create(WATCH, cookie, version, flag) */ >> + op->watch.cookie = va_arg(args, u64); >> + op->watch.ver = va_arg(args, u64); >> + op->watch.ver = cpu_to_le64(op->watch.ver); /* XXX */ > > why the /* XXX */ comment? Because it's the only value here that is converted from cpu byte order. It was added in this commit: a71b891bc7d77a070e723c8c53d1dd73cf931555 rbd: send header version when notifying And I think was done without full understanding that it was being done different from all the others. I think it may be wrong but I haven't really looked at it yet. Pulling them all into this function made this difference more obvious. It was a "note to self" that I wanted to fix that. I normally try to resolve anything like that before I post for review but I guess I forgot. There may be others. -Alex >> + if (opcode == CEPH_OSD_OP_WATCH && va_arg(args, int)) >> + op->watch.flag = (u8) 1; >> + break; >> default: >> rbd_warn(NULL, "unsupported opcode %hu\n", opcode); >> kfree(op); >> @@ -1434,14 +1426,10 @@ static int rbd_req_sync_notify_ack(struct >> rbd_device *rbd_dev, >> struct ceph_osd_req_op *op; >> int ret; >> >> - op = rbd_create_rw_op(CEPH_OSD_OP_NOTIFY_ACK, 0, 0); >> + op = rbd_osd_req_op_create(CEPH_OSD_OP_NOTIFY_ACK, notify_id, ver); >> if (!op) >> return -ENOMEM; >> >> - op->watch.ver = cpu_to_le64(ver); >> - op->watch.cookie = notify_id; >> - op->watch.flag = 0; >> - >> ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP, >> rbd_dev->header_name, 0, 0, NULL, >> NULL, 0, >> @@ -1450,7 +1438,8 @@ static int rbd_req_sync_notify_ack(struct >> rbd_device *rbd_dev, >> NULL, 0, >> rbd_simple_req_cb, 0, NULL); >> >> - rbd_destroy_op(op); >> + rbd_osd_req_op_destroy(op); >> + >> return ret; >> } >> >> @@ -1480,14 +1469,9 @@ static void rbd_watch_cb(u64 ver, u64 notify_id, >> u8 opcode, void *data) >> */ >> static int rbd_req_sync_watch(struct rbd_device *rbd_dev, int start) >> { >> - struct ceph_osd_req_op *op; >> struct ceph_osd_request **linger_req = NULL; >> - __le64 version = 0; >> - int ret; >> - >> - op = rbd_create_rw_op(CEPH_OSD_OP_WATCH, 0, 0); >> - if (!op) >> - return -ENOMEM; >> + struct ceph_osd_req_op *op; >> + int ret = 0; >> >> if (start) { >> struct ceph_osd_client *osdc; >> @@ -1496,26 +1480,28 @@ static int rbd_req_sync_watch(struct rbd_device >> *rbd_dev, int start) >> ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0, rbd_dev, >> &rbd_dev->watch_event); >> if (ret < 0) >> - goto done; >> - version = cpu_to_le64(rbd_dev->header.obj_version); >> + return ret; >> linger_req = &rbd_dev->watch_request; >> + } else { >> + rbd_assert(rbd_dev->watch_request != NULL); >> } >> >> - op->watch.ver = version; >> - op->watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie); >> - op->watch.flag = (u8) start ? 1 : 0; >> - >> - ret = rbd_req_sync_op(rbd_dev, >> + op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH, >> + rbd_dev->watch_event->cookie, >> + rbd_dev->header.obj_version, start); >> + if (op) >> + ret = rbd_req_sync_op(rbd_dev, >> CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK, >> op, rbd_dev->header_name, >> 0, 0, NULL, linger_req, NULL); >> >> - if (!start || ret < 0) { >> + /* Cancel the event if we're tearing down, or on error */ >> + >> + if (!start || !op || ret < 0) { >> ceph_osdc_cancel_event(rbd_dev->watch_event); >> rbd_dev->watch_event = NULL; >> } >> -done: >> - rbd_destroy_op(op); >> + rbd_osd_req_op_destroy(op); >> >> return ret; >> } >> > -- 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 01/16/2013 10:37 PM, Alex Elder wrote: > On 01/16/2013 10:23 PM, Josh Durgin wrote: >> On 01/04/2013 07:07 AM, Alex Elder wrote: >>> The two remaining osd ops used by rbd are CEPH_OSD_OP_WATCH and >>> CEPH_OSD_OP_NOTIFY_ACK. Move the setup of those operations into >>> rbd_osd_req_op_create(), and get rid of rbd_create_rw_op() and >>> rbd_destroy_op(). >>> >>> Signed-off-by: Alex Elder <elder@inktank.com> . . . >>> + case CEPH_OSD_OP_NOTIFY_ACK: >>> + case CEPH_OSD_OP_WATCH: >>> + /* rbd_osd_req_op_create(NOTIFY_ACK, cookie, version) */ >>> + /* rbd_osd_req_op_create(WATCH, cookie, version, flag) */ >>> + op->watch.cookie = va_arg(args, u64); >>> + op->watch.ver = va_arg(args, u64); >>> + op->watch.ver = cpu_to_le64(op->watch.ver); /* XXX */ >> >> why the /* XXX */ comment? > > Because it's the only value here that is converted > from cpu byte order. It was added in this commit: > > a71b891bc7d77a070e723c8c53d1dd73cf931555 > rbd: send header version when notifying > > And I think was done without full understanding that it > was being done different from all the others. I think > it may be wrong but I haven't really looked at it yet. > Pulling them all into this function made this difference > more obvious. I've created this to track getting this issue resolved: http://tracker.newdream.net/issues/3847 > It was a "note to self" that I wanted to fix that. > > I normally try to resolve anything like that before I > post for review but I guess I forgot. There may be > others. I'm deleting the "XXX" comment before I commit this change. -Alex . . . -- 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 9f41c32..21fbf82 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1027,24 +1027,6 @@ out_err: return NULL; } -static struct ceph_osd_req_op *rbd_create_rw_op(int opcode, u64 ofs, u64 len) -{ - struct ceph_osd_req_op *op; - - op = kzalloc(sizeof (*op), GFP_NOIO); - if (!op) - return NULL; - - op->op = opcode; - - return op; -} - -static void rbd_destroy_op(struct ceph_osd_req_op *op) -{ - kfree(op); -} - struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...) {
The two remaining osd ops used by rbd are CEPH_OSD_OP_WATCH and CEPH_OSD_OP_NOTIFY_ACK. Move the setup of those operations into rbd_osd_req_op_create(), and get rid of rbd_create_rw_op() and rbd_destroy_op(). Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 68 ++++++++++++++++++++------------------------------- 1 file changed, 27 insertions(+), 41 deletions(-) struct ceph_osd_req_op *op; @@ -1087,6 +1069,16 @@ struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...) op->cls.indata_len = (u32) size; op->payload_len += size; break; + case CEPH_OSD_OP_NOTIFY_ACK: + case CEPH_OSD_OP_WATCH: + /* rbd_osd_req_op_create(NOTIFY_ACK, cookie, version) */ + /* rbd_osd_req_op_create(WATCH, cookie, version, flag) */ + op->watch.cookie = va_arg(args, u64); + op->watch.ver = va_arg(args, u64); + op->watch.ver = cpu_to_le64(op->watch.ver); /* XXX */ + if (opcode == CEPH_OSD_OP_WATCH && va_arg(args, int)) + op->watch.flag = (u8) 1; + break; default: rbd_warn(NULL, "unsupported opcode %hu\n", opcode); kfree(op); @@ -1434,14 +1426,10 @@ static int rbd_req_sync_notify_ack(struct rbd_device *rbd_dev, struct ceph_osd_req_op *op; int ret; - op = rbd_create_rw_op(CEPH_OSD_OP_NOTIFY_ACK, 0, 0); + op = rbd_osd_req_op_create(CEPH_OSD_OP_NOTIFY_ACK, notify_id, ver); if (!op) return -ENOMEM; - op->watch.ver = cpu_to_le64(ver); - op->watch.cookie = notify_id; - op->watch.flag = 0; - ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP, rbd_dev->header_name, 0, 0, NULL, NULL, 0, @@ -1450,7 +1438,8 @@ static int rbd_req_sync_notify_ack(struct rbd_device *rbd_dev, NULL, 0, rbd_simple_req_cb, 0, NULL); - rbd_destroy_op(op); + rbd_osd_req_op_destroy(op); + return ret; } @@ -1480,14 +1469,9 @@ static void rbd_watch_cb(u64 ver, u64 notify_id, u8 opcode, void *data) */ static int rbd_req_sync_watch(struct rbd_device *rbd_dev, int start) { - struct ceph_osd_req_op *op; struct ceph_osd_request **linger_req = NULL; - __le64 version = 0; - int ret; - - op = rbd_create_rw_op(CEPH_OSD_OP_WATCH, 0, 0); - if (!op) - return -ENOMEM; + struct ceph_osd_req_op *op; + int ret = 0; if (start) { struct ceph_osd_client *osdc; @@ -1496,26 +1480,28 @@ static int rbd_req_sync_watch(struct rbd_device *rbd_dev, int start) ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0, rbd_dev, &rbd_dev->watch_event); if (ret < 0) - goto done; - version = cpu_to_le64(rbd_dev->header.obj_version); + return ret; linger_req = &rbd_dev->watch_request; + } else { + rbd_assert(rbd_dev->watch_request != NULL); } - op->watch.ver = version; - op->watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie); - op->watch.flag = (u8) start ? 1 : 0; - - ret = rbd_req_sync_op(rbd_dev, + op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH, + rbd_dev->watch_event->cookie, + rbd_dev->header.obj_version, start); + if (op) + ret = rbd_req_sync_op(rbd_dev, CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK, op, rbd_dev->header_name, 0, 0, NULL, linger_req, NULL); - if (!start || ret < 0) { + /* Cancel the event if we're tearing down, or on error */ + + if (!start || !op || ret < 0) { ceph_osdc_cancel_event(rbd_dev->watch_event); rbd_dev->watch_event = NULL; } -done: - rbd_destroy_op(op); + rbd_osd_req_op_destroy(op); return ret; }