* [PATCH 0/2] libceph: osd_client should not take bio reference
@ 2013-01-30 20:47 Alex Elder
2013-01-30 20:49 ` [PATCH 1/2] libceph: add a compatibility check interface Alex Elder
2013-01-30 20:49 ` [PATCH 2/2] rbd: don't take extra bio reference for osd client Alex Elder
0 siblings, 2 replies; 6+ messages in thread
From: Alex Elder @ 2013-01-30 20:47 UTC (permalink / raw)
To: ceph-devel@vger.kernel.org
Currently when the osd client gets a request that involves a
bio list, it assumes the initiator (the rbd client) has taken
an extra reference to the first bio on that list. When the
request is done, the initiator calls ceph_osdc_release_request()
and at that time the osd client drops that presumed reference.
The second patch in this series simply gets rid of that extra
reference entirely, because it's not needed. Doing this creates
an incompatibility between differing versions of both rbd and
the osd client (libceph). So The first patch adds a simple
compatibility check capability so it's possible to avoid
that causing any problems.
-Alex
[PATCH 1/2] libceph: add a compatibility check interface
[PATCH 2/2] rbd: don't take extra bio reference for osd client
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] libceph: add a compatibility check interface
2013-01-30 20:47 [PATCH 0/2] libceph: osd_client should not take bio reference Alex Elder
@ 2013-01-30 20:49 ` Alex Elder
2013-01-31 0:29 ` Josh Durgin
2013-01-30 20:49 ` [PATCH 2/2] rbd: don't take extra bio reference for osd client Alex Elder
1 sibling, 1 reply; 6+ messages in thread
From: Alex Elder @ 2013-01-30 20:49 UTC (permalink / raw)
To: ceph-devel@vger.kernel.org
An upcoming change implements semantic change that could lead to
a crash if an old version of the libceph kernel module is used with
a new version of the rbd kernel module.
In order to preclude that possibility, this adds a compatibilty
check interface. If this interface doesn't exist, the modules are
obviously not compatible. But if it does exist, this provides a way
of letting the caller know whether it will operate properly with
this libceph module.
Perhaps confusingly, it returns false right now. The semantic
change mentioned above will make it return true.
This resolves:
http://tracker.ceph.com/issues/3800
Signed-off-by: Alex Elder <elder@inktank.com>
---
include/linux/ceph/libceph.h | 2 ++
net/ceph/ceph_common.c | 16 ++++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 084d3c6..c44275a 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -193,6 +193,8 @@ static inline int calc_pages_for(u64 off, u64 len)
}
/* ceph_common.c */
+extern bool libceph_compatible(void *data);
+
extern const char *ceph_msg_type_name(int type);
extern int ceph_check_fsid(struct ceph_client *client, struct ceph_fsid
*fsid);
extern struct kmem_cache *ceph_inode_cachep;
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index ee71ea2..a98c03f 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -26,6 +26,22 @@
#include "crypto.h"
+/*
+ * Module compatibility interface. For now it doesn't do anything,
+ * but its existence signals a certain level of functionality.
+ *
+ * The data buffer is used to pass information both to and from
+ * libceph. The return value indicates whether libceph determines
+ * it is compatible with the caller (from another kernel module),
+ * given the provided data.
+ *
+ * The data pointer can be null.
+ */
+bool libceph_compatible(void *data)
+{
+ return false;
+}
+EXPORT_SYMBOL(libceph_compatible);
/*
* find filename portion of a path (/foo/bar/baz -> baz)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] rbd: don't take extra bio reference for osd client
2013-01-30 20:47 [PATCH 0/2] libceph: osd_client should not take bio reference Alex Elder
2013-01-30 20:49 ` [PATCH 1/2] libceph: add a compatibility check interface Alex Elder
@ 2013-01-30 20:49 ` Alex Elder
2013-01-31 0:33 ` Josh Durgin
1 sibling, 1 reply; 6+ messages in thread
From: Alex Elder @ 2013-01-30 20:49 UTC (permalink / raw)
To: ceph-devel@vger.kernel.org
Currently, if the OSD client finds an osd request has had a bio list
attached to it, it drops a reference to it (or rather, to the first
entry on that list) when the request is released.
The code that added that reference (i.e., the rbd client) is
therefore required to take an extra reference to that first bio
structure.
The osd client doesn't really do anything with the bio pointer other
than transfer it from the osd request structure to outgoing (for
writes) and ingoing (for reads) messages. So it really isn't the
right place to be taking or dropping references.
Furthermore, the rbd client already holds references to all bio
structures it passes to the osd client, and holds them until the
request is completed. So there's no need for this extra reference
whatsoever.
So remove the bio_put() call in ceph_osdc_release_request(), as
well as its matching bio_get() call in rbd_osd_req_create().
This change could lead to a crash if old libceph.ko was used with
new rbd.ko. Add a compatibility check at rbd initialization time to
avoid this possibilty.
This resolves:
http://tracker.ceph.com/issues/3798 and
http://tracker.ceph.com/issues/3799
Signed-off-by: Alex Elder <elder@inktank.com>
---
drivers/block/rbd.c | 3 ++-
net/ceph/ceph_common.c | 2 +-
net/ceph/osd_client.c | 4 ----
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a9ce58c..6586800 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1342,7 +1342,6 @@ static struct ceph_osd_request *rbd_osd_req_create(
case OBJ_REQUEST_BIO:
rbd_assert(obj_request->bio_list != NULL);
osd_req->r_bio = obj_request->bio_list;
- bio_get(osd_req->r_bio);
/* osd client requires "num pages" even for bio */
osd_req->r_num_pages = calc_pages_for(offset, length);
break;
@@ -4150,6 +4149,8 @@ int __init rbd_init(void)
{
int rc;
+ if (!libceph_compatible(NULL))
+ return -EINVAL;
rc = rbd_sysfs_init();
if (rc)
return rc;
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index a98c03f..c236c235 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -39,7 +39,7 @@
*/
bool libceph_compatible(void *data)
{
- return false;
+ return true;
}
EXPORT_SYMBOL(libceph_compatible);
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 500ae8b..ba03648 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -147,10 +147,6 @@ void ceph_osdc_release_request(struct kref *kref)
if (req->r_own_pages)
ceph_release_page_vector(req->r_pages,
req->r_num_pages);
-#ifdef CONFIG_BLOCK
- if (req->r_bio)
- bio_put(req->r_bio);
-#endif
ceph_put_snap_context(req->r_snapc);
ceph_pagelist_release(&req->r_trail);
if (req->r_mempool)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libceph: add a compatibility check interface
2013-01-30 20:49 ` [PATCH 1/2] libceph: add a compatibility check interface Alex Elder
@ 2013-01-31 0:29 ` Josh Durgin
0 siblings, 0 replies; 6+ messages in thread
From: Josh Durgin @ 2013-01-31 0:29 UTC (permalink / raw)
To: Alex Elder; +Cc: ceph-devel@vger.kernel.org
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
On 01/30/2013 12:49 PM, Alex Elder wrote:
> An upcoming change implements semantic change that could lead to
> a crash if an old version of the libceph kernel module is used with
> a new version of the rbd kernel module.
>
> In order to preclude that possibility, this adds a compatibilty
> check interface. If this interface doesn't exist, the modules are
> obviously not compatible. But if it does exist, this provides a way
> of letting the caller know whether it will operate properly with
> this libceph module.
>
> Perhaps confusingly, it returns false right now. The semantic
> change mentioned above will make it return true.
>
> This resolves:
> http://tracker.ceph.com/issues/3800
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
> include/linux/ceph/libceph.h | 2 ++
> net/ceph/ceph_common.c | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index 084d3c6..c44275a 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -193,6 +193,8 @@ static inline int calc_pages_for(u64 off, u64 len)
> }
>
> /* ceph_common.c */
> +extern bool libceph_compatible(void *data);
> +
> extern const char *ceph_msg_type_name(int type);
> extern int ceph_check_fsid(struct ceph_client *client, struct ceph_fsid
> *fsid);
> extern struct kmem_cache *ceph_inode_cachep;
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index ee71ea2..a98c03f 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -26,6 +26,22 @@
> #include "crypto.h"
>
>
> +/*
> + * Module compatibility interface. For now it doesn't do anything,
> + * but its existence signals a certain level of functionality.
> + *
> + * The data buffer is used to pass information both to and from
> + * libceph. The return value indicates whether libceph determines
> + * it is compatible with the caller (from another kernel module),
> + * given the provided data.
> + *
> + * The data pointer can be null.
> + */
> +bool libceph_compatible(void *data)
> +{
> + return false;
> +}
> +EXPORT_SYMBOL(libceph_compatible);
>
> /*
> * find filename portion of a path (/foo/bar/baz -> baz)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] rbd: don't take extra bio reference for osd client
2013-01-30 20:49 ` [PATCH 2/2] rbd: don't take extra bio reference for osd client Alex Elder
@ 2013-01-31 0:33 ` Josh Durgin
2013-01-31 12:33 ` Alex Elder
0 siblings, 1 reply; 6+ messages in thread
From: Josh Durgin @ 2013-01-31 0:33 UTC (permalink / raw)
To: Alex Elder; +Cc: ceph-devel@vger.kernel.org
It'd be nice to have a log message when libceph_compatible fails.
It could be a future patch though.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
On 01/30/2013 12:49 PM, Alex Elder wrote:
> Currently, if the OSD client finds an osd request has had a bio list
> attached to it, it drops a reference to it (or rather, to the first
> entry on that list) when the request is released.
>
> The code that added that reference (i.e., the rbd client) is
> therefore required to take an extra reference to that first bio
> structure.
>
> The osd client doesn't really do anything with the bio pointer other
> than transfer it from the osd request structure to outgoing (for
> writes) and ingoing (for reads) messages. So it really isn't the
> right place to be taking or dropping references.
>
> Furthermore, the rbd client already holds references to all bio
> structures it passes to the osd client, and holds them until the
> request is completed. So there's no need for this extra reference
> whatsoever.
>
> So remove the bio_put() call in ceph_osdc_release_request(), as
> well as its matching bio_get() call in rbd_osd_req_create().
>
> This change could lead to a crash if old libceph.ko was used with
> new rbd.ko. Add a compatibility check at rbd initialization time to
> avoid this possibilty.
>
> This resolves:
> http://tracker.ceph.com/issues/3798 and
> http://tracker.ceph.com/issues/3799
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
> drivers/block/rbd.c | 3 ++-
> net/ceph/ceph_common.c | 2 +-
> net/ceph/osd_client.c | 4 ----
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a9ce58c..6586800 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1342,7 +1342,6 @@ static struct ceph_osd_request *rbd_osd_req_create(
> case OBJ_REQUEST_BIO:
> rbd_assert(obj_request->bio_list != NULL);
> osd_req->r_bio = obj_request->bio_list;
> - bio_get(osd_req->r_bio);
> /* osd client requires "num pages" even for bio */
> osd_req->r_num_pages = calc_pages_for(offset, length);
> break;
> @@ -4150,6 +4149,8 @@ int __init rbd_init(void)
> {
> int rc;
>
> + if (!libceph_compatible(NULL))
> + return -EINVAL;
> rc = rbd_sysfs_init();
> if (rc)
> return rc;
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index a98c03f..c236c235 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -39,7 +39,7 @@
> */
> bool libceph_compatible(void *data)
> {
> - return false;
> + return true;
> }
> EXPORT_SYMBOL(libceph_compatible);
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 500ae8b..ba03648 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -147,10 +147,6 @@ void ceph_osdc_release_request(struct kref *kref)
> if (req->r_own_pages)
> ceph_release_page_vector(req->r_pages,
> req->r_num_pages);
> -#ifdef CONFIG_BLOCK
> - if (req->r_bio)
> - bio_put(req->r_bio);
> -#endif
> ceph_put_snap_context(req->r_snapc);
> ceph_pagelist_release(&req->r_trail);
> if (req->r_mempool)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] rbd: don't take extra bio reference for osd client
2013-01-31 0:33 ` Josh Durgin
@ 2013-01-31 12:33 ` Alex Elder
0 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2013-01-31 12:33 UTC (permalink / raw)
To: Josh Durgin; +Cc: ceph-devel@vger.kernel.org
On 01/30/2013 06:33 PM, Josh Durgin wrote:
> It'd be nice to have a log message when libceph_compatible fails.
> It could be a future patch though.
>
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
You're right, and I had thought of doing that but forgot.
I will add it before I commit. Thanks a lot.
-Alex
> On 01/30/2013 12:49 PM, Alex Elder wrote:
>> Currently, if the OSD client finds an osd request has had a bio list
>> attached to it, it drops a reference to it (or rather, to the first
>> entry on that list) when the request is released.
>>
>> The code that added that reference (i.e., the rbd client) is
>> therefore required to take an extra reference to that first bio
>> structure.
>>
>> The osd client doesn't really do anything with the bio pointer other
>> than transfer it from the osd request structure to outgoing (for
>> writes) and ingoing (for reads) messages. So it really isn't the
>> right place to be taking or dropping references.
>>
>> Furthermore, the rbd client already holds references to all bio
>> structures it passes to the osd client, and holds them until the
>> request is completed. So there's no need for this extra reference
>> whatsoever.
>>
>> So remove the bio_put() call in ceph_osdc_release_request(), as
>> well as its matching bio_get() call in rbd_osd_req_create().
>>
>> This change could lead to a crash if old libceph.ko was used with
>> new rbd.ko. Add a compatibility check at rbd initialization time to
>> avoid this possibilty.
>>
>> This resolves:
>> http://tracker.ceph.com/issues/3798 and
>> http://tracker.ceph.com/issues/3799
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>> drivers/block/rbd.c | 3 ++-
>> net/ceph/ceph_common.c | 2 +-
>> net/ceph/osd_client.c | 4 ----
>> 3 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index a9ce58c..6586800 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -1342,7 +1342,6 @@ static struct ceph_osd_request *rbd_osd_req_create(
>> case OBJ_REQUEST_BIO:
>> rbd_assert(obj_request->bio_list != NULL);
>> osd_req->r_bio = obj_request->bio_list;
>> - bio_get(osd_req->r_bio);
>> /* osd client requires "num pages" even for bio */
>> osd_req->r_num_pages = calc_pages_for(offset, length);
>> break;
>> @@ -4150,6 +4149,8 @@ int __init rbd_init(void)
>> {
>> int rc;
>>
>> + if (!libceph_compatible(NULL))
>> + return -EINVAL;
>> rc = rbd_sysfs_init();
>> if (rc)
>> return rc;
>> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
>> index a98c03f..c236c235 100644
>> --- a/net/ceph/ceph_common.c
>> +++ b/net/ceph/ceph_common.c
>> @@ -39,7 +39,7 @@
>> */
>> bool libceph_compatible(void *data)
>> {
>> - return false;
>> + return true;
>> }
>> EXPORT_SYMBOL(libceph_compatible);
>>
>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>> index 500ae8b..ba03648 100644
>> --- a/net/ceph/osd_client.c
>> +++ b/net/ceph/osd_client.c
>> @@ -147,10 +147,6 @@ void ceph_osdc_release_request(struct kref *kref)
>> if (req->r_own_pages)
>> ceph_release_page_vector(req->r_pages,
>> req->r_num_pages);
>> -#ifdef CONFIG_BLOCK
>> - if (req->r_bio)
>> - bio_put(req->r_bio);
>> -#endif
>> ceph_put_snap_context(req->r_snapc);
>> ceph_pagelist_release(&req->r_trail);
>> if (req->r_mempool)
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-31 12:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-30 20:47 [PATCH 0/2] libceph: osd_client should not take bio reference Alex Elder
2013-01-30 20:49 ` [PATCH 1/2] libceph: add a compatibility check interface Alex Elder
2013-01-31 0:29 ` Josh Durgin
2013-01-30 20:49 ` [PATCH 2/2] rbd: don't take extra bio reference for osd client Alex Elder
2013-01-31 0:33 ` Josh Durgin
2013-01-31 12:33 ` Alex Elder
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.