Re: [PATCH 3/4] libceph: rename __ceph_osdc_alloc_messages to ceph_osdc_alloc_num_messages

[Ilya Dryomov <idryomov@gmail.com>]

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