Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: axboe@kernel.dk, hch@infradead.org, sagi@grimberg.me,
	jgg@mellanox.com, dledford@redhat.com,
	danil.kipnis@cloud.ionos.com, rpenyaev@suse.de,
	Roman Pen <roman.penyaev@profitbricks.com>,
	Jack Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH v4 20/25] ibnbd: server: main functionality
Date: Wed, 18 Sep 2019 10:41:13 -0700
Message-ID: <5ceebb9c-b7ae-8e0c-6f07-d83e878e23d0@acm.org> (raw)
In-Reply-To: <20190620150337.7847-21-jinpuwang@gmail.com>

On 6/20/19 8:03 AM, Jack Wang wrote:
> +#undef pr_fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME " L" __stringify(__LINE__) ": " fmt

Same comment here as for a previous patch - please do not include line 
number information in pr_fmt().

> +MODULE_AUTHOR("ibnbd@profitbricks.com");
> +MODULE_VERSION(IBNBD_VER_STRING);
> +MODULE_DESCRIPTION("InfiniBand Network Block Device Server");
> +MODULE_LICENSE("GPL");

Please remove the version number (MODULE_VERSION()).

> +static char dev_search_path[PATH_MAX] = DEFAULT_DEV_SEARCH_PATH;

Please change dev_search_path[] into a dynamically allocated string to 
avoid a hard-coded length limit.

> +	if (dup[strlen(dup) - 1] == '\n')
> +		dup[strlen(dup) - 1] = '\0';

Can this be changed into a call to strim()?

> +static void ibnbd_endio(void *priv, int error)
> +{
> +	struct ibnbd_io_private *ibnbd_priv = priv;
> +	struct ibnbd_srv_sess_dev *sess_dev = ibnbd_priv->sess_dev;
> +
> +	ibnbd_put_sess_dev(sess_dev);
> +
> +	ibtrs_srv_resp_rdma(ibnbd_priv->id, error);
> +
> +	kfree(priv);
> +}

Since ibtrs_srv_resp_rdma() starts an RDMA WRITE without waiting for the 
write completion, shouldn't the session reference be retained until the 
completion for that RDMA WRITE has been received? In other words, is 
there a risk with the current approach that the buffer that is being 
transferred to the client will be freed before the RDMA WRITE has finished?

> +static struct ibnbd_srv_sess_dev *
> +ibnbd_get_sess_dev(int dev_id, struct ibnbd_srv_session *srv_sess)
> +{
> +	struct ibnbd_srv_sess_dev *sess_dev;
> +	int ret = 0;
> +
> +	read_lock(&srv_sess->index_lock);
> +	sess_dev = idr_find(&srv_sess->index_idr, dev_id);
> +	if (likely(sess_dev))
> +		ret = kref_get_unless_zero(&sess_dev->kref);
> +	read_unlock(&srv_sess->index_lock);
> +
> +	if (unlikely(!sess_dev || !ret))
> +		return ERR_PTR(-ENXIO);
> +
> +	return sess_dev;
> +}

Something that is not important: isn't the sess_dev check superfluous in 
the if-statement just above the return statement? If ret == 1, does that 
imply that sess_dev != 0 ?

Has it been considered to return -ENODEV instead of -ENXIO if no device 
is found?

