[RFC PATCH 0/6] reduce the size of rnbd_clt_dev

[RFC PATCH 0/6] reduce the size of rnbd_clt_dev

[Guoqing Jiang]

Hi,

The struct rnbd_clt_dev added some members (wc, fua and max_hw_sectors
etc) which are used to set up gendisk and request_queue, but seems only
map scenario need to setup them since rnbd_client_setup_device is not
called from remap path.

Previously, pahole reports.

	/* size: 272, cachelines: 5, members: 29 */
	/* sum members: 259, holes: 4, sum holes: 13 */
	/* last cacheline: 16 bytes */

After the series, it changes to

	/* size: 224, cachelines: 4, members: 17 */
	/* last cacheline: 32 bytes */

Please review.

Thanks,
Guoqing   

Guoqing Jiang (6):
  rnbd-clt: open code send_msg_open in rnbd_clt_map_device
  rnbd-clt: don't free rsp in msg_open_conf for map scenario
  rnbd-clt: kill read_only from struct rnbd_clt_dev
  rnbd-clt: reduce the size of struct rnbd_clt_dev
  rnbd-clt: adjust the layout of struct rnbd_clt_dev
  rnbd-clt: refactor rnbd_clt_change_capacity

 drivers/block/rnbd/rnbd-clt-sysfs.c |   2 +-
 drivers/block/rnbd/rnbd-clt.c       | 202 +++++++++++++++++-----------
 drivers/block/rnbd/rnbd-clt.h       |  18 +--
 3 files changed, 124 insertions(+), 98 deletions(-)

-- 
2.34.1

[RFC PATCH 1/6] rnbd-clt: open code send_msg_open in rnbd_clt_map_device

[Guoqing Jiang]

Let's open code it in rnbd_clt_map_device, then we can use information
from rsp to setup gendisk and request_queue in next commits. After that,
we can remove some members (wc, fua and max_hw_sectors etc) from struct
rnbd_clt_dev.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt.c | 44 +++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 409c76b81aed..0396532da742 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -1562,7 +1562,14 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 {
 	struct rnbd_clt_session *sess;
 	struct rnbd_clt_dev *dev;
-	int ret;
+	int ret, errno;
+	struct rnbd_msg_open_rsp *rsp;
+	struct rnbd_msg_open msg;
+	struct rnbd_iu *iu;
+	struct kvec vec = {
+		.iov_base = &msg,
+		.iov_len  = sizeof(msg)
+	};
 
 	if (exists_devpath(pathname, sessname))
 		return ERR_PTR(-EEXIST);
@@ -1582,13 +1589,46 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 		ret = -EEXIST;
 		goto put_dev;
 	}
-	ret = send_msg_open(dev, RTRS_PERMIT_WAIT);
+
+	rsp = kzalloc(sizeof(*rsp), GFP_KERNEL);
+	if (!rsp) {
+		ret = -ENOMEM;
+		goto del_dev;
+	}
+
+	iu = rnbd_get_iu(sess, RTRS_ADMIN_CON, RTRS_PERMIT_WAIT);
+	if (!iu) {
+		ret = -ENOMEM;
+		kfree(rsp);
+		goto del_dev;
+	}
+	iu->buf = rsp;
+	iu->dev = dev;
+	sg_init_one(iu->sgt.sgl, rsp, sizeof(*rsp));
+
+	msg.hdr.type    = cpu_to_le16(RNBD_MSG_OPEN);
+	msg.access_mode = dev->access_mode;
+	strscpy(msg.dev_name, dev->pathname, sizeof(msg.dev_name));
+
+	WARN_ON(!rnbd_clt_get_dev(dev));
+	ret = send_usr_msg(sess->rtrs, READ, iu,
+			   &vec, sizeof(*rsp), iu->sgt.sgl, 1,
+			   msg_open_conf, &errno, RTRS_PERMIT_WAIT);
+	if (ret) {
+		rnbd_clt_put_dev(dev);
+		rnbd_put_iu(sess, iu);
+		kfree(rsp);
+	} else {
+		ret = errno;
+	}
+	rnbd_put_iu(sess, iu);
 	if (ret) {
 		rnbd_clt_err(dev,
 			      "map_device: failed, can't open remote device, err: %d\n",
 			      ret);
 		goto del_dev;
 	}
+
 	mutex_lock(&dev->lock);
 	pr_debug("Opened remote device: session=%s, path='%s'\n",
 		 sess->sessname, pathname);
-- 
2.34.1

[RFC PATCH 2/6] rnbd-clt: don't free rsp in msg_open_conf for map scenario

[Guoqing Jiang]

For map scenario, rsp is freed in two places:

1. msg_open_conf frees rsp if rtrs_clt_request returns 0.

2. Otherwise, rsp is freed by the call sites of rtrs_clt_request.

Now, We'd like to control full lifecycle of rsp in rnbd_clt_map_device,
with that, it is feasible to pass rsp to rnbd_client_setup_device in
next commit.

For 1, it is possible to free rsp from the caller of send_usr_msg
because of the synchronization of iu->comp.wait. And we put iu later
in rnbd_clt_map_device to ensure order of release rsp and iu.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 0396532da742..6c4970878d23 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -507,6 +507,11 @@ static void msg_open_conf(struct work_struct *work)
 	struct rnbd_msg_open_rsp *rsp = iu->buf;
 	struct rnbd_clt_dev *dev = iu->dev;
 	int errno = iu->errno;
+	bool from_map = false;
+
+	/* INIT state is only triggered from rnbd_clt_map_device */
+	if (dev->dev_state == DEV_STATE_INIT)
+		from_map = true;
 
 	if (errno) {
 		rnbd_clt_err(dev,
@@ -523,7 +528,9 @@ static void msg_open_conf(struct work_struct *work)
 			send_msg_close(dev, device_id, RTRS_PERMIT_NOWAIT);
 		}
 	}
-	kfree(rsp);
+	/* We free rsp in rnbd_clt_map_device for map scenario */
+	if (!from_map)
+		kfree(rsp);
 	wake_up_iu_comp(iu, errno);
 	rnbd_put_iu(dev->sess, iu);
 	rnbd_clt_put_dev(dev);
@@ -1617,16 +1624,14 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 	if (ret) {
 		rnbd_clt_put_dev(dev);
 		rnbd_put_iu(sess, iu);
-		kfree(rsp);
 	} else {
 		ret = errno;
 	}
-	rnbd_put_iu(sess, iu);
 	if (ret) {
 		rnbd_clt_err(dev,
 			      "map_device: failed, can't open remote device, err: %d\n",
 			      ret);
-		goto del_dev;
+		goto put_iu;
 	}
 
 	mutex_lock(&dev->lock);
@@ -1651,12 +1656,17 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 		       dev->max_hw_sectors, dev->wc, dev->fua);
 
 	mutex_unlock(&dev->lock);
+	kfree(rsp);
+	rnbd_put_iu(sess, iu);
 	rnbd_clt_put_sess(sess);
 
 	return dev;
 
 send_close:
 	send_msg_close(dev, dev->device_id, RTRS_PERMIT_WAIT);
+put_iu:
+	kfree(rsp);
+	rnbd_put_iu(sess, iu);
 del_dev:
 	delete_dev(dev);
 put_dev:
-- 
2.34.1

[RFC PATCH 3/6] rnbd-clt: kill read_only from struct rnbd_clt_dev

[Guoqing Jiang]

