On Wed, 2020-07-01 at 21:40 +0200, Ilya Dryomov wrote:
> On Wed, Jul 1, 2020 at 9:17 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > ...and make it public and export it.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > > 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,
--
Jeff Layton <jlayton@xxxxxxxxxx>
