Re: [PATCH v11 21/26] block/rnbd: server: main functionality

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	Roman Penyaev <rpenyaev@suse.de>,
	Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Subject: Re: [PATCH v11 21/26] block/rnbd: server: main functionality
Date: Tue, 31 Mar 2020 11:29:45 +0200	[thread overview]
Message-ID: <CAMGffE=4LNom-aWJhogqjgD+mHPp1_B9g=rEzHzdr=x9Zy6vAw@mail.gmail.com> (raw)
In-Reply-To: <ba7c258f-a169-f2d5-3d62-62a7d09908a4@acm.org>

On Sat, Mar 28, 2020 at 6:41 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2020-03-20 05:16, Jack Wang wrote:
> > +static int __read_mostly port_nr = RTRS_PORT;
>
> Would uint16_t be sufficient for this kernel module parameter?
yes, u16 is enough.
>
> Is this kernel module parameter used anywhere in the hot path? If not,
> should __read_mostly perhaps be left out?
not in hot path, __read_mostly can be removed.
>
> > +module_param_named(port_nr, port_nr, int, 0444);
> > +MODULE_PARM_DESC(port_nr,
> > +              "The port number server is listening on (default: "
>                                 ^^^
>                                 the?
right!
> > +              __stringify(RTRS_PORT)")");
> > +
> > +#define DEFAULT_DEV_SEARCH_PATH "/"
>
> > +static void destroy_device(struct rnbd_srv_dev *dev)
> > +{
> > +     WARN(!list_empty(&dev->sess_dev_list),
> > +          "Device %s is being destroyed but still in use!\n",
> > +          dev->id);
>
> Has it been considered to change WARN() into WARN_ONCE()?
ok.
>
> > +static int rnbd_srv_rdma_ev(struct rtrs_srv *rtrs, void *priv,
> > +                          struct rtrs_srv_op *id, int dir,
> > +                          void *data, size_t datalen, const void *usr,
> > +                          size_t usrlen)
> > +{
> > +     struct rnbd_srv_session *srv_sess = priv;
> > +     const struct rnbd_msg_hdr *hdr = usr;
> > +     int ret = 0;
> > +     u16 type;
> > +
> > +     if (WARN_ON(!srv_sess))
> > +             return -ENODEV;
>
> Same question here: how about using WARN_ON_ONCE() instead of WARN_ON()?
ok.
>
> > +static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
> > +                                  const char *dev_name)
> > +{
> > +     char *full_path;
> > +     char *a, *b;
> > +
> > +     full_path = kmalloc(PATH_MAX, GFP_KERNEL);
> > +     if (!full_path)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     /*
> > +      * Replace %SESSNAME% with a real session name in order to
> > +      * create device namespace.
> > +      */
> > +     a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
> > +     if (a) {
> > +             int len = a - dev_search_path;
> > +
> > +             len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
> > +                            dev_search_path, srv_sess->sessname, dev_name);
> > +             if (len >= PATH_MAX) {
> > +                     pr_err("Tooooo looong path: %s, %s, %s\n",
> > +                            dev_search_path, srv_sess->sessname, dev_name);
> > +                     kfree(full_path);
> > +                     return ERR_PTR(-EINVAL);
> > +             }
> > +     } else {
> > +             snprintf(full_path, PATH_MAX, "%s/%s",
> > +                      dev_search_path, dev_name);
> > +     }
>
> Has it been considered to use kasprintf() instead of kmalloc() + snprintf()?
will convert to kasprintf.
>
> Otherwise this patch looks fine to me.
>
> Thanks,
>
> Bart.
Thanks Bart!