The member is not needed since we can call get_disk_ro to achieve the
same goal.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt.c | 8 ++------
 drivers/block/rnbd/rnbd-clt.h | 1 -
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 6c4970878d23..0bade2680eb9 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -949,7 +949,7 @@ static int rnbd_client_open(struct block_device *block_device, fmode_t mode)
 {
 	struct rnbd_clt_dev *dev = block_device->bd_disk->private_data;
 
-	if (dev->read_only && (mode & FMODE_WRITE))
+	if (get_disk_ro(dev->gd) && (mode & FMODE_WRITE))
 		return -EPERM;
 
 	if (dev->dev_state == DEV_STATE_UNMAPPED ||
@@ -1402,12 +1402,8 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
 
 	set_capacity(dev->gd, dev->nsectors);
 
-	if (dev->access_mode == RNBD_ACCESS_RO) {
-		dev->read_only = true;
+	if (dev->access_mode == RNBD_ACCESS_RO)
 		set_disk_ro(dev->gd, true);
-	} else {
-		dev->read_only = false;
-	}
 
 	/*
 	 * Network device does not need rotational
diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
index 2e2e8c4a85c1..26fb91d800e3 100644
--- a/drivers/block/rnbd/rnbd-clt.h
+++ b/drivers/block/rnbd/rnbd-clt.h
@@ -117,7 +117,6 @@ struct rnbd_clt_dev {
 	char			*pathname;
 	enum rnbd_access_mode	access_mode;
 	u32			nr_poll_queues;
-	bool			read_only;
 	bool			wc;
 	bool			fua;
 	u32			max_hw_sectors;
-- 
2.34.1

[RFC PATCH 4/6] rnbd-clt: reduce the size of struct rnbd_clt_dev

[Guoqing Jiang]

Previously, both map and remap trigger rnbd_clt_set_dev_attr to set
some members in rnbd_clt_dev such as wc, fua and logical_block_size
etc, but those members are only useful for map scenario given the
setup_request_queue is only called from the path:

rnbd_clt_map_device -> rnbd_client_setup_device

Since rnbd_clt_map_device frees rsp after rnbd_client_setup_device,
we can pass rsp to rnbd_client_setup_device and it's callees, which
means queue's attributes can be set directly from relevant members
of rsp instead from rnbd_clt_dev.

After that, we can kill 11 members from rnbd_clt_dev, and we don't
need rnbd_clt_set_dev_attr either.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt.c | 118 ++++++++++++++++------------------
 drivers/block/rnbd/rnbd-clt.h |  11 ----
 2 files changed, 55 insertions(+), 74 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 0bade2680eb9..2c63cd5ac09d 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -68,38 +68,12 @@ static inline bool rnbd_clt_get_dev(struct rnbd_clt_dev *dev)
 	return refcount_inc_not_zero(&dev->refcount);
 }
 
-static int rnbd_clt_set_dev_attr(struct rnbd_clt_dev *dev,
-				 const struct rnbd_msg_open_rsp *rsp)
-{
-	struct rnbd_clt_session *sess = dev->sess;
-
-	if (!rsp->logical_block_size)
-		return -EINVAL;
-
-	dev->device_id		    = le32_to_cpu(rsp->device_id);
-	dev->nsectors		    = le64_to_cpu(rsp->nsectors);
-	dev->logical_block_size	    = le16_to_cpu(rsp->logical_block_size);
-	dev->physical_block_size    = le16_to_cpu(rsp->physical_block_size);
-	dev->max_discard_sectors    = le32_to_cpu(rsp->max_discard_sectors);
-	dev->discard_granularity    = le32_to_cpu(rsp->discard_granularity);
-	dev->discard_alignment	    = le32_to_cpu(rsp->discard_alignment);
-	dev->secure_discard	    = le16_to_cpu(rsp->secure_discard);
-	dev->wc			    = !!(rsp->cache_policy & RNBD_WRITEBACK);
-	dev->fua		    = !!(rsp->cache_policy & RNBD_FUA);
-
-	dev->max_hw_sectors = sess->max_io_size / SECTOR_SIZE;
-	dev->max_segments = sess->max_segments;
-
-	return 0;
-}
-
 static int rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
 				    size_t new_nsectors)
 {
-	rnbd_clt_info(dev, "Device size changed from %zu to %zu sectors\n",
-		       dev->nsectors, new_nsectors);
-	dev->nsectors = new_nsectors;
-	set_capacity_and_notify(dev->gd, dev->nsectors);
+	rnbd_clt_info(dev, "Device size changed from %llu to %zu sectors\n",
+		      get_capacity(dev->gd), new_nsectors);
+	set_capacity_and_notify(dev->gd, new_nsectors);
 	return 0;
 }
 
@@ -123,15 +97,17 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
 		 * If the device was remapped and the size changed in the
 		 * meantime we need to revalidate it
 		 */
-		if (dev->nsectors != nsectors)
+		if (get_capacity(dev->gd) != nsectors)
 			rnbd_clt_change_capacity(dev, nsectors);
 		gd_kobj = &disk_to_dev(dev->gd)->kobj;
 		kobject_uevent(gd_kobj, KOBJ_ONLINE);
 		rnbd_clt_info(dev, "Device online, device remapped successfully\n");
 	}
-	err = rnbd_clt_set_dev_attr(dev, rsp);
-	if (err)
+	if (!rsp->logical_block_size) {
+		err = -EINVAL;
 		goto out;
+	}
+	dev->device_id = le32_to_cpu(rsp->device_id);
 	dev->dev_state = DEV_STATE_MAPPED;
 
 out:
@@ -970,10 +946,10 @@ static int rnbd_client_getgeo(struct block_device *block_device,
 			      struct hd_geometry *geo)
 {
 	u64 size;
-	struct rnbd_clt_dev *dev;
+	struct rnbd_clt_dev *dev = block_device->bd_disk->private_data;
+	struct queue_limits *limit = &dev->queue->limits;
 
-	dev = block_device->bd_disk->private_data;
-	size = dev->size * (dev->logical_block_size / SECTOR_SIZE);
+	size = dev->size * (limit->logical_block_size / SECTOR_SIZE);
 	geo->cylinders	= size >> 6;	/* size/64 */
 	geo->heads	= 4;
 	geo->sectors	= 16;
@@ -1357,11 +1333,15 @@ static void rnbd_init_mq_hw_queues(struct rnbd_clt_dev *dev)
 	}
 }
 
-static void setup_request_queue(struct rnbd_clt_dev *dev)
+static void setup_request_queue(struct rnbd_clt_dev *dev,
+				struct rnbd_msg_open_rsp *rsp)
 {
-	blk_queue_logical_block_size(dev->queue, dev->logical_block_size);
-	blk_queue_physical_block_size(dev->queue, dev->physical_block_size);
-	blk_queue_max_hw_sectors(dev->queue, dev->max_hw_sectors);
+	blk_queue_logical_block_size(dev->queue,
+				     le16_to_cpu(rsp->logical_block_size));
+	blk_queue_physical_block_size(dev->queue,
+				      le16_to_cpu(rsp->physical_block_size));
+	blk_queue_max_hw_sectors(dev->queue,
+				 dev->sess->max_io_size / SECTOR_SIZE);
 
 	/*
 	 * we don't support discards to "discontiguous" segments
@@ -1369,21 +1349,27 @@ static void setup_request_queue(struct rnbd_clt_dev *dev)
 	 */
 	blk_queue_max_discard_segments(dev->queue, 1);
 
-	blk_queue_max_discard_sectors(dev->queue, dev->max_discard_sectors);
-	dev->queue->limits.discard_granularity	= dev->discard_granularity;
-	dev->queue->limits.discard_alignment	= dev->discard_alignment;
-	if (dev->secure_discard)
+	blk_queue_max_discard_sectors(dev->queue,
+				      le32_to_cpu(rsp->max_discard_sectors));
+	dev->queue->limits.discard_granularity =
+					le32_to_cpu(rsp->discard_granularity);
+	dev->queue->limits.discard_alignment =
+					le32_to_cpu(rsp->discard_alignment);
+	if (le16_to_cpu(rsp->secure_discard))
 		blk_queue_max_secure_erase_sectors(dev->queue,
-				dev->max_discard_sectors);
+					le32_to_cpu(rsp->max_discard_sectors));
 	blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, dev->queue);
 	blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
