Re: [PATCH 2/4] libceph: refactor osdc request initialization

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2020-07-01 at 20:08 +0200, Ilya Dryomov wrote:
> On Wed, Jul 1, 2020 at 5:54 PM Jeff Layton <jlayton@kernel.org> wrote:
> > Turn the request_init helper into a more full-featured initialization
> > routine that we can use to initialize an already-allocated request.
> > Make it a public and exported function so we can use it from ceph.ko.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  include/linux/ceph/osd_client.h |  4 ++++
> >  net/ceph/osd_client.c           | 28 +++++++++++++++-------------
> >  2 files changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h
> > index 8d63dc22cb36..40a08c4e5d8d 100644
> > --- a/include/linux/ceph/osd_client.h
> > +++ b/include/linux/ceph/osd_client.h
> > @@ -495,6 +495,10 @@ extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *,
> > 
> >  extern void ceph_osdc_get_request(struct ceph_osd_request *req);
> >  extern void ceph_osdc_put_request(struct ceph_osd_request *req);
> > +void ceph_osdc_init_request(struct ceph_osd_request *req,
> > +                           struct ceph_osd_client *osdc,
> > +                           struct ceph_snap_context *snapc,
> > +                           unsigned int num_ops);
> > 
> >  extern int ceph_osdc_start_request(struct ceph_osd_client *osdc,
> >                                    struct ceph_osd_request *req,
> > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> > index 3cff29d38b9f..4ddf23120b1a 100644
> > --- a/net/ceph/osd_client.c
> > +++ b/net/ceph/osd_client.c
> > @@ -523,7 +523,10 @@ void ceph_osdc_put_request(struct ceph_osd_request *req)
> >  }
> >  EXPORT_SYMBOL(ceph_osdc_put_request);
> > 
> > -static void request_init(struct ceph_osd_request *req)
> > +void ceph_osdc_init_request(struct ceph_osd_request *req,
> > +                           struct ceph_osd_client *osdc,
> > +                           struct ceph_snap_context *snapc,
> > +                           unsigned int num_ops)
> >  {
> >         /* req only, each op is zeroed in osd_req_op_init() */
> >         memset(req, 0, sizeof(*req));
> > @@ -535,7 +538,13 @@ static void request_init(struct ceph_osd_request *req)
> >         INIT_LIST_HEAD(&req->r_private_item);
> > 
> >         target_init(&req->r_t);
> > +
> > +       req->r_osdc = osdc;
> > +       req->r_num_ops = num_ops;
> > +       req->r_snapid = CEPH_NOSNAP;
> > +       req->r_snapc = ceph_get_snap_context(snapc);
> >  }
> > +EXPORT_SYMBOL(ceph_osdc_init_request);
> > 
> >  /*
> >   * This is ugly, but it allows us to reuse linger registration and ping
> > @@ -563,12 +572,9 @@ static void request_reinit(struct ceph_osd_request *req)
> >         WARN_ON(kref_read(&reply_msg->kref) != 1);
> >         target_destroy(&req->r_t);
> > 
> > -       request_init(req);
> > -       req->r_osdc = osdc;
> > +       ceph_osdc_init_request(req, osdc, snapc, num_ops);
> >         req->r_mempool = mempool;
> > -       req->r_num_ops = num_ops;
> >         req->r_snapid = snapid;
> > -       req->r_snapc = snapc;
> >         req->r_linger = linger;
> >         req->r_request = request_msg;
> >         req->r_reply = reply_msg;
> > @@ -591,15 +597,11 @@ struct ceph_osd_request *ceph_osdc_alloc_request(struct ceph_osd_client *osdc,
> >                 BUG_ON(num_ops > CEPH_OSD_MAX_OPS);
> >                 req = kmalloc(struct_size(req, r_ops, num_ops), gfp_flags);
> >         }
> > -       if (unlikely(!req))
> > -               return NULL;
> > 
> > -       request_init(req);
> > -       req->r_osdc = osdc;
> > -       req->r_mempool = use_mempool;
> > -       req->r_num_ops = num_ops;
> > -       req->r_snapid = CEPH_NOSNAP;
> > -       req->r_snapc = ceph_get_snap_context(snapc);
> > +       if (likely(req)) {
> > +               req->r_mempool = use_mempool;
> > +               ceph_osdc_init_request(req, osdc, snapc, num_ops);
> > +       }
> > 
> >         dout("%s req %p\n", __func__, req);
> >         return req;
> 
> What is going to use ceph_osdc_init_request()?
> 
> Given that OSD request allocation is non-trivial, exporting a
> routine for initializing already allocated requests doesn't seem
> like a good idea.  How do you ensure that the OSD request to be
> initialized has enough room for passed num_ops?  What about calling
> this routine on a request that has already been initialized?
> None of these issues exist today...
> 

Oops, I put this one in the pile by mistake. We don't need this,
currently. I'll drop this patch from the series.

I've been working with dhowells' fscache rework and the exported helper
would be needed in the patches I have to wire that up to cephfs.
Basically we'll need to allocate a structure that contains both a
fscache request and an OSD request. If those come to fruition, then
we'll need something like this (and an updated way to handle freeing
those objects).

-- 
Jeff Layton <jlayton@kernel.org>