libceph: osd_client should not take bio reference
Subsystems (3)
cover libceph: osd_client should not take bio reference
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
PATCH 1/2 libceph: add a compatibility check interface
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
--- include/linux/ceph/libceph.h | 2 ++ net/ceph/ceph_common.c | 16 ++++++++++++++++ 2 files changed, 18 insertions(+)
include/linux/ceph/libceph.h +2 -0
@@ -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;
net/ceph/ceph_common.c +16 -0
@@ -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)
Signature · 1 line
1.7.9.5
Re: [PATCH 1/2] libceph: add a compatibility check interface
Quoted 64 lines · Alex Elder
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/3800Signed-off-by Alex Elder <[email protected]>--- include/linux/ceph/libceph.h | 2 ++ net/ceph/ceph_common.c | 16 ++++++++++++++++ 2 files changed, 18 insertions(+)include/linux/ceph/libceph.h +2
-0@@ -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;net/ceph/ceph_common.c +16
-0@@ -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)
PATCH 2/2 rbd: don't take extra bio reference for osd client
In reply to Alex ElderCurrently, 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
--- drivers/block/rbd.c | 3 ++- net/ceph/ceph_common.c | 2 +- net/ceph/osd_client.c | 4 ---- 3 files changed, 3 insertions(+), 6 deletions(-)
drivers/block/rbd.c +2 -1
@@ -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;
net/ceph/ceph_common.c +1 -1
@@ -39,7 +39,7 @@*/ bool libceph_compatible (void *data) { -return false; +return true; }EXPORT_SYMBOL (libceph_compatible);
net/ceph/osd_client.c +0 -4
@@ -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)
Signature · 1 line
1.7.9.5
Re: [PATCH 2/2] rbd: don't take extra bio reference for osd client
It'd be nice to have a log message when libceph_compatible fails. It could be a future patch though.
Quoted 86 lines · Alex Elder
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 inceph_osdc_release_request(), as well as its matchingbio_get()call inrbd_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/3799Signed-off-by Alex Elder <[email protected]>--- drivers/block/rbd.c | 3 ++- net/ceph/ceph_common.c | 2 +- net/ceph/osd_client.c | 4 ---- 3 files changed, 3 insertions(+), 6 deletions(-)drivers/block/rbd.c +2
-1@@ -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;net/ceph/ceph_common.c +1
-1@@ -39,7 +39,7 @@*/ bool libceph_compatible (void *data) { -return false; +return true; }EXPORT_SYMBOL (libceph_compatible);net/ceph/osd_client.c +0
-4@@ -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)
Re: [PATCH 2/2] rbd: don't take extra bio reference for osd client
Quoted 4 lines · Josh Durgin
It'd be nice to have a log message when
libceph_compatiblefails. It could be a future patch though.Reviewed-by Josh Durgin <[email protected]>
You're right, and I had thought of doing that but forgot. I will add it before I commit. Thanks a lot.
-Alex
Quoted 88 lines · Alex Elder
Quoted 86 lines
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 inceph_osdc_release_request(), as well as its matchingbio_get()call inrbd_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/3799Signed-off-by Alex Elder <[email protected]>--- drivers/block/rbd.c | 3 ++- net/ceph/ceph_common.c | 2 +- net/ceph/osd_client.c | 4 ---- 3 files changed, 3 insertions(+), 6 deletions(-)drivers/block/rbd.c +2
-1@@ -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;net/ceph/ceph_common.c +1
-1@@ -39,7 +39,7 @@*/ bool libceph_compatible (void *data) { -return false; +return true; }EXPORT_SYMBOL (libceph_compatible);net/ceph/osd_client.c +0
-4@@ -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)