-	blk_queue_max_segments(dev->queue, dev->max_segments);
+	blk_queue_max_segments(dev->queue, dev->sess->max_segments);
 	blk_queue_io_opt(dev->queue, dev->sess->max_io_size);
 	blk_queue_virt_boundary(dev->queue, SZ_4K - 1);
-	blk_queue_write_cache(dev->queue, dev->wc, dev->fua);
+	blk_queue_write_cache(dev->queue,
+			      !!(rsp->cache_policy & RNBD_WRITEBACK),
+			      !!(rsp->cache_policy & RNBD_FUA));
 }
 
-static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
+static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev,
+				   struct rnbd_msg_open_rsp *rsp, int idx)
 {
 	int err;
 
@@ -1395,12 +1381,12 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
 	dev->gd->private_data	= dev;
 	snprintf(dev->gd->disk_name, sizeof(dev->gd->disk_name), "rnbd%d",
 		 idx);
-	pr_debug("disk_name=%s, capacity=%zu\n",
+	pr_debug("disk_name=%s, capacity=%llu\n",
 		 dev->gd->disk_name,
-		 dev->nsectors * (dev->logical_block_size / SECTOR_SIZE)
-		 );
+		 le64_to_cpu(rsp->nsectors) *
+		 (le16_to_cpu(rsp->logical_block_size) / SECTOR_SIZE));
 
-	set_capacity(dev->gd, dev->nsectors);
+	set_capacity(dev->gd, le64_to_cpu(rsp->nsectors));
 
 	if (dev->access_mode == RNBD_ACCESS_RO)
 		set_disk_ro(dev->gd, true);
@@ -1416,11 +1402,13 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
 	return err;
 }
 
-static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)
+static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
+				    struct rnbd_msg_open_rsp *rsp)
 {
 	int idx = dev->clt_device_id;
 
-	dev->size = dev->nsectors * dev->logical_block_size;
+	dev->size = le64_to_cpu(rsp->nsectors) *
+			le16_to_cpu(rsp->logical_block_size);
 
 	dev->gd = blk_mq_alloc_disk(&dev->sess->tag_set, dev);
 	if (IS_ERR(dev->gd))
@@ -1428,8 +1416,8 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)
 	dev->queue = dev->gd->queue;
 	rnbd_init_mq_hw_queues(dev);
 
-	setup_request_queue(dev);
-	return rnbd_clt_setup_gen_disk(dev, idx);
+	setup_request_queue(dev, rsp);
+	return rnbd_clt_setup_gen_disk(dev, rsp, idx);
 }
 
 static struct rnbd_clt_dev *init_dev(struct rnbd_clt_session *sess,
@@ -1633,7 +1621,7 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 	mutex_lock(&dev->lock);
 	pr_debug("Opened remote device: session=%s, path='%s'\n",
 		 sess->sessname, pathname);
-	ret = rnbd_client_setup_device(dev);
+	ret = rnbd_client_setup_device(dev, rsp);
 	if (ret) {
 		rnbd_clt_err(dev,
 			      "map_device: Failed to configure device, err: %d\n",
@@ -1643,13 +1631,17 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
 	}
 
 	rnbd_clt_info(dev,
-		       "map_device: Device mapped as %s (nsectors: %zu, logical_block_size: %d, physical_block_size: %d, max_discard_sectors: %d, discard_granularity: %d, discard_alignment: %d, secure_discard: %d, max_segments: %d, max_hw_sectors: %d, wc: %d, fua: %d)\n",
-		       dev->gd->disk_name, dev->nsectors,
-		       dev->logical_block_size, dev->physical_block_size,
-		       dev->max_discard_sectors,
-		       dev->discard_granularity, dev->discard_alignment,
-		       dev->secure_discard, dev->max_segments,
-		       dev->max_hw_sectors, dev->wc, dev->fua);
+		       "map_device: Device mapped as %s (nsectors: %llu, logical_block_size: %d, physical_block_size: %d, max_discard_sectors: %d, discard_granularity: %d, discard_alignment: %d, secure_discard: %d, max_segments: %d, max_hw_sectors: %d, wc: %d, fua: %d)\n",
+		       dev->gd->disk_name, le64_to_cpu(rsp->nsectors),
+		       le16_to_cpu(rsp->logical_block_size),
+		       le16_to_cpu(rsp->physical_block_size),
+		       le32_to_cpu(rsp->max_discard_sectors),
+		       le32_to_cpu(rsp->discard_granularity),
+		       le32_to_cpu(rsp->discard_alignment),
+		       le16_to_cpu(rsp->secure_discard),
+		       sess->max_segments, sess->max_io_size / SECTOR_SIZE,
+		       !!(rsp->cache_policy & RNBD_WRITEBACK),
+		       !!(rsp->cache_policy & RNBD_FUA));
 
 	mutex_unlock(&dev->lock);
 	kfree(rsp);
diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
index 26fb91d800e3..7520272541b1 100644
--- a/drivers/block/rnbd/rnbd-clt.h
+++ b/drivers/block/rnbd/rnbd-clt.h
@@ -117,17 +117,6 @@ struct rnbd_clt_dev {
 	char			*pathname;
 	enum rnbd_access_mode	access_mode;
 	u32			nr_poll_queues;
-	bool			wc;
-	bool			fua;
-	u32			max_hw_sectors;
-	u32			max_discard_sectors;
-	u32			discard_granularity;
-	u32			discard_alignment;
-	u16			secure_discard;
-	u16			physical_block_size;
-	u16			logical_block_size;
-	u16			max_segments;
-	size_t			nsectors;
 	u64			size;		/* device size in bytes */
 	struct list_head        list;
 	struct gendisk		*gd;
-- 
2.34.1

[RFC PATCH 5/6] rnbd-clt: adjust the layout of struct rnbd_clt_dev

[Guoqing Jiang]

While at it, let re-arrange the struct to remove holes.

Before, pahole reports

	/* size: 232, cachelines: 4, members: 17 */
	/* sum members: 224, holes: 2, sum holes: 8 */
	/* last cacheline: 40 bytes */

After the change, the report changes to

	/* size: 224, cachelines: 4, members: 17 */
	/* last cacheline: 32 bytes */

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
index 7520272541b1..df237d2ea0d9 100644
--- a/drivers/block/rnbd/rnbd-clt.h
+++ b/drivers/block/rnbd/rnbd-clt.h
@@ -106,6 +106,7 @@ struct rnbd_queue {
 };
 
 struct rnbd_clt_dev {
+	struct kobject		kobj;
 	struct rnbd_clt_session	*sess;
 	struct request_queue	*queue;
 	struct rnbd_queue	*hw_queues;
@@ -114,15 +115,14 @@ struct rnbd_clt_dev {
 	u32			clt_device_id;
 	struct mutex		lock;
 	enum rnbd_clt_dev_state	dev_state;
+	refcount_t		refcount;
 	char			*pathname;
 	enum rnbd_access_mode	access_mode;
 	u32			nr_poll_queues;
 	u64			size;		/* device size in bytes */
 	struct list_head        list;
 	struct gendisk		*gd;
-	struct kobject		kobj;
 	char			*blk_symlink_name;
-	refcount_t		refcount;
 	struct work_struct	unmap_on_rmmod_work;
 };
 
-- 
2.34.1

[RFC PATCH 6/6] rnbd-clt: refactor rnbd_clt_change_capacity

[Guoqing Jiang]

1. process_msg_open_rsp checks if capacity changed or not before call
rnbd_clt_change_capacity while the checking also make sense for
rnbd_clt_resize_dev_store, let's move the checking into the function.

2. change the parameter type to 'sector_t' then we don't need to cast
it from rnbd_clt_resize_dev_store, and update rnbd_clt_resize_disk too.

3. no need to checking the return value, make it return void.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-clt-sysfs.c |  2 +-
 drivers/block/rnbd/rnbd-clt.c       | 24 ++++++++++++------------
 drivers/block/rnbd/rnbd-clt.h       |  2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-clt-sysfs.c b/drivers/block/rnbd/rnbd-clt-sysfs.c
index 2be5d87a3ca6..e7c7d9a68168 100644
--- a/drivers/block/rnbd/rnbd-clt-sysfs.c
+++ b/drivers/block/rnbd/rnbd-clt-sysfs.c
@@ -376,7 +376,7 @@ static ssize_t rnbd_clt_resize_dev_store(struct kobject *kobj,
 	if (ret)
 		return ret;
 
-	ret = rnbd_clt_resize_disk(dev, (size_t)sectors);
+	ret = rnbd_clt_resize_disk(dev, sectors);
 	if (ret)
 		return ret;
 
diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 2c63cd5ac09d..6c6c4ba3d41d 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -68,13 +68,18 @@ static inline bool rnbd_clt_get_dev(struct rnbd_clt_dev *dev)
 	return refcount_inc_not_zero(&dev->refcount);
 }
 
-static int rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
-				    size_t new_nsectors)
+static void rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
+				     sector_t new_nsectors)
 {
-	rnbd_clt_info(dev, "Device size changed from %llu to %zu sectors\n",
+	if (get_capacity(dev->gd) != new_nsectors)
+		return;
+
+	/*
+	 * If the size changed, we need to revalidate it
+	 */
+	rnbd_clt_info(dev, "Device size changed from %llu to %llu sectors\n",
 		      get_capacity(dev->gd), new_nsectors);
 	set_capacity_and_notify(dev->gd, new_nsectors);
-	return 0;
 }
 
 static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
@@ -93,12 +98,7 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
 	if (dev->dev_state == DEV_STATE_MAPPED_DISCONNECTED) {
 		u64 nsectors = le64_to_cpu(rsp->nsectors);
 
-		/*
-		 * If the device was remapped and the size changed in the
-		 * meantime we need to revalidate it
-		 */
-		if (get_capacity(dev->gd) != nsectors)
-			rnbd_clt_change_capacity(dev, nsectors);
+		rnbd_clt_change_capacity(dev, nsectors);
 		gd_kobj = &disk_to_dev(dev->gd)->kobj;
 		kobject_uevent(gd_kobj, KOBJ_ONLINE);
 		rnbd_clt_info(dev, "Device online, device remapped successfully\n");
@@ -116,7 +116,7 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
 	return err;
 }
 
-int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize)
+int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, sector_t newsize)
 {
 	int ret = 0;
 
@@ -126,7 +126,7 @@ int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize)
 		ret = -ENOENT;
 		goto out;
 	}
