Re: [PATCH REPOST 6/6] rbd: move remaining osd op setup into rbd_osd_req_op_create()
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 . . . >>> +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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH REPOST 6/6] rbd: move remaining osd op setup into rbd_osd_req_op_create()
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
>> ---
>> 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
Re: [PATCH REPOST 6/6] rbd: move remaining osd op setup into rbd_osd_req_op_create()
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
---
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 majord
[PATCH REPOST 6/6] rbd: move remaining osd op setup into rbd_osd_req_op_create()
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
---
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 */
+ 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;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-