> +static int create_sess(struct ibtrs_srv *ibtrs)
> +{
 > [ ... ]
> +	strlcpy(srv_sess->sessname, sessname, sizeof(srv_sess->sessname));

Please change the session name into a dynamically allocated string such 
that strdup() can be used instead of strlcpy().

> +static int process_msg_open(struct ibtrs_srv *ibtrs,
> +			    struct ibnbd_srv_session *srv_sess,
> +			    const void *msg, size_t len,
> +			    void *data, size_t datalen);
> +
> +static int process_msg_sess_info(struct ibtrs_srv *ibtrs,
> +				 struct ibnbd_srv_session *srv_sess,
> +				 const void *msg, size_t len,
> +				 void *data, size_t datalen);

Can the code be reordered such that these forward declarations can be 
dropped?

> +static struct ibnbd_srv_sess_dev *
> +ibnbd_srv_create_set_sess_dev(struct ibnbd_srv_session *srv_sess,
> +			      const struct ibnbd_msg_open *open_msg,
> +			      struct ibnbd_dev *ibnbd_dev, fmode_t open_flags,
> +			      struct ibnbd_srv_dev *srv_dev)
> +{
> +	struct ibnbd_srv_sess_dev *sdev = ibnbd_sess_dev_alloc(srv_sess);
> +
> +	if (IS_ERR(sdev))
> +		return sdev;
> +
> +	kref_init(&sdev->kref);
> +
> +	strlcpy(sdev->pathname, open_msg->dev_name, sizeof(sdev->pathname));

Can the path name be changed into a dynamically allocated string?

> +static char *ibnbd_srv_get_full_path(struct ibnbd_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()?

> +static int process_msg_sess_info(struct ibtrs_srv *ibtrs,
> +				 struct ibnbd_srv_session *srv_sess,
> +				 const void *msg, size_t len,
> +				 void *data, size_t datalen)
> +{
> +	const struct ibnbd_msg_sess_info *sess_info_msg = msg;
> +	struct ibnbd_msg_sess_info_rsp *rsp = data;
> +
> +	srv_sess->ver = min_t(u8, sess_info_msg->ver, IBNBD_PROTO_VER_MAJOR);
> +	pr_debug("Session %s using protocol version %d (client version: %d,"
> +		 " server version: %d)\n", srv_sess->sessname,
> +		 srv_sess->ver, sess_info_msg->ver, IBNBD_PROTO_VER_MAJOR);

Has this patch been verified with checkpatch? I think checkpatch 
recommends not to split literal strings.

> +/**
> + * find_srv_sess_dev() - a dev is already opened by this name
> + *
> + * Return struct ibnbd_srv_sess_dev if srv_sess already opened the dev_name
> + * NULL if the session didn't open the device yet.
> + */
> +static struct ibnbd_srv_sess_dev *
> +find_srv_sess_dev(struct ibnbd_srv_session *srv_sess, const char *dev_name)
> +{
> +	struct ibnbd_srv_sess_dev *sess_dev;
> +
> +	if (list_empty(&srv_sess->sess_dev_list))
> +		return NULL;
> +
> +	list_for_each_entry(sess_dev, &srv_sess->sess_dev_list, sess_list)
> +		if (!strcmp(sess_dev->pathname, dev_name))
> +			return sess_dev;
> +
> +	return NULL;
> +}

Is explicit the list_empty() check really necessary? Would the behavior 
of this function change if that check is left out?

Has the posted code been compiled with W=1? I'm asking this because the 
documentation of the function arguments is missing from the kernel-doc 
header. I expect that a warning will be reported if this code is 
compiled with W=1.

Thanks,

Bart.

  parent reply index

Thread overview: 123+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190620150337.7847-1-jinpuwang@gmail.com>
2019-06-20 15:03 ` [PATCH v4 01/25] sysfs: export sysfs_remove_file_self() Jack Wang
2019-09-23 17:21   ` Bart Van Assche
2019-09-25  9:30     ` Danil Kipnis
2019-07-09  9:55 ` [PATCH v4 00/25] InfiniBand Transport (IBTRS) and Network Block Device (IBNBD) Danil Kipnis
2019-07-09 11:00   ` Leon Romanovsky
2019-07-09 11:17     ` Greg KH
2019-07-09 11:57       ` Jinpu Wang
2019-07-09 13:32       ` Leon Romanovsky
2019-07-09 15:39       ` Bart Van Assche
2019-07-09 11:37     ` Jinpu Wang
2019-07-09 12:06       ` Jason Gunthorpe
2019-07-09 13:15         ` Jinpu Wang
2019-07-09 13:19           ` Jason Gunthorpe
2019-07-09 14:17             ` Jinpu Wang
2019-07-09 21:27             ` Sagi Grimberg
2019-07-19 13:12               ` Danil Kipnis
2019-07-10 14:55     ` Danil Kipnis
2019-07-09 12:04   ` Jason Gunthorpe
2019-07-09 19:45   ` Sagi Grimberg
2019-07-10 13:55     ` Jason Gunthorpe
2019-07-10 16:25       ` Sagi Grimberg
2019-07-10 17:25         ` Jason Gunthorpe
2019-07-10 19:11           ` Sagi Grimberg
2019-07-11  7:27             ` Danil Kipnis
2019-07-11  8:54     ` Danil Kipnis
2019-07-12  0:22       ` Sagi Grimberg
2019-07-12  7:57         ` Jinpu Wang
2019-07-12 19:40           ` Sagi Grimberg
2019-07-15 11:21             ` Jinpu Wang
2019-07-12 10:58         ` Danil Kipnis
     [not found] ` <20190620150337.7847-26-jinpuwang@gmail.com>
2019-07-09 15:10   ` [PATCH v4 25/25] MAINTAINERS: Add maintainer for IBNBD/IBTRS modules Leon Romanovsky
2019-07-09 15:18     ` Jinpu Wang
2019-07-09 15:51       ` Leon Romanovsky
2019-09-13 23:56   ` Bart Van Assche
2019-09-19 10:30     ` Jinpu Wang
     [not found] ` <20190620150337.7847-16-jinpuwang@gmail.com>
2019-09-13 22:10   ` [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers Bart Van Assche
2019-09-15 14:30     ` Jinpu Wang
2019-09-16  5:27       ` Leon Romanovsky
2019-09-16 13:45         ` Bart Van Assche
2019-09-17 15:41           ` Leon Romanovsky
2019-09-17 15:52             ` Jinpu Wang
2019-09-16  7:08       ` Danil Kipnis
2019-09-16 14:57       ` Jinpu Wang
2019-09-16 17:25         ` Bart Van Assche
2019-09-17 12:27           ` Jinpu Wang
2019-09-16 15:39       ` Jinpu Wang
2019-09-18 15:26         ` Bart Van Assche
2019-09-18 16:11           ` Jinpu Wang
     [not found] ` <20190620150337.7847-17-jinpuwang@gmail.com>
2019-09-13 22:25   ` [PATCH v4 16/25] ibnbd: client: private header with client structs and functions Bart Van Assche
2019-09-17 16:36     ` Jinpu Wang
2019-09-25 23:43       ` Danil Kipnis
2019-09-26 10:00         ` Jinpu Wang
     [not found] ` <20190620150337.7847-18-jinpuwang@gmail.com>
2019-09-13 23:46   ` [PATCH v4 17/25] ibnbd: client: main functionality Bart Van Assche
2019-09-16 14:17     ` Danil Kipnis
2019-09-16 16:46       ` Bart Van Assche
2019-09-17 11:39         ` Danil Kipnis
2019-09-18  7:14           ` Danil Kipnis
2019-09-18 15:47             ` Bart Van Assche
2019-09-20  8:29               ` Danil Kipnis
2019-09-25 22:26               ` Danil Kipnis
2019-09-26  9:55                 ` Roman Penyaev
2019-09-26 15:01                   ` Bart Van Assche
2019-09-27  8:52                     ` Roman Penyaev
2019-09-27  9:32                       ` Danil Kipnis
2019-09-27 12:18                         ` Danil Kipnis
2019-09-27 16:37                       ` Bart Van Assche
2019-09-27 16:50                         ` Roman Penyaev
2019-09-27 17:16                           ` Bart Van Assche
2019-09-17 13:09     ` Jinpu Wang
2019-09-17 16:46       ` Bart Van Assche
2019-09-18 12:02         ` Jinpu Wang
2019-09-18 16:05     ` Jinpu Wang
2019-09-14  0:00   ` Bart Van Assche
     [not found] ` <20190620150337.7847-25-jinpuwang@gmail.com>
2019-09-13 23:58   ` [PATCH v4 24/25] ibnbd: a bit of documentation Bart Van Assche
2019-09-18 12:22     ` Jinpu Wang
     [not found] ` <20190620150337.7847-19-jinpuwang@gmail.com>
2019-09-18 16:28   ` [PATCH v4 18/25] ibnbd: client: sysfs interface functions Bart Van Assche
2019-09-19 15:55     ` Jinpu Wang
     [not found] ` <20190620150337.7847-21-jinpuwang@gmail.com>
2019-09-18 17:41   ` Bart Van Assche [this message]
2019-09-20  7:36     ` [PATCH v4 20/25] ibnbd: server: main functionality Danil Kipnis
2019-09-20 15:42       ` Bart Van Assche
2019-09-23 15:19         ` Danil Kipnis
     [not found] ` <20190620150337.7847-22-jinpuwang@gmail.com>
2019-09-18 21:46   ` [PATCH v4 21/25] ibnbd: server: functionality for IO submission to file or block dev Bart Van Assche
2019-09-26 14:04     ` Jinpu Wang
2019-09-26 15:11       ` Bart Van Assche
2019-09-26 15:25         ` Danil Kipnis
2019-09-26 15:29           ` Bart Van Assche
2019-09-26 15:38             ` Danil Kipnis
2019-09-26 15:42               ` Jinpu Wang
     [not found] ` <20190620150337.7847-3-jinpuwang@gmail.com>
2019-09-23 17:44   ` [PATCH v4 02/25] ibtrs: public interface header to establish RDMA connections Bart Van Assche
2019-09-25 10:20     ` Danil Kipnis
2019-09-25 15:38       ` Bart Van Assche
     [not found] ` <20190620150337.7847-7-jinpuwang@gmail.com>
2019-09-23 21:51   ` [PATCH v4 06/25] ibtrs: client: main functionality Bart Van Assche
2019-09-25 17:36     ` Danil Kipnis
2019-09-25 18:55       ` Bart Van Assche
2019-09-25 20:50         ` Danil Kipnis
2019-09-25 21:08           ` Bart Van Assche
2019-09-25 21:16             ` Bart Van Assche
2019-09-25 22:53             ` Danil Kipnis
2019-09-25 23:21               ` Bart Van Assche
2019-09-26  9:16                 ` Danil Kipnis
     [not found] ` <20190620150337.7847-4-jinpuwang@gmail.com>
2019-09-23 22:50   ` [PATCH v4 03/25] ibtrs: private headers with IBTRS protocol structs and helpers Bart Van Assche
2019-09-25 21:45     ` Danil Kipnis
2019-09-25 21:57       ` Bart Van Assche
2019-09-27  8:56     ` Jinpu Wang
     [not found] ` <20190620150337.7847-5-jinpuwang@gmail.com>
2019-09-23 23:03   ` [PATCH v4 04/25] ibtrs: core: lib functions shared between client and server modules Bart Van Assche
2019-09-27 10:13     ` Jinpu Wang
     [not found] ` <20190620150337.7847-6-jinpuwang@gmail.com>
2019-09-23 23:05   ` [PATCH v4 05/25] ibtrs: client: private header with client structs and functions Bart Van Assche
2019-09-27 10:18     ` Jinpu Wang
     [not found] ` <20190620150337.7847-8-jinpuwang@gmail.com>
2019-09-23 23:15   ` [PATCH v4 07/25] ibtrs: client: statistics functions Bart Van Assche
2019-09-27 12:00     ` Jinpu Wang
     [not found] ` <20190620150337.7847-10-jinpuwang@gmail.com>
2019-09-23 23:21   ` [PATCH v4 09/25] ibtrs: server: private header with server structs and functions Bart Van Assche
2019-09-27 12:04     ` Jinpu Wang
     [not found] ` <20190620150337.7847-11-jinpuwang@gmail.com>
2019-09-23 23:49   ` [PATCH v4 10/25] ibtrs: server: main functionality Bart Van Assche
2019-09-27 15:03     ` Jinpu Wang
2019-09-27 15:11       ` Bart Van Assche
2019-09-27 15:19         ` Jinpu Wang
     [not found] ` <20190620150337.7847-12-jinpuwang@gmail.com>
2019-09-23 23:56   ` [PATCH v4 11/25] ibtrs: server: statistics functions Bart Van Assche
2019-10-02 15:15     ` Jinpu Wang
2019-10-02 15:42       ` Leon Romanovsky
2019-10-02 15:45         ` Jinpu Wang
2019-10-02 16:00           ` Leon Romanovsky
     [not found] ` <20190620150337.7847-13-jinpuwang@gmail.com>
2019-09-24  0:00   ` [PATCH v4 12/25] ibtrs: server: sysfs interface functions Bart Van Assche
2019-10-02 15:11     ` Jinpu Wang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5ceebb9c-b7ae-8e0c-6f07-d83e878e23d0@acm.org \
    --to=bvanassche@acm.org \
    --cc=axboe@kernel.dk \
    --cc=danil.kipnis@cloud.ionos.com \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jgg@mellanox.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=jinpuwang@gmail.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=roman.penyaev@profitbricks.com \
    --cc=rpenyaev@suse.de \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org linux-rdma@archiver.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/ public-inbox