-	ret = rnbd_clt_change_capacity(dev, newsize);
+	rnbd_clt_change_capacity(dev, newsize);
 
 out:
 	mutex_unlock(&dev->lock);
diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
index df237d2ea0d9..a48e040abe63 100644
--- a/drivers/block/rnbd/rnbd-clt.h
+++ b/drivers/block/rnbd/rnbd-clt.h
@@ -138,7 +138,7 @@ int rnbd_clt_unmap_device(struct rnbd_clt_dev *dev, bool force,
 			   const struct attribute *sysfs_self);
 
 int rnbd_clt_remap_device(struct rnbd_clt_dev *dev);
-int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize);
+int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, sector_t newsize);
 
 /* rnbd-clt-sysfs.c */
 
-- 
2.34.1

Re: [RFC PATCH 0/6] reduce the size of rnbd_clt_dev

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:49 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Hi,
>
> The struct rnbd_clt_dev added some members (wc, fua and max_hw_sectors
> etc) which are used to set up gendisk and request_queue, but seems only
> map scenario need to setup them since rnbd_client_setup_device is not
> called from remap path.
>
> Previously, pahole reports.
>
>         /* size: 272, cachelines: 5, members: 29 */
>         /* sum members: 259, holes: 4, sum holes: 13 */
>         /* last cacheline: 16 bytes */
>
> After the series, it changes to
>
>         /* size: 224, cachelines: 4, members: 17 */
>         /* last cacheline: 32 bytes */
>
> Please review.
>
> Thanks,
> Guoqing
Hi Guoqing,

Thanks for the patchset, I had a brief look, in general I like the
idea, will run a regression test. and sort out the details.

Thanks!
Jinpu

Re: [RFC PATCH 1/6] rnbd-clt: open code send_msg_open in rnbd_clt_map_device

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:49 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Let's open code it in rnbd_clt_map_device, then we can use information
> from rsp to setup gendisk and request_queue in next commits. After that,
> we can remove some members (wc, fua and max_hw_sectors etc) from struct
> rnbd_clt_dev.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/block/rnbd/rnbd-clt.c | 44 +++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 409c76b81aed..0396532da742 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -1562,7 +1562,14 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>  {
>         struct rnbd_clt_session *sess;
>         struct rnbd_clt_dev *dev;
> -       int ret;
> +       int ret, errno;
> +       struct rnbd_msg_open_rsp *rsp;
> +       struct rnbd_msg_open msg;
> +       struct rnbd_iu *iu;
> +       struct kvec vec = {
> +               .iov_base = &msg,
> +               .iov_len  = sizeof(msg)
> +       };
>
>         if (exists_devpath(pathname, sessname))
>                 return ERR_PTR(-EEXIST);
> @@ -1582,13 +1589,46 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>                 ret = -EEXIST;
>                 goto put_dev;
>         }
> -       ret = send_msg_open(dev, RTRS_PERMIT_WAIT);
> +
> +       rsp = kzalloc(sizeof(*rsp), GFP_KERNEL);
> +       if (!rsp) {
> +               ret = -ENOMEM;
> +               goto del_dev;
> +       }
> +
> +       iu = rnbd_get_iu(sess, RTRS_ADMIN_CON, RTRS_PERMIT_WAIT);
> +       if (!iu) {
> +               ret = -ENOMEM;
> +               kfree(rsp);
> +               goto del_dev;
> +       }
> +       iu->buf = rsp;
> +       iu->dev = dev;
> +       sg_init_one(iu->sgt.sgl, rsp, sizeof(*rsp));
> +
> +       msg.hdr.type    = cpu_to_le16(RNBD_MSG_OPEN);
> +       msg.access_mode = dev->access_mode;
> +       strscpy(msg.dev_name, dev->pathname, sizeof(msg.dev_name));
> +
> +       WARN_ON(!rnbd_clt_get_dev(dev));
> +       ret = send_usr_msg(sess->rtrs, READ, iu,
> +                          &vec, sizeof(*rsp), iu->sgt.sgl, 1,
> +                          msg_open_conf, &errno, RTRS_PERMIT_WAIT);
> +       if (ret) {
> +               rnbd_clt_put_dev(dev);
> +               rnbd_put_iu(sess, iu);
> +               kfree(rsp);
> +       } else {
> +               ret = errno;
> +       }
> +       rnbd_put_iu(sess, iu);
>         if (ret) {
>                 rnbd_clt_err(dev,
>                               "map_device: failed, can't open remote device, err: %d\n",
>                               ret);
>                 goto del_dev;
>         }
> +
looks ok, except this new line seems not needed.

Thx!
>         mutex_lock(&dev->lock);
>         pr_debug("Opened remote device: session=%s, path='%s'\n",
>                  sess->sessname, pathname);
> --
> 2.34.1
>

