| Message ID | 20200701155446.41141-4-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libceph/ceph: cleanups and move more into ceph.ko | expand |
On Wed, Jul 1, 2020 at 5:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > ...and make it public and export it. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > include/linux/ceph/osd_client.h | 3 +++ > net/ceph/osd_client.c | 13 +++++++------ > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h > index 40a08c4e5d8d..71b7610c3a3c 100644 > --- a/include/linux/ceph/osd_client.h > +++ b/include/linux/ceph/osd_client.h > @@ -481,6 +481,9 @@ extern struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client * > unsigned int num_ops, > bool use_mempool, > gfp_t gfp_flags); > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > + int num_request_data_items, > + int num_reply_data_items); > int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp); > > extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *, > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 4ddf23120b1a..7be78fa6e2c3 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -613,9 +613,9 @@ static int ceph_oloc_encoding_size(const struct ceph_object_locator *oloc) > return 8 + 4 + 4 + 4 + (oloc->pool_ns ? oloc->pool_ns->len : 0); > } > > -static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > - int num_request_data_items, > - int num_reply_data_items) > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > + int num_request_data_items, > + int num_reply_data_items) > { > struct ceph_osd_client *osdc = req->r_osdc; > struct ceph_msg *msg; > @@ -672,6 +672,7 @@ static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > return 0; > } > +EXPORT_SYMBOL(ceph_osdc_alloc_num_messages); > > static bool osd_req_opcode_valid(u16 opcode) > { > @@ -738,8 +739,8 @@ int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp) > int num_request_data_items, num_reply_data_items; > > get_num_data_items(req, &num_request_data_items, &num_reply_data_items); > - return __ceph_osdc_alloc_messages(req, gfp, num_request_data_items, > - num_reply_data_items); > + return ceph_osdc_alloc_num_messages(req, gfp, num_request_data_items, > + num_reply_data_items); > } > EXPORT_SYMBOL(ceph_osdc_alloc_messages); > > @@ -1129,7 +1130,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc, > * also covers ceph_uninline_data(). If more multi-op request > * use cases emerge, we will need a separate helper. > */ > - r = __ceph_osdc_alloc_messages(req, GFP_NOFS, num_ops, 0); > + r = ceph_osdc_alloc_num_messages(req, GFP_NOFS, num_ops, 0); > else > r = ceph_osdc_alloc_messages(req, GFP_NOFS); > if (r) I think exporting __ceph_osdc_alloc_messages() is wrong, at least conceptually. Only the OSD client should be concerned with message data items and their count, as they are an implementation detail of the OSD client and the messenger. Exporting something that takes message data items counts without exporting something for counting them suggests that users will somehow do that on their own and we don't want that. Thanks, Ilya
On Wed, 2020-07-01 at 20:48 +0200, Ilya Dryomov wrote: > On Wed, Jul 1, 2020 at 5:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > ...and make it public and export it. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > include/linux/ceph/osd_client.h | 3 +++ > > net/ceph/osd_client.c | 13 +++++++------ > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h > > index 40a08c4e5d8d..71b7610c3a3c 100644 > > --- a/include/linux/ceph/osd_client.h > > +++ b/include/linux/ceph/osd_client.h > > @@ -481,6 +481,9 @@ extern struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client * > > unsigned int num_ops, > > bool use_mempool, > > gfp_t gfp_flags); > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > + int num_request_data_items, > > + int num_reply_data_items); > > int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp); > > > > extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *, > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > index 4ddf23120b1a..7be78fa6e2c3 100644 > > --- a/net/ceph/osd_client.c > > +++ b/net/ceph/osd_client.c > > @@ -613,9 +613,9 @@ static int ceph_oloc_encoding_size(const struct ceph_object_locator *oloc) > > return 8 + 4 + 4 + 4 + (oloc->pool_ns ? oloc->pool_ns->len : 0); > > } > > > > -static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > - int num_request_data_items, > > - int num_reply_data_items) > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > + int num_request_data_items, > > + int num_reply_data_items) > > { > > struct ceph_osd_client *osdc = req->r_osdc; > > struct ceph_msg *msg; > > @@ -672,6 +672,7 @@ static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > return 0; > > } > > +EXPORT_SYMBOL(ceph_osdc_alloc_num_messages); > > > > static bool osd_req_opcode_valid(u16 opcode) > > { > > @@ -738,8 +739,8 @@ int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp) > > int num_request_data_items, num_reply_data_items; > > > > get_num_data_items(req, &num_request_data_items, &num_reply_data_items); > > - return __ceph_osdc_alloc_messages(req, gfp, num_request_data_items, > > - num_reply_data_items); > > + return ceph_osdc_alloc_num_messages(req, gfp, num_request_data_items, > > + num_reply_data_items); > > } > > EXPORT_SYMBOL(ceph_osdc_alloc_messages); > > > > @@ -1129,7 +1130,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc, > > * also covers ceph_uninline_data(). If more multi-op request > > * use cases emerge, we will need a separate helper. > > */ > > - r = __ceph_osdc_alloc_messages(req, GFP_NOFS, num_ops, 0); > > + r = ceph_osdc_alloc_num_messages(req, GFP_NOFS, num_ops, 0); > > else > > r = ceph_osdc_alloc_messages(req, GFP_NOFS); > > if (r) > > I think exporting __ceph_osdc_alloc_messages() is wrong, at least > conceptually. Only the OSD client should be concerned with message > data items and their count, as they are an implementation detail of > the OSD client and the messenger. Exporting something that takes > message data items counts without exporting something for counting > them suggests that users will somehow do that on their own and we > don't want that. > We already do that in ceph_osdc_new_request(). That function takes a num_ops value that describes the number of OSD ops needed, and the callers have to fill it out with the number of ops they expect the call to use (see the calls in ceph_writepages_start for example). I'm not sure what you're suggesting we do instead. The caller currently does need to know the number of ops, and the calculation it does to figure that out is somewhat complex and really only in writepages (IIRC). Are you suggesting we ought to move more of that into libcephfs? If so, from what info should the OSD client be determining the number of op slots to allocate?
On Wed, Jul 1, 2020 at 9:17 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2020-07-01 at 20:48 +0200, Ilya Dryomov wrote: > > On Wed, Jul 1, 2020 at 5:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > > ...and make it public and export it. > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > include/linux/ceph/osd_client.h | 3 +++ > > > net/ceph/osd_client.c | 13 +++++++------ > > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h > > > index 40a08c4e5d8d..71b7610c3a3c 100644 > > > --- a/include/linux/ceph/osd_client.h > > > +++ b/include/linux/ceph/osd_client.h > > > @@ -481,6 +481,9 @@ extern struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client * > > > unsigned int num_ops, > > > bool use_mempool, > > > gfp_t gfp_flags); > > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > > + int num_request_data_items, > > > + int num_reply_data_items); > > > int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp); > > > > > > extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *, > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > > index 4ddf23120b1a..7be78fa6e2c3 100644 > > > --- a/net/ceph/osd_client.c > > > +++ b/net/ceph/osd_client.c > > > @@ -613,9 +613,9 @@ static int ceph_oloc_encoding_size(const struct ceph_object_locator *oloc) > > > return 8 + 4 + 4 + 4 + (oloc->pool_ns ? oloc->pool_ns->len : 0); > > > } > > > > > > -static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > > - int num_request_data_items, > > > - int num_reply_data_items) > > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > > + int num_request_data_items, > > > + int num_reply_data_items) > > > { > > > struct ceph_osd_client *osdc = req->r_osdc; > > > struct ceph_msg *msg; > > > @@ -672,6 +672,7 @@ static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > > > return 0; > > > } > > > +EXPORT_SYMBOL(ceph_osdc_alloc_num_messages); > > > > > > static bool osd_req_opcode_valid(u16 opcode) > > > { > > > @@ -738,8 +739,8 @@ int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp) > > > int num_request_data_items, num_reply_data_items; > > > > > > get_num_data_items(req, &num_request_data_items, &num_reply_data_items); > > > - return __ceph_osdc_alloc_messages(req, gfp, num_request_data_items, > > > - num_reply_data_items); > > > + return ceph_osdc_alloc_num_messages(req, gfp, num_request_data_items, > > > + num_reply_data_items); > > > } > > > EXPORT_SYMBOL(ceph_osdc_alloc_messages); > > > > > > @@ -1129,7 +1130,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc, > > > * also covers ceph_uninline_data(). If more multi-op request > > > * use cases emerge, we will need a separate helper. > > > */ > > > - r = __ceph_osdc_alloc_messages(req, GFP_NOFS, num_ops, 0); > > > + r = ceph_osdc_alloc_num_messages(req, GFP_NOFS, num_ops, 0); > > > else > > > r = ceph_osdc_alloc_messages(req, GFP_NOFS); > > > if (r) > > > > I think exporting __ceph_osdc_alloc_messages() is wrong, at least > > conceptually. Only the OSD client should be concerned with message > > data items and their count, as they are an implementation detail of > > the OSD client and the messenger. Exporting something that takes > > message data items counts without exporting something for counting > > them suggests that users will somehow do that on their own and we > > don't want that. > > > > We already do that in ceph_osdc_new_request(). That function takes a > num_ops value that describes the number of OSD ops needed, and the > callers have to fill it out with the number of ops they expect the call > to use (see the calls in ceph_writepages_start for example). The number of message data items is not necessarily the same as the number of OSD ops. Further, the number of message data items is different for request and reply messages of the OSD request. __ceph_osdc_alloc_messages() is private precisely to avoid this confusion. Thanks, Ilya
On Wed, 2020-07-01 at 21:40 +0200, Ilya Dryomov wrote: > On Wed, Jul 1, 2020 at 9:17 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2020-07-01 at 20:48 +0200, Ilya Dryomov wrote: > > > On Wed, Jul 1, 2020 at 5:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > ...and make it public and export it. > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > include/linux/ceph/osd_client.h | 3 +++ > > > > net/ceph/osd_client.c | 13 +++++++------ > > > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h > > > > index 40a08c4e5d8d..71b7610c3a3c 100644 > > > > --- a/include/linux/ceph/osd_client.h > > > > +++ b/include/linux/ceph/osd_client.h > > > > @@ -481,6 +481,9 @@ extern struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client * > > > > unsigned int num_ops, > > > > bool use_mempool, > > > > gfp_t gfp_flags); > > > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > + int num_request_data_items, > > > > + int num_reply_data_items); > > > > int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp); > > > > > > > > extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *, > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > > > index 4ddf23120b1a..7be78fa6e2c3 100644 > > > > --- a/net/ceph/osd_client.c > > > > +++ b/net/ceph/osd_client.c > > > > @@ -613,9 +613,9 @@ static int ceph_oloc_encoding_size(const struct ceph_object_locator *oloc) > > > > return 8 + 4 + 4 + 4 + (oloc->pool_ns ? oloc->pool_ns->len : 0); > > > > } > > > > > > > > -static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > - int num_request_data_items, > > > > - int num_reply_data_items) > > > > +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > + int num_request_data_items, > > > > + int num_reply_data_items) > > > > { > > > > struct ceph_osd_client *osdc = req->r_osdc; > > > > struct ceph_msg *msg; > > > > @@ -672,6 +672,7 @@ static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, > > > > > > > > return 0; > > > > } > > > > +EXPORT_SYMBOL(ceph_osdc_alloc_num_messages); > > > > > > > > static bool osd_req_opcode_valid(u16 opcode) > > > > { > > > > @@ -738,8 +739,8 @@ int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp) > > > > int num_request_data_items, num_reply_data_items; > > > > > > > > get_num_data_items(req, &num_request_data_items, &num_reply_data_items); > > > > - return __ceph_osdc_alloc_messages(req, gfp, num_request_data_items, > > > > - num_reply_data_items); > > > > + return ceph_osdc_alloc_num_messages(req, gfp, num_request_data_items, > > > > + num_reply_data_items); > > > > } > > > > EXPORT_SYMBOL(ceph_osdc_alloc_messages); > > > > > > > > @@ -1129,7 +1130,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc, > > > > * also covers ceph_uninline_data(). If more multi-op request > > > > * use cases emerge, we will need a separate helper. > > > > */ > > > > - r = __ceph_osdc_alloc_messages(req, GFP_NOFS, num_ops, 0); > > > > + r = ceph_osdc_alloc_num_messages(req, GFP_NOFS, num_ops, 0); > > > > else > > > > r = ceph_osdc_alloc_messages(req, GFP_NOFS); > > > > if (r) > > > > > > I think exporting __ceph_osdc_alloc_messages() is wrong, at least > > > conceptually. Only the OSD client should be concerned with message > > > data items and their count, as they are an implementation detail of > > > the OSD client and the messenger. Exporting something that takes > > > message data items counts without exporting something for counting > > > them suggests that users will somehow do that on their own and we > > > don't want that. > > > > > > > We already do that in ceph_osdc_new_request(). That function takes a > > num_ops value that describes the number of OSD ops needed, and the > > callers have to fill it out with the number of ops they expect the call > > to use (see the calls in ceph_writepages_start for example). > > The number of message data items is not necessarily the same as > the number of OSD ops. Further, the number of message data items > is different for request and reply messages of the OSD request. > __ceph_osdc_alloc_messages() is private precisely to avoid this > confusion. > Got it, thanks. I'll think about how to rework this code in light of that. Cheers,
diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h index 40a08c4e5d8d..71b7610c3a3c 100644 --- a/include/linux/ceph/osd_client.h +++ b/include/linux/ceph/osd_client.h @@ -481,6 +481,9 @@ extern struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client * unsigned int num_ops, bool use_mempool, gfp_t gfp_flags); +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, + int num_request_data_items, + int num_reply_data_items); int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp); extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *, diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 4ddf23120b1a..7be78fa6e2c3 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -613,9 +613,9 @@ static int ceph_oloc_encoding_size(const struct ceph_object_locator *oloc) return 8 + 4 + 4 + 4 + (oloc->pool_ns ? oloc->pool_ns->len : 0); } -static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, - int num_request_data_items, - int num_reply_data_items) +int ceph_osdc_alloc_num_messages(struct ceph_osd_request *req, gfp_t gfp, + int num_request_data_items, + int num_reply_data_items) { struct ceph_osd_client *osdc = req->r_osdc; struct ceph_msg *msg; @@ -672,6 +672,7 @@ static int __ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp, return 0; } +EXPORT_SYMBOL(ceph_osdc_alloc_num_messages); static bool osd_req_opcode_valid(u16 opcode) { @@ -738,8 +739,8 @@ int ceph_osdc_alloc_messages(struct ceph_osd_request *req, gfp_t gfp) int num_request_data_items, num_reply_data_items; get_num_data_items(req, &num_request_data_items, &num_reply_data_items); - return __ceph_osdc_alloc_messages(req, gfp, num_request_data_items, - num_reply_data_items); + return ceph_osdc_alloc_num_messages(req, gfp, num_request_data_items, + num_reply_data_items); } EXPORT_SYMBOL(ceph_osdc_alloc_messages); @@ -1129,7 +1130,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc, * also covers ceph_uninline_data(). If more multi-op request * use cases emerge, we will need a separate helper. */ - r = __ceph_osdc_alloc_messages(req, GFP_NOFS, num_ops, 0); + r = ceph_osdc_alloc_num_messages(req, GFP_NOFS, num_ops, 0); else r = ceph_osdc_alloc_messages(req, GFP_NOFS); if (r)
...and make it public and export it. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- include/linux/ceph/osd_client.h | 3 +++ net/ceph/osd_client.c | 13 +++++++------ 2 files changed, 10 insertions(+), 6 deletions(-)