Re: [RFC PATCH 6/6] rnbd-clt: refactor rnbd_clt_change_capacity

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> 1. process_msg_open_rsp checks if capacity changed or not before call
> rnbd_clt_change_capacity while the checking also make sense for
> rnbd_clt_resize_dev_store, let's move the checking into the function.
>
> 2. change the parameter type to 'sector_t' then we don't need to cast
> it from rnbd_clt_resize_dev_store, and update rnbd_clt_resize_disk too.
>
> 3. no need to checking the return value, make it return void.
>
better to split this into 3 patches.
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/block/rnbd/rnbd-clt-sysfs.c |  2 +-
>  drivers/block/rnbd/rnbd-clt.c       | 24 ++++++++++++------------
>  drivers/block/rnbd/rnbd-clt.h       |  2 +-
>  3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt-sysfs.c b/drivers/block/rnbd/rnbd-clt-sysfs.c
> index 2be5d87a3ca6..e7c7d9a68168 100644
> --- a/drivers/block/rnbd/rnbd-clt-sysfs.c
> +++ b/drivers/block/rnbd/rnbd-clt-sysfs.c
> @@ -376,7 +376,7 @@ static ssize_t rnbd_clt_resize_dev_store(struct kobject *kobj,
>         if (ret)
>                 return ret;
>
> -       ret = rnbd_clt_resize_disk(dev, (size_t)sectors);
> +       ret = rnbd_clt_resize_disk(dev, sectors);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 2c63cd5ac09d..6c6c4ba3d41d 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -68,13 +68,18 @@ static inline bool rnbd_clt_get_dev(struct rnbd_clt_dev *dev)
>         return refcount_inc_not_zero(&dev->refcount);
>  }
>
> -static int rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
> -                                   size_t new_nsectors)
> +static void rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
> +                                    sector_t new_nsectors)
>  {
> -       rnbd_clt_info(dev, "Device size changed from %llu to %zu sectors\n",
> +       if (get_capacity(dev->gd) != new_nsectors)
> +               return;
This change is broken, it leads to resize no longer work, should be "=="
> +
> +       /*
> +        * If the size changed, we need to revalidate it
> +        */
> +       rnbd_clt_info(dev, "Device size changed from %llu to %llu sectors\n",
>                       get_capacity(dev->gd), new_nsectors);
>         set_capacity_and_notify(dev->gd, new_nsectors);
> -       return 0;
>  }
>
>  static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
> @@ -93,12 +98,7 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
>         if (dev->dev_state == DEV_STATE_MAPPED_DISCONNECTED) {
>                 u64 nsectors = le64_to_cpu(rsp->nsectors);
>
> -               /*
> -                * If the device was remapped and the size changed in the
> -                * meantime we need to revalidate it
> -                */
> -               if (get_capacity(dev->gd) != nsectors)
> -                       rnbd_clt_change_capacity(dev, nsectors);
> +               rnbd_clt_change_capacity(dev, nsectors);
>                 gd_kobj = &disk_to_dev(dev->gd)->kobj;
>                 kobject_uevent(gd_kobj, KOBJ_ONLINE);
>                 rnbd_clt_info(dev, "Device online, device remapped successfully\n");
> @@ -116,7 +116,7 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
>         return err;
>  }
>
> -int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize)
> +int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, sector_t newsize)
>  {
>         int ret = 0;
>
> @@ -126,7 +126,7 @@ int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize)
>                 ret = -ENOENT;
>                 goto out;
>         }
> -       ret = rnbd_clt_change_capacity(dev, newsize);
> +       rnbd_clt_change_capacity(dev, newsize);
>
>  out:
>         mutex_unlock(&dev->lock);
> diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
> index df237d2ea0d9..a48e040abe63 100644
> --- a/drivers/block/rnbd/rnbd-clt.h
> +++ b/drivers/block/rnbd/rnbd-clt.h
> @@ -138,7 +138,7 @@ int rnbd_clt_unmap_device(struct rnbd_clt_dev *dev, bool force,
>                            const struct attribute *sysfs_self);
>
>  int rnbd_clt_remap_device(struct rnbd_clt_dev *dev);
> -int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, size_t newsize);
> +int rnbd_clt_resize_disk(struct rnbd_clt_dev *dev, sector_t newsize);
>
>  /* rnbd-clt-sysfs.c */
>
> --
> 2.34.1
>

Re: [RFC PATCH 2/6] rnbd-clt: don't free rsp in msg_open_conf for map scenario

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> For map scenario, rsp is freed in two places:
>
> 1. msg_open_conf frees rsp if rtrs_clt_request returns 0.
>
> 2. Otherwise, rsp is freed by the call sites of rtrs_clt_request.
>
> Now, We'd like to control full lifecycle of rsp in rnbd_clt_map_device,
> with that, it is feasible to pass rsp to rnbd_client_setup_device in
> next commit.
>
> For 1, it is possible to free rsp from the caller of send_usr_msg
> because of the synchronization of iu->comp.wait. And we put iu later
> in rnbd_clt_map_device to ensure order of release rsp and iu.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/block/rnbd/rnbd-clt.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 0396532da742..6c4970878d23 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -507,6 +507,11 @@ static void msg_open_conf(struct work_struct *work)
>         struct rnbd_msg_open_rsp *rsp = iu->buf;
>         struct rnbd_clt_dev *dev = iu->dev;
>         int errno = iu->errno;
> +       bool from_map = false;
> +
> +       /* INIT state is only triggered from rnbd_clt_map_device */
> +       if (dev->dev_state == DEV_STATE_INIT)
> +               from_map = true;
>
>         if (errno) {
>                 rnbd_clt_err(dev,
> @@ -523,7 +528,9 @@ static void msg_open_conf(struct work_struct *work)
>                         send_msg_close(dev, device_id, RTRS_PERMIT_NOWAIT);
>                 }
>         }
> -       kfree(rsp);
> +       /* We free rsp in rnbd_clt_map_device for map scenario */
> +       if (!from_map)
> +               kfree(rsp);
>         wake_up_iu_comp(iu, errno);
>         rnbd_put_iu(dev->sess, iu);
>         rnbd_clt_put_dev(dev);
> @@ -1617,16 +1624,14 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>         if (ret) {
>                 rnbd_clt_put_dev(dev);
>                 rnbd_put_iu(sess, iu);
> -               kfree(rsp);
>         } else {
>                 ret = errno;
>         }
> -       rnbd_put_iu(sess, iu);
>         if (ret) {
>                 rnbd_clt_err(dev,
>                               "map_device: failed, can't open remote device, err: %d\n",
>                               ret);
> -               goto del_dev;
> +               goto put_iu;
>         }
>
>         mutex_lock(&dev->lock);
> @@ -1651,12 +1656,17 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>                        dev->max_hw_sectors, dev->wc, dev->fua);
>
>         mutex_unlock(&dev->lock);
> +       kfree(rsp);
> +       rnbd_put_iu(sess, iu);
>         rnbd_clt_put_sess(sess);
>
>         return dev;
>
>  send_close:
>         send_msg_close(dev, dev->device_id, RTRS_PERMIT_WAIT);
> +put_iu:
> +       kfree(rsp);
> +       rnbd_put_iu(sess, iu);
>  del_dev:
>         delete_dev(dev);
>  put_dev:
> --
> 2.34.1
>
lgtm.
Acked-by: Jack Wang <jinpu.wang@ionos.com>

Re: [RFC PATCH 3/6] rnbd-clt: kill read_only from struct rnbd_clt_dev

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> The member is not needed since we can call get_disk_ro to achieve the
> same goal.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
lgtm
Acked-by: Jack Wang <jinpu.wang@ionos.com>


> ---
>  drivers/block/rnbd/rnbd-clt.c | 8 ++------
>  drivers/block/rnbd/rnbd-clt.h | 1 -
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 6c4970878d23..0bade2680eb9 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -949,7 +949,7 @@ static int rnbd_client_open(struct block_device *block_device, fmode_t mode)
>  {
>         struct rnbd_clt_dev *dev = block_device->bd_disk->private_data;
>
> -       if (dev->read_only && (mode & FMODE_WRITE))
> +       if (get_disk_ro(dev->gd) && (mode & FMODE_WRITE))
>                 return -EPERM;
>
>         if (dev->dev_state == DEV_STATE_UNMAPPED ||
> @@ -1402,12 +1402,8 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
>
>         set_capacity(dev->gd, dev->nsectors);
>
> -       if (dev->access_mode == RNBD_ACCESS_RO) {
> -               dev->read_only = true;
> +       if (dev->access_mode == RNBD_ACCESS_RO)
>                 set_disk_ro(dev->gd, true);
> -       } else {
> -               dev->read_only = false;
> -       }
>
>         /*
>          * Network device does not need rotational
> diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
> index 2e2e8c4a85c1..26fb91d800e3 100644
> --- a/drivers/block/rnbd/rnbd-clt.h
> +++ b/drivers/block/rnbd/rnbd-clt.h
> @@ -117,7 +117,6 @@ struct rnbd_clt_dev {
>         char                    *pathname;
>         enum rnbd_access_mode   access_mode;
>         u32                     nr_poll_queues;
> -       bool                    read_only;
>         bool                    wc;
>         bool                    fua;
>         u32                     max_hw_sectors;
> --
> 2.34.1
>

Re: [RFC PATCH 4/6] rnbd-clt: reduce the size of struct rnbd_clt_dev

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Previously, both map and remap trigger rnbd_clt_set_dev_attr to set
> some members in rnbd_clt_dev such as wc, fua and logical_block_size
> etc, but those members are only useful for map scenario given the
> setup_request_queue is only called from the path:
>
> rnbd_clt_map_device -> rnbd_client_setup_device
>
> Since rnbd_clt_map_device frees rsp after rnbd_client_setup_device,
> we can pass rsp to rnbd_client_setup_device and it's callees, which
> means queue's attributes can be set directly from relevant members
> of rsp instead from rnbd_clt_dev.
>
> After that, we can kill 11 members from rnbd_clt_dev, and we don't
> need rnbd_clt_set_dev_attr either.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
lgtm
Acked-by: Jack Wang <jinpu.wang@ionos.com>

> ---
>  drivers/block/rnbd/rnbd-clt.c | 118 ++++++++++++++++------------------
>  drivers/block/rnbd/rnbd-clt.h |  11 ----
>  2 files changed, 55 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 0bade2680eb9..2c63cd5ac09d 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -68,38 +68,12 @@ static inline bool rnbd_clt_get_dev(struct rnbd_clt_dev *dev)
>         return refcount_inc_not_zero(&dev->refcount);
>  }
>
> -static int rnbd_clt_set_dev_attr(struct rnbd_clt_dev *dev,
> -                                const struct rnbd_msg_open_rsp *rsp)
> -{
> -       struct rnbd_clt_session *sess = dev->sess;
> -
> -       if (!rsp->logical_block_size)
> -               return -EINVAL;
> -
> -       dev->device_id              = le32_to_cpu(rsp->device_id);
> -       dev->nsectors               = le64_to_cpu(rsp->nsectors);
> -       dev->logical_block_size     = le16_to_cpu(rsp->logical_block_size);
> -       dev->physical_block_size    = le16_to_cpu(rsp->physical_block_size);
> -       dev->max_discard_sectors    = le32_to_cpu(rsp->max_discard_sectors);
> -       dev->discard_granularity    = le32_to_cpu(rsp->discard_granularity);
> -       dev->discard_alignment      = le32_to_cpu(rsp->discard_alignment);
> -       dev->secure_discard         = le16_to_cpu(rsp->secure_discard);
> -       dev->wc                     = !!(rsp->cache_policy & RNBD_WRITEBACK);
> -       dev->fua                    = !!(rsp->cache_policy & RNBD_FUA);
> -
> -       dev->max_hw_sectors = sess->max_io_size / SECTOR_SIZE;
> -       dev->max_segments = sess->max_segments;
> -
> -       return 0;
> -}
> -
>  static int rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
>                                     size_t new_nsectors)
>  {
> -       rnbd_clt_info(dev, "Device size changed from %zu to %zu sectors\n",
> -                      dev->nsectors, new_nsectors);
> -       dev->nsectors = new_nsectors;
> -       set_capacity_and_notify(dev->gd, dev->nsectors);
> +       rnbd_clt_info(dev, "Device size changed from %llu to %zu sectors\n",
> +                     get_capacity(dev->gd), new_nsectors);
> +       set_capacity_and_notify(dev->gd, new_nsectors);
>         return 0;
>  }
>
> @@ -123,15 +97,17 @@ static int process_msg_open_rsp(struct rnbd_clt_dev *dev,
>                  * If the device was remapped and the size changed in the
>                  * meantime we need to revalidate it
>                  */
> -               if (dev->nsectors != nsectors)
> +               if (get_capacity(dev->gd) != nsectors)
>                         rnbd_clt_change_capacity(dev, nsectors);
>                 gd_kobj = &disk_to_dev(dev->gd)->kobj;
>                 kobject_uevent(gd_kobj, KOBJ_ONLINE);
>                 rnbd_clt_info(dev, "Device online, device remapped successfully\n");
>         }
> -       err = rnbd_clt_set_dev_attr(dev, rsp);
> -       if (err)
> +       if (!rsp->logical_block_size) {
> +               err = -EINVAL;
>                 goto out;
> +       }
> +       dev->device_id = le32_to_cpu(rsp->device_id);
>         dev->dev_state = DEV_STATE_MAPPED;
>
>  out:
> @@ -970,10 +946,10 @@ static int rnbd_client_getgeo(struct block_device *block_device,
>                               struct hd_geometry *geo)
>  {
>         u64 size;
> -       struct rnbd_clt_dev *dev;
> +       struct rnbd_clt_dev *dev = block_device->bd_disk->private_data;
> +       struct queue_limits *limit = &dev->queue->limits;
>
> -       dev = block_device->bd_disk->private_data;
> -       size = dev->size * (dev->logical_block_size / SECTOR_SIZE);
> +       size = dev->size * (limit->logical_block_size / SECTOR_SIZE);
>         geo->cylinders  = size >> 6;    /* size/64 */
>         geo->heads      = 4;
>         geo->sectors    = 16;
> @@ -1357,11 +1333,15 @@ static void rnbd_init_mq_hw_queues(struct rnbd_clt_dev *dev)
>         }
>  }
>
> -static void setup_request_queue(struct rnbd_clt_dev *dev)
> +static void setup_request_queue(struct rnbd_clt_dev *dev,
> +                               struct rnbd_msg_open_rsp *rsp)
>  {
> -       blk_queue_logical_block_size(dev->queue, dev->logical_block_size);
> -       blk_queue_physical_block_size(dev->queue, dev->physical_block_size);
> -       blk_queue_max_hw_sectors(dev->queue, dev->max_hw_sectors);
> +       blk_queue_logical_block_size(dev->queue,
> +                                    le16_to_cpu(rsp->logical_block_size));
> +       blk_queue_physical_block_size(dev->queue,
> +                                     le16_to_cpu(rsp->physical_block_size));
> +       blk_queue_max_hw_sectors(dev->queue,
> +                                dev->sess->max_io_size / SECTOR_SIZE);
>
>         /*
>          * we don't support discards to "discontiguous" segments
> @@ -1369,21 +1349,27 @@ static void setup_request_queue(struct rnbd_clt_dev *dev)
>          */
>         blk_queue_max_discard_segments(dev->queue, 1);
>
> -       blk_queue_max_discard_sectors(dev->queue, dev->max_discard_sectors);
> -       dev->queue->limits.discard_granularity  = dev->discard_granularity;
> -       dev->queue->limits.discard_alignment    = dev->discard_alignment;
> -       if (dev->secure_discard)
> +       blk_queue_max_discard_sectors(dev->queue,
> +                                     le32_to_cpu(rsp->max_discard_sectors));
> +       dev->queue->limits.discard_granularity =
> +                                       le32_to_cpu(rsp->discard_granularity);
> +       dev->queue->limits.discard_alignment =
> +                                       le32_to_cpu(rsp->discard_alignment);
> +       if (le16_to_cpu(rsp->secure_discard))
>                 blk_queue_max_secure_erase_sectors(dev->queue,
> -                               dev->max_discard_sectors);
> +                                       le32_to_cpu(rsp->max_discard_sectors));
>         blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, dev->queue);
>         blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
> -       blk_queue_max_segments(dev->queue, dev->max_segments);
> +       blk_queue_max_segments(dev->queue, dev->sess->max_segments);
>         blk_queue_io_opt(dev->queue, dev->sess->max_io_size);
>         blk_queue_virt_boundary(dev->queue, SZ_4K - 1);
> -       blk_queue_write_cache(dev->queue, dev->wc, dev->fua);
> +       blk_queue_write_cache(dev->queue,
> +                             !!(rsp->cache_policy & RNBD_WRITEBACK),
> +                             !!(rsp->cache_policy & RNBD_FUA));
>  }
>
> -static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
> +static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev,
> +                                  struct rnbd_msg_open_rsp *rsp, int idx)
>  {
>         int err;
>
> @@ -1395,12 +1381,12 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
>         dev->gd->private_data   = dev;
>         snprintf(dev->gd->disk_name, sizeof(dev->gd->disk_name), "rnbd%d",
>                  idx);
> -       pr_debug("disk_name=%s, capacity=%zu\n",
> +       pr_debug("disk_name=%s, capacity=%llu\n",
>                  dev->gd->disk_name,
> -                dev->nsectors * (dev->logical_block_size / SECTOR_SIZE)
> -                );
> +                le64_to_cpu(rsp->nsectors) *
> +                (le16_to_cpu(rsp->logical_block_size) / SECTOR_SIZE));
>
> -       set_capacity(dev->gd, dev->nsectors);
> +       set_capacity(dev->gd, le64_to_cpu(rsp->nsectors));
>
>         if (dev->access_mode == RNBD_ACCESS_RO)
>                 set_disk_ro(dev->gd, true);
> @@ -1416,11 +1402,13 @@ static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
>         return err;
>  }
>
> -static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)
> +static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
> +                                   struct rnbd_msg_open_rsp *rsp)
>  {
>         int idx = dev->clt_device_id;
>
> -       dev->size = dev->nsectors * dev->logical_block_size;
> +       dev->size = le64_to_cpu(rsp->nsectors) *
> +                       le16_to_cpu(rsp->logical_block_size);
>
>         dev->gd = blk_mq_alloc_disk(&dev->sess->tag_set, dev);
>         if (IS_ERR(dev->gd))
> @@ -1428,8 +1416,8 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)
>         dev->queue = dev->gd->queue;
>         rnbd_init_mq_hw_queues(dev);
>
> -       setup_request_queue(dev);
> -       return rnbd_clt_setup_gen_disk(dev, idx);
> +       setup_request_queue(dev, rsp);
> +       return rnbd_clt_setup_gen_disk(dev, rsp, idx);
>  }
>
>  static struct rnbd_clt_dev *init_dev(struct rnbd_clt_session *sess,
> @@ -1633,7 +1621,7 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>         mutex_lock(&dev->lock);
>         pr_debug("Opened remote device: session=%s, path='%s'\n",
>                  sess->sessname, pathname);
> -       ret = rnbd_client_setup_device(dev);
> +       ret = rnbd_client_setup_device(dev, rsp);
>         if (ret) {
>                 rnbd_clt_err(dev,
>                               "map_device: Failed to configure device, err: %d\n",
> @@ -1643,13 +1631,17 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>         }
>
>         rnbd_clt_info(dev,
> -                      "map_device: Device mapped as %s (nsectors: %zu, logical_block_size: %d, physical_block_size: %d, max_discard_sectors: %d, discard_granularity: %d, discard_alignment: %d, secure_discard: %d, max_segments: %d, max_hw_sectors: %d, wc: %d, fua: %d)\n",
> -                      dev->gd->disk_name, dev->nsectors,
> -                      dev->logical_block_size, dev->physical_block_size,
> -                      dev->max_discard_sectors,
> -                      dev->discard_granularity, dev->discard_alignment,
> -                      dev->secure_discard, dev->max_segments,
> -                      dev->max_hw_sectors, dev->wc, dev->fua);
> +                      "map_device: Device mapped as %s (nsectors: %llu, logical_block_size: %d, physical_block_size: %d, max_discard_sectors: %d, discard_granularity: %d, discard_alignment: %d, secure_discard: %d, max_segments: %d, max_hw_sectors: %d, wc: %d, fua: %d)\n",
> +                      dev->gd->disk_name, le64_to_cpu(rsp->nsectors),
> +                      le16_to_cpu(rsp->logical_block_size),
> +                      le16_to_cpu(rsp->physical_block_size),
> +                      le32_to_cpu(rsp->max_discard_sectors),
> +                      le32_to_cpu(rsp->discard_granularity),
> +                      le32_to_cpu(rsp->discard_alignment),
> +                      le16_to_cpu(rsp->secure_discard),
> +                      sess->max_segments, sess->max_io_size / SECTOR_SIZE,
> +                      !!(rsp->cache_policy & RNBD_WRITEBACK),
> +                      !!(rsp->cache_policy & RNBD_FUA));
>
>         mutex_unlock(&dev->lock);
>         kfree(rsp);
> diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
> index 26fb91d800e3..7520272541b1 100644
> --- a/drivers/block/rnbd/rnbd-clt.h
> +++ b/drivers/block/rnbd/rnbd-clt.h
> @@ -117,17 +117,6 @@ struct rnbd_clt_dev {
>         char                    *pathname;
>         enum rnbd_access_mode   access_mode;
>         u32                     nr_poll_queues;
> -       bool                    wc;
> -       bool                    fua;
> -       u32                     max_hw_sectors;
> -       u32                     max_discard_sectors;
> -       u32                     discard_granularity;
> -       u32                     discard_alignment;
> -       u16                     secure_discard;
> -       u16                     physical_block_size;
> -       u16                     logical_block_size;
> -       u16                     max_segments;
> -       size_t                  nsectors;
>         u64                     size;           /* device size in bytes */
>         struct list_head        list;
>         struct gendisk          *gd;
> --
> 2.34.1
>

Re: [RFC PATCH 5/6] rnbd-clt: adjust the layout of struct rnbd_clt_dev

[Jinpu Wang]

On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> While at it, let re-arrange the struct to remove holes.
>
> Before, pahole reports
>
>         /* size: 232, cachelines: 4, members: 17 */
>         /* sum members: 224, holes: 2, sum holes: 8 */
>         /* last cacheline: 40 bytes */
>
> After the change, the report changes to
>
>         /* size: 224, cachelines: 4, members: 17 */
>         /* last cacheline: 32 bytes */
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
lgtm
Acked-by: Jack Wang <jinpu.wang@ionos.com>

> ---
>  drivers/block/rnbd/rnbd-clt.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h
> index 7520272541b1..df237d2ea0d9 100644
> --- a/drivers/block/rnbd/rnbd-clt.h
> +++ b/drivers/block/rnbd/rnbd-clt.h
> @@ -106,6 +106,7 @@ struct rnbd_queue {
>  };
>
>  struct rnbd_clt_dev {
> +       struct kobject          kobj;
>         struct rnbd_clt_session *sess;
>         struct request_queue    *queue;
>         struct rnbd_queue       *hw_queues;
> @@ -114,15 +115,14 @@ struct rnbd_clt_dev {
>         u32                     clt_device_id;
>         struct mutex            lock;
>         enum rnbd_clt_dev_state dev_state;
> +       refcount_t              refcount;
>         char                    *pathname;
>         enum rnbd_access_mode   access_mode;
>         u32                     nr_poll_queues;
>         u64                     size;           /* device size in bytes */
>         struct list_head        list;
>         struct gendisk          *gd;
> -       struct kobject          kobj;
>         char                    *blk_symlink_name;
> -       refcount_t              refcount;
>         struct work_struct      unmap_on_rmmod_work;
>  };
>
> --
> 2.34.1
>

Re: [RFC PATCH 6/6] rnbd-clt: refactor rnbd_clt_change_capacity

[Guoqing Jiang]

On 6/22/22 6:57 PM, Jinpu Wang wrote:
> On Mon, Jun 20, 2022 at 5:50 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> 1. process_msg_open_rsp checks if capacity changed or not before call
>> rnbd_clt_change_capacity while the checking also make sense for
>> rnbd_clt_resize_dev_store, let's move the checking into the function.
>>
>> 2. change the parameter type to 'sector_t' then we don't need to cast
>> it from rnbd_clt_resize_dev_store, and update rnbd_clt_resize_disk too.
>>
>> 3. no need to checking the return value, make it return void.
>>
> better to split this into 3 patches.

Will do.

>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/block/rnbd/rnbd-clt-sysfs.c |  2 +-
>>   drivers/block/rnbd/rnbd-clt.c       | 24 ++++++++++++------------
>>   drivers/block/rnbd/rnbd-clt.h       |  2 +-
>>   3 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/block/rnbd/rnbd-clt-sysfs.c b/drivers/block/rnbd/rnbd-clt-sysfs.c
>> index 2be5d87a3ca6..e7c7d9a68168 100644
>> --- a/drivers/block/rnbd/rnbd-clt-sysfs.c
>> +++ b/drivers/block/rnbd/rnbd-clt-sysfs.c
>> @@ -376,7 +376,7 @@ static ssize_t rnbd_clt_resize_dev_store(struct kobject *kobj,
>>          if (ret)
>>                  return ret;
>>
>> -       ret = rnbd_clt_resize_disk(dev, (size_t)sectors);
>> +       ret = rnbd_clt_resize_disk(dev, sectors);
>>          if (ret)
>>                  return ret;
>>
>> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
>> index 2c63cd5ac09d..6c6c4ba3d41d 100644
>> --- a/drivers/block/rnbd/rnbd-clt.c
>> +++ b/drivers/block/rnbd/rnbd-clt.c
>> @@ -68,13 +68,18 @@ static inline bool rnbd_clt_get_dev(struct rnbd_clt_dev *dev)
>>          return refcount_inc_not_zero(&dev->refcount);
>>   }
>>
>> -static int rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
>> -                                   size_t new_nsectors)
>> +static void rnbd_clt_change_capacity(struct rnbd_clt_dev *dev,
>> +                                    sector_t new_nsectors)
>>   {
>> -       rnbd_clt_info(dev, "Device size changed from %llu to %zu sectors\n",
>> +       if (get_capacity(dev->gd) != new_nsectors)
>> +               return;
> This change is broken, it leads to resize no longer work, should be "=="

Yes, thanks for catch it.

Thanks,
Guoqing

Re: [RFC PATCH 1/6] rnbd-clt: open code send_msg_open in rnbd_clt_map_device

[Guoqing Jiang]

On 6/21/22 11:11 PM, Jinpu Wang wrote:
> On Mon, Jun 20, 2022 at 5:49 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Let's open code it in rnbd_clt_map_device, then we can use information
>> from rsp to setup gendisk and request_queue in next commits. After that,
>> we can remove some members (wc, fua and max_hw_sectors etc) from struct
>> rnbd_clt_dev.
>>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/block/rnbd/rnbd-clt.c | 44 +++++++++++++++++++++++++++++++++--
>>   1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
>> index 409c76b81aed..0396532da742 100644
>> --- a/drivers/block/rnbd/rnbd-clt.c
>> +++ b/drivers/block/rnbd/rnbd-clt.c
>> @@ -1562,7 +1562,14 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>>   {
>>          struct rnbd_clt_session *sess;
>>          struct rnbd_clt_dev *dev;
>> -       int ret;
>> +       int ret, errno;
>> +       struct rnbd_msg_open_rsp *rsp;
>> +       struct rnbd_msg_open msg;
>> +       struct rnbd_iu *iu;
>> +       struct kvec vec = {
>> +               .iov_base = &msg,
>> +               .iov_len  = sizeof(msg)
>> +       };
>>
>>          if (exists_devpath(pathname, sessname))
>>                  return ERR_PTR(-EEXIST);
>> @@ -1582,13 +1589,46 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname,
>>                  ret = -EEXIST;
>>                  goto put_dev;
>>          }
>> -       ret = send_msg_open(dev, RTRS_PERMIT_WAIT);
>> +
>> +       rsp = kzalloc(sizeof(*rsp), GFP_KERNEL);
>> +       if (!rsp) {
>> +               ret = -ENOMEM;
>> +               goto del_dev;
>> +       }
>> +
>> +       iu = rnbd_get_iu(sess, RTRS_ADMIN_CON, RTRS_PERMIT_WAIT);
>> +       if (!iu) {
>> +               ret = -ENOMEM;
>> +               kfree(rsp);
>> +               goto del_dev;
>> +       }
>> +       iu->buf = rsp;
>> +       iu->dev = dev;
>> +       sg_init_one(iu->sgt.sgl, rsp, sizeof(*rsp));
>> +
>> +       msg.hdr.type    = cpu_to_le16(RNBD_MSG_OPEN);
>> +       msg.access_mode = dev->access_mode;
>> +       strscpy(msg.dev_name, dev->pathname, sizeof(msg.dev_name));
>> +
>> +       WARN_ON(!rnbd_clt_get_dev(dev));
>> +       ret = send_usr_msg(sess->rtrs, READ, iu,
>> +                          &vec, sizeof(*rsp), iu->sgt.sgl, 1,
>> +                          msg_open_conf, &errno, RTRS_PERMIT_WAIT);
>> +       if (ret) {
>> +               rnbd_clt_put_dev(dev);
>> +               rnbd_put_iu(sess, iu);
>> +               kfree(rsp);
>> +       } else {
>> +               ret = errno;
>> +       }
>> +       rnbd_put_iu(sess, iu);
>>          if (ret) {
>>                  rnbd_clt_err(dev,
>>                                "map_device: failed, can't open remote device, err: %d\n",
>>                                ret);
>>                  goto del_dev;
>>          }
>> +
> looks ok, except this new line seems not needed.

Will remove it in next version.

Thanks,
Guoqing

Re: [RFC PATCH 0/6] reduce the size of rnbd_clt_dev

[Guoqing Jiang]

Hi Jinpu,

On 6/21/22 8:16 PM, Jinpu Wang wrote:
> On Mon, Jun 20, 2022 at 5:49 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Hi,
>>
>> The struct rnbd_clt_dev added some members (wc, fua and max_hw_sectors
>> etc) which are used to set up gendisk and request_queue, but seems only
>> map scenario need to setup them since rnbd_client_setup_device is not
>> called from remap path.
>>
>> Previously, pahole reports.
>>
>>          /* size: 272, cachelines: 5, members: 29 */
>>          /* sum members: 259, holes: 4, sum holes: 13 */
>>          /* last cacheline: 16 bytes */
>>
>> After the series, it changes to
>> t
>>          /* last cacheline: 32 bytes */
>>
>> Please review.
>>
>> Thanks,
>> Guoqing
> Hi Guoqing,
>
> Thanks for the patchset, I had a brief look, in general I like the
> idea, will run a regression test. and sort out the details.
>

Thanks for the review!

Thanks,
Guoqing