ublk fixups

ublk fixups

[Christoph Hellwig]

Hi Ming, hi Jens,

this series has a bunch of fixes and cleanups of ublk.  The most important
one is the last one, which moves ublk over to a proper gendisk life cycle.

This series passes the ubdsrv tests.

Diffstat:
 MAINTAINERS                   |    7 
 drivers/block/ublk_drv.c      |  530 ++++++++++++++++++------------------------
 include/uapi/linux/ublk_cmd.h |    1 
 3 files changed, 241 insertions(+), 297 deletions(-)

[PATCH 1/8] ublk: add a MAINTAINERS entry

[Christoph Hellwig]

Make get_maintainers.pl work for ublk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe5daf1415013..8a4bbca9f28f4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20415,6 +20415,13 @@ F:	Documentation/filesystems/ubifs-authentication.rst
 F:	Documentation/filesystems/ubifs.rst
 F:	fs/ubifs/
 
+UBLK USERSPACE BLOCK DRIVER
+M:	Ming Lei <ming.lei@redhat.com>
+L:	linux-block@vger.kernel.org
+S:	Maintained
+F:	drivers/block/ublk_drv.c
+F:	include/uapi/linux/ublk_cmd.h
+
 UCLINUX (M68KNOMMU AND COLDFIRE)
 M:	Greg Ungerer <gerg@linux-m68k.org>
 L:	linux-m68k@lists.linux-m68k.org
-- 
2.30.2

[PATCH 2/8] ublk: remove UBLK_IO_F_PREFLUSH

[Christoph Hellwig]

REQ_PREFLUSH is turned into REQ_OP_FLUSH by the flush state machine
and thus never seen by a blk-mq based driver.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c      | 3 ---
 include/uapi/linux/ublk_cmd.h | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index b90481b295a74..07913b5bccd90 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -392,9 +392,6 @@ static inline unsigned int ublk_req_build_flags(struct request *req)
 	if (req->cmd_flags & REQ_FUA)
 		flags |= UBLK_IO_F_FUA;
 
-	if (req->cmd_flags & REQ_PREFLUSH)
-		flags |= UBLK_IO_F_PREFLUSH;
-
 	if (req->cmd_flags & REQ_NOUNMAP)
 		flags |= UBLK_IO_F_NOUNMAP;
 
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index d6879eea2fde0..917580b341984 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -107,7 +107,6 @@ struct ublksrv_ctrl_dev_info {
 #define		UBLK_IO_F_FAILFAST_DRIVER	(1U << 10)
 #define		UBLK_IO_F_META			(1U << 11)
 #define		UBLK_IO_F_FUA			(1U << 13)
-#define		UBLK_IO_F_PREFLUSH		(1U << 14)
 #define		UBLK_IO_F_NOUNMAP		(1U << 15)
 #define		UBLK_IO_F_SWAP			(1U << 16)
 
-- 
2.30.2

[PATCH 3/8] ublk: remove the empty open and release block device operations

[Christoph Hellwig]

No need to define empty versions, they can just be left out.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 07913b5bccd90..deabcb23ae2af 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -208,19 +208,8 @@ static inline int ublk_queue_cmd_buf_size(struct ublk_device *ub, int q_id)
 			PAGE_SIZE);
 }
 
-static int ublk_open(struct block_device *bdev, fmode_t mode)
-{
-	return 0;
-}
-
-static void ublk_release(struct gendisk *disk, fmode_t mode)
-{
-}
-
 static const struct block_device_operations ub_fops = {
 	.owner =	THIS_MODULE,
-	.open =		ublk_open,
-	.release =	ublk_release,
 };
 
 #define UBLK_MAX_PIN_PAGES	32
-- 
2.30.2

[PATCH 4/8] ublk: simplify ublk_ch_open and ublk_ch_release

[Christoph Hellwig]

fops->open and fops->release are always paired.  Use simple atomic bit
ops ot indicate if the device is opened instead of a count that can
only be 0 and 1 and a useless cmpxchg loop in ublk_ch_release.

Also don't bother clearing file->private_data is the file is about to
be freed anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index deabcb23ae2af..1f7bbbc3276a2 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -125,7 +125,8 @@ struct ublk_device {
 	struct cdev		cdev;
 	struct device		cdev_dev;
 
-	atomic_t		ch_open_cnt;
+#define UB_STATE_OPEN		(1 << 0)
+	unsigned long		state;
 	int			ub_number;
 
 	struct mutex		mutex;
@@ -647,21 +648,17 @@ static int ublk_ch_open(struct inode *inode, struct file *filp)
 	struct ublk_device *ub = container_of(inode->i_cdev,
 			struct ublk_device, cdev);
 
-	if (atomic_cmpxchg(&ub->ch_open_cnt, 0, 1) == 0) {
-		filp->private_data = ub;
-		return 0;
-	}
-	return -EBUSY;
+	if (test_and_set_bit(UB_STATE_OPEN, &ub->state))
+		return -EBUSY;
+	filp->private_data = ub;
+	return 0;
 }
 
 static int ublk_ch_release(struct inode *inode, struct file *filp)
 {
 	struct ublk_device *ub = filp->private_data;
 
-	while (atomic_cmpxchg(&ub->ch_open_cnt, 1, 0) != 1)
-		cpu_relax();
-
-	filp->private_data = NULL;
+	clear_bit(UB_STATE_OPEN, &ub->state);
 	return 0;
 }
 
-- 
2.30.2

[PATCH 5/8] ublk: cleanup ublk_ctrl_uring_cmd

[Christoph Hellwig]

Move all per-command work into the per-command ublk_ctrl_* helpers
instead of being split over those, ublk_ctrl_cmd_validate, and the main
ublk_ctrl_uring_cmd handler.  To facilitate that, the old
ublk_ctrl_stop_dev function that just contained two function calls is
folded into both callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 234 +++++++++++++++++++--------------------
 1 file changed, 116 insertions(+), 118 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 1f7bbbc3276a2..af70c18796e70 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -813,13 +813,6 @@ static void ublk_stop_dev(struct ublk_device *ub)
 	cancel_delayed_work_sync(&ub->monitor_work);
 }
 
-static int ublk_ctrl_stop_dev(struct ublk_device *ub)
-{
-	ublk_stop_dev(ub);
-	cancel_work_sync(&ub->stop_work);
-	return 0;
-}
-
 static inline bool ublk_queue_ready(struct ublk_queue *ubq)
 {
 	return ubq->nr_io_ready == ubq->q_depth;
@@ -1205,8 +1198,8 @@ static int ublk_add_dev(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	ublk_ctrl_stop_dev(ub);
-
+	ublk_stop_dev(ub);
+	cancel_work_sync(&ub->stop_work);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
 	put_device(&ub->cdev_dev);
 }
@@ -1227,36 +1220,45 @@ static struct ublk_device *ublk_get_device_from_id(int idx)
 	return ub;
 }
 
-static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd)
+static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
-	int ret = -EINVAL;
 	int ublksrv_pid = (int)header->data[0];
 	unsigned long dev_blocks = header->data[1];
+	struct ublk_device *ub;
+	int ret = -EINVAL;
 
 	if (ublksrv_pid <= 0)
-		return ret;
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
 
 	wait_for_completion_interruptible(&ub->completion);
 
 	schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD);
 
 	mutex_lock(&ub->mutex);
-	if (!disk_live(ub->ub_disk)) {
-		/* We may get disk size updated */
-		if (dev_blocks) {
-			ub->dev_info.dev_blocks = dev_blocks;
-			ublk_update_capacity(ub);
-		}
-		ub->dev_info.ublksrv_pid = ublksrv_pid;
-		ret = add_disk(ub->ub_disk);
-		if (!ret)
-			ub->dev_info.state = UBLK_S_DEV_LIVE;
-	} else {
+	if (disk_live(ub->ub_disk)) {
 		ret = -EEXIST;
+		goto out_unlock;
 	}
-	mutex_unlock(&ub->mutex);
 
+	/* We may get disk size updated */
+	if (dev_blocks) {
+		ub->dev_info.dev_blocks = dev_blocks;
+		ublk_update_capacity(ub);
+	}
+	ub->dev_info.ublksrv_pid = ublksrv_pid;
+	ret = add_disk(ub->ub_disk);
+	if (ret)
+		goto out_unlock;
+
+	ub->dev_info.state = UBLK_S_DEV_LIVE;
+out_unlock:
+	mutex_unlock(&ub->mutex);
+	ublk_put_device(ub);
 	return ret;
 }
 
@@ -1281,6 +1283,13 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 	unsigned long queue;
 	unsigned int retlen;
 	int ret = -EINVAL;
+	
+	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
+		return -EINVAL;
+	if (header->len & (sizeof(unsigned long)-1))
+		return -EINVAL;
+	if (!header->addr)
+		return -EINVAL;
 
 	ub = ublk_get_device_from_id(header->dev_id);
 	if (!ub)
@@ -1311,38 +1320,64 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 	return ret;
 }
 
-static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_dev_info *info,
-		void __user *argp, int idx)
+static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
 {
+	pr_devel("%s: dev id %d flags %llx\n", __func__,
+			info->dev_id, info->flags[0]);
+	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
+			info->nr_hw_queues, info->queue_depth,
+			info->block_size, info->dev_blocks);
+}
+
+static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
+	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublksrv_ctrl_dev_info info;
 	struct ublk_device *ub;
-	int ret;
+	int ret = -EINVAL;
+
+	if (header->len < sizeof(info) || !header->addr)
+		return -EINVAL;
+	if (header->queue_id != (u16)-1) {
+		pr_warn("%s: queue_id is wrong %x\n",
+			__func__, header->queue_id);
+		return -EINVAL;
+	}
+	if (copy_from_user(&info, argp, sizeof(info)))
+		return -EFAULT;
+	ublk_dump_dev_info(&ub->dev_info);
+	if (header->dev_id != info.dev_id) {
+		pr_warn("%s: dev id not match %u %u\n",
+			__func__, header->dev_id, info.dev_id);
+		return -EINVAL;
+	}
 
 	ret = mutex_lock_killable(&ublk_ctl_mutex);
 	if (ret)
 		return ret;
 
-	ub = __ublk_create_dev(idx);
-	if (!IS_ERR_OR_NULL(ub)) {
-		memcpy(&ub->dev_info, info, sizeof(*info));
+	ub = __ublk_create_dev(header->dev_id);
+	if (IS_ERR(ub)) {
+		ret = PTR_ERR(ub);
+		goto out_unlock;
+	}
 
-		/* update device id */
-		ub->dev_info.dev_id = ub->ub_number;
+	memcpy(&ub->dev_info, &info, sizeof(info));
 
-		ret = ublk_add_dev(ub);
-		if (!ret) {
-			if (copy_to_user(argp, &ub->dev_info, sizeof(*info))) {
-				ublk_remove(ub);
-				ret = -EFAULT;
-			}
-		}
-	} else {
-		if (IS_ERR(ub))
-			ret = PTR_ERR(ub);
-		else
-			ret = -ENOMEM;
+	/* update device id */
+	ub->dev_info.dev_id = ub->ub_number;
+
+	ret = ublk_add_dev(ub);
+	if (ret)
+		goto out_unlock;
+
+	if (copy_to_user(argp, &ub->dev_info, sizeof(info))) {
+		ublk_remove(ub);
+		ret = -EFAULT;
 	}
+out_unlock:
 	mutex_unlock(&ublk_ctl_mutex);
-
 	return ret;
 }
 
@@ -1386,16 +1421,6 @@ static int ublk_ctrl_del_dev(int idx)
 	return ret;
 }
 
-
-static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
-{
-	pr_devel("%s: dev id %d flags %llx\n", __func__,
-			info->dev_id, info->flags[0]);
-	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
-			info->nr_hw_queues, info->queue_depth,
-			info->block_size, info->dev_blocks);
-}
-
 static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
@@ -1405,59 +1430,47 @@ static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
 			header->data[0], header->addr, header->len);
 }
 
-static int ublk_ctrl_cmd_validate(struct io_uring_cmd *cmd,
-		struct ublksrv_ctrl_dev_info *info)
+static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
-	u32 cmd_op = cmd->cmd_op;
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublk_device *ub;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
 
-	switch (cmd_op) {
-	case UBLK_CMD_GET_DEV_INFO:
-		if (header->len < sizeof(*info) || !header->addr)
-			return -EINVAL;
-		break;
-	case UBLK_CMD_ADD_DEV:
-		if (header->len < sizeof(*info) || !header->addr)
-			return -EINVAL;
-		if (copy_from_user(info, argp, sizeof(*info)) != 0)
-			return -EFAULT;
-		ublk_dump_dev_info(info);
-		if (header->dev_id != info->dev_id) {
-			printk(KERN_WARNING "%s: cmd %x, dev id not match %u %u\n",
-					__func__, cmd_op, header->dev_id,
-					info->dev_id);
-			return -EINVAL;
-		}
-		if (header->queue_id != (u16)-1) {
-			printk(KERN_WARNING "%s: cmd %x queue_id is wrong %x\n",
-					__func__, cmd_op, header->queue_id);
-			return -EINVAL;
-		}
-		break;
-	case UBLK_CMD_GET_QUEUE_AFFINITY:
-		if ((header->len * BITS_PER_BYTE) < nr_cpu_ids)
-			return -EINVAL;
-		if (header->len & (sizeof(unsigned long)-1))
-			return -EINVAL;
-		if (!header->addr)
-			return -EINVAL;
-	}
+	ublk_stop_dev(ub);
+	cancel_work_sync(&ub->stop_work);
 
+	ublk_put_device(ub);
 	return 0;
 }
 
-static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
-		unsigned int issue_flags)
+static int ublk_ctrl_get_dev_info(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	void __user *argp = (void __user *)(unsigned long)header->addr;
-	struct ublksrv_ctrl_dev_info info;
-	u32 cmd_op = cmd->cmd_op;
 	struct ublk_device *ub;
+	int ret = 0;
+
+	if (header->len < sizeof(struct ublksrv_ctrl_dev_info) || !header->addr)
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
+
+	if (copy_to_user(argp, &ub->dev_info, sizeof(ub->dev_info)))
+		ret = -EFAULT;
+	ublk_put_device(ub);
+
+	return ret;
+}
+
+static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
+		unsigned int issue_flags)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	int ret = -EINVAL;
 
 	ublk_ctrl_cmd_dump(cmd);
@@ -1465,38 +1478,23 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 	if (!(issue_flags & IO_URING_F_SQE128))
 		goto out;
 
-	ret = ublk_ctrl_cmd_validate(cmd, &info);
-	if (ret)
+	ret = -EPERM;
+	if (!capable(CAP_SYS_ADMIN))
 		goto out;
 
 	ret = -ENODEV;
-	switch (cmd_op) {
+	switch (cmd->cmd_op) {
 	case UBLK_CMD_START_DEV:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			ret = ublk_ctrl_start_dev(ub, cmd);
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_start_dev(cmd);
 		break;
 	case UBLK_CMD_STOP_DEV:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			ret = ublk_ctrl_stop_dev(ub);
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_stop_dev(cmd);
 		break;
 	case UBLK_CMD_GET_DEV_INFO:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			if (copy_to_user(argp, &ub->dev_info, sizeof(info)))
-				ret = -EFAULT;
-			else
-				ret = 0;
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_get_dev_info(cmd);
 		break;
 	case UBLK_CMD_ADD_DEV:
-		ret = ublk_ctrl_add_dev(&info, argp, header->dev_id);
+		ret = ublk_ctrl_add_dev(cmd);
 		break;
 	case UBLK_CMD_DEL_DEV:
 		ret = ublk_ctrl_del_dev(header->dev_id);
-- 
2.30.2

[PATCH 6/8] ublk: fold __ublk_create_dev into ublk_ctrl_add_dev

[Christoph Hellwig]

Fold __ublk_create_dev into its only caller to avoid the packing and
unpacking of the return value into an ERR_PTR.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index af70c18796e70..7d57cbecfc8a0 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1017,23 +1017,6 @@ static int __ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	return err;
 }
 
-static struct ublk_device *__ublk_create_dev(int idx)
-{
-	struct ublk_device *ub = NULL;
-	int ret;
-
-	ub = kzalloc(sizeof(*ub), GFP_KERNEL);
-	if (!ub)
-		return ERR_PTR(-ENOMEM);
-
-	ret = __ublk_alloc_dev_number(ub, idx);
-	if (ret < 0) {
-		kfree(ub);
-		return ERR_PTR(ret);
-	}
-	return ub;
-}
-
 static void __ublk_destroy_dev(struct ublk_device *ub)
 {
 	spin_lock(&ublk_idr_lock);
@@ -1357,9 +1340,14 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	if (ret)
 		return ret;
 
-	ub = __ublk_create_dev(header->dev_id);
-	if (IS_ERR(ub)) {
-		ret = PTR_ERR(ub);
+	ret = -ENOMEM;
+	ub = kzalloc(sizeof(*ub), GFP_KERNEL);
+	if (!ub)
+		goto out_unlock;
+
+	ret = __ublk_alloc_dev_number(ub, header->dev_id);
+	if (ret < 0) {
+		kfree(ub);
 		goto out_unlock;
 	}
 
-- 
2.30.2

[PATCH 7/8] ublk: rewrite ublk_ctrl_get_queue_affinity to not rely on hctx->cpumask

[Christoph Hellwig]

Looking at the hctxs and cpumap is not safe without at very last a RCU
reference.  It also requires the queue to be set up before starting the
device, which leads to rather awkware life time rules.

Instead rewrite ublk_ctrl_get_queue_affinity to call blk_mq_map_queues
on an on-stack blk_mq_queue_map and build the cpumask from that.

Note: given that ublk has not made it into a released kernel it might
make sense to change the ABI for this command to instead copy the
qmap.mq_map array (a nr_cpu_ids sized array of unsigned integer
values  where the CPU index directly points to the queue) to userspace.

That way all the information could be transported to userspace in a
single command.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 63 ++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 7d57cbecfc8a0..67c6c46b8e07e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1245,26 +1245,16 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	return ret;
 }
 
-static struct blk_mq_hw_ctx *ublk_get_hw_queue(struct ublk_device *ub,
-		unsigned int index)
-{
-	struct blk_mq_hw_ctx *hctx;
-	unsigned long i;
-
-	queue_for_each_hw_ctx(ub->ub_queue, hctx, i)
-		if (hctx->queue_num == index)
-			return hctx;
-	return NULL;
-}
-
 static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	void __user *argp = (void __user *)(unsigned long)header->addr;
-	struct blk_mq_hw_ctx *hctx;
+	struct blk_mq_queue_map	qmap = {};
+	cpumask_var_t cpumask;
 	struct ublk_device *ub;
 	unsigned long queue;
 	unsigned int retlen;
+	unsigned int i;
 	int ret = -EINVAL;
 	
 	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
@@ -1276,30 +1266,41 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 
 	ub = ublk_get_device_from_id(header->dev_id);
 	if (!ub)
-		goto out;
+		return -EINVAL;
 
 	queue = header->data[0];
 	if (queue >= ub->dev_info.nr_hw_queues)
-		goto out;
-	hctx = ublk_get_hw_queue(ub, queue);
-	if (!hctx)
-		goto out;
+		goto out_put_device;
+
+	qmap.mq_map = kcalloc(nr_cpu_ids, sizeof(qmap.mq_map), GFP_KERNEL);
+	if (!qmap.mq_map)
+		goto out_put_device;
+	qmap.nr_queues = ub->dev_info.nr_hw_queues;
+	blk_mq_map_queues(&qmap);
+
+	ret = -ENOMEM;
+	if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
+		goto out_free_qmap;
+
+	for_each_possible_cpu(i)
+		if (qmap.mq_map[i] == queue)
+			cpumask_set_cpu(i, cpumask);
 
+	ret = -EFAULT;
 	retlen = min_t(unsigned short, header->len, cpumask_size());
-	if (copy_to_user(argp, hctx->cpumask, retlen)) {
-		ret = -EFAULT;
-		goto out;
-	}
-	if (retlen != header->len) {
-		if (clear_user(argp + retlen, header->len - retlen)) {
-			ret = -EFAULT;
-			goto out;
-		}
-	}
+	if (copy_to_user(argp, cpumask, retlen))
+		goto out_put_device;
+
+	if (retlen != header->len &&
+	    clear_user(argp + retlen, header->len - retlen))
+		goto out_put_device;
+
 	ret = 0;
- out:
-	if (ub)
-		ublk_put_device(ub);
+	free_cpumask_var(cpumask);
+out_free_qmap:
+	kfree(qmap.mq_map);
+out_put_device:
+	ublk_put_device(ub);
 	return ret;
 }
 
-- 
2.30.2

[PATCH 8/8] ublk: defer disk allocation

[Christoph Hellwig]

Defer allocating the gendisk and request_queue until UBLK_CMD_START_DEV
is called.  This avoids funky life times where a disk is allocated
and then can be added and removed multiple times, which has never been
supported by the block layer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 202 ++++++++++++++++-----------------------
 1 file changed, 85 insertions(+), 117 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 67c6c46b8e07e..85c3277ade372 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -112,7 +112,6 @@ struct ublk_queue {
 
 struct ublk_device {
 	struct gendisk		*ub_disk;
-	struct request_queue	*ub_queue;
 
 	char	*__queues;
 
@@ -126,6 +125,7 @@ struct ublk_device {
 	struct device		cdev_dev;
 
 #define UB_STATE_OPEN		(1 << 0)
+#define UB_STATE_USED		(1 << 1)
 	unsigned long		state;
 	int			ub_number;
 
@@ -156,8 +156,6 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
 
 static struct miscdevice ublk_misc;
 
-static struct lock_class_key ublk_bio_compl_lkclass;
-
 static inline bool ublk_can_use_task_work(const struct ublk_queue *ubq)
 {
 	if (IS_BUILTIN(CONFIG_BLK_DEV_UBLK) &&
@@ -209,8 +207,17 @@ static inline int ublk_queue_cmd_buf_size(struct ublk_device *ub, int q_id)
 			PAGE_SIZE);
 }
 
+static void ublk_free_disk(struct gendisk *disk)
+{
+	struct ublk_device *ub = disk->private_data;
+
+	clear_bit(UB_STATE_USED, &ub->state);
+	put_device(&ub->cdev_dev);
+}
+
 static const struct block_device_operations ub_fops = {
 	.owner =	THIS_MODULE,
+	.free_disk =	ublk_free_disk,
 };
 
 #define UBLK_MAX_PIN_PAGES	32
@@ -801,13 +808,16 @@ static void ublk_cancel_dev(struct ublk_device *ub)
 static void ublk_stop_dev(struct ublk_device *ub)
 {
 	mutex_lock(&ub->mutex);
-	if (!disk_live(ub->ub_disk))
+	if (ub->dev_info.state != UBLK_S_DEV_LIVE)
 		goto unlock;
 
 	del_gendisk(ub->ub_disk);
 	ub->dev_info.state = UBLK_S_DEV_DEAD;
 	ub->dev_info.ublksrv_pid = -1;
 	ublk_cancel_dev(ub);
+	blk_mq_free_tag_set(&ub->tag_set);
+	put_disk(ub->ub_disk);
+	ub->ub_disk = NULL;
  unlock:
 	mutex_unlock(&ub->mutex);
 	cancel_delayed_work_sync(&ub->monitor_work);
@@ -1033,12 +1043,6 @@ static void ublk_cdev_rel(struct device *dev)
 {
 	struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
 
-	blk_mq_destroy_queue(ub->ub_queue);
-
-	put_disk(ub->ub_disk);
-
-	blk_mq_free_tag_set(&ub->tag_set);
-
 	ublk_deinit_queues(ub);
 
 	__ublk_destroy_dev(ub);
@@ -1078,107 +1082,6 @@ static void ublk_stop_work_fn(struct work_struct *work)
 	ublk_stop_dev(ub);
 }
 
-static void ublk_update_capacity(struct ublk_device *ub)
-{
-	unsigned int max_rq_bytes;
-
-	/* make max request buffer size aligned with PAGE_SIZE */
-	max_rq_bytes = round_down(ub->dev_info.rq_max_blocks <<
-			ub->bs_shift, PAGE_SIZE);
-	ub->dev_info.rq_max_blocks = max_rq_bytes >> ub->bs_shift;
-
-	set_capacity(ub->ub_disk, ub->dev_info.dev_blocks << (ub->bs_shift - 9));
-}
-
-/* add disk & cdev, cleanup everything in case of failure */
-static int ublk_add_dev(struct ublk_device *ub)
-{
-	struct gendisk *disk;
-	int err = -ENOMEM;
-	int bsize;
-
-	/* We are not ready to support zero copy */
-	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
-
-	bsize = ub->dev_info.block_size;
-	ub->bs_shift = ilog2(bsize);
-
-	ub->dev_info.nr_hw_queues = min_t(unsigned int,
-			ub->dev_info.nr_hw_queues, nr_cpu_ids);
-
-	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
-	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
-
-	if (ublk_init_queues(ub))
-		goto out_destroy_dev;
-
-	ub->tag_set.ops = &ublk_mq_ops;
-	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
-	ub->tag_set.queue_depth = ub->dev_info.queue_depth;
-	ub->tag_set.numa_node = NUMA_NO_NODE;
-	ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
-	ub->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
-	ub->tag_set.driver_data = ub;
-
-	err = blk_mq_alloc_tag_set(&ub->tag_set);
-	if (err)
-		goto out_deinit_queues;
-
-	ub->ub_queue = blk_mq_init_queue(&ub->tag_set);
-	if (IS_ERR(ub->ub_queue)) {
-		err = PTR_ERR(ub->ub_queue);
-		goto out_cleanup_tags;
-	}
-	ub->ub_queue->queuedata = ub;
-
-	disk = ub->ub_disk = blk_mq_alloc_disk_for_queue(ub->ub_queue,
-						 &ublk_bio_compl_lkclass);
-	if (!disk) {
-		err = -ENOMEM;
-		goto out_free_request_queue;
-	}
-
-	blk_queue_logical_block_size(ub->ub_queue, bsize);
-	blk_queue_physical_block_size(ub->ub_queue, bsize);
-	blk_queue_io_min(ub->ub_queue, bsize);
-
-	blk_queue_max_hw_sectors(ub->ub_queue, ub->dev_info.rq_max_blocks <<
-			(ub->bs_shift - 9));
-
-	ub->ub_queue->limits.discard_granularity = PAGE_SIZE;
-
-	blk_queue_max_discard_sectors(ub->ub_queue, UINT_MAX >> 9);
-	blk_queue_max_write_zeroes_sectors(ub->ub_queue, UINT_MAX >> 9);
-
-	ublk_update_capacity(ub);
-
-	disk->fops		= &ub_fops;
-	disk->private_data	= ub;
-	disk->queue		= ub->ub_queue;
-	sprintf(disk->disk_name, "ublkb%d", ub->ub_number);
-
-	mutex_init(&ub->mutex);
-
-	/* add char dev so that ublksrv daemon can be setup */
-	err = ublk_add_chdev(ub);
-	if (err)
-		return err;
-
-	/* don't expose disk now until we got start command from cdev */
-
-	return 0;
-
-out_free_request_queue:
-	blk_mq_destroy_queue(ub->ub_queue);
-out_cleanup_tags:
-	blk_mq_free_tag_set(&ub->tag_set);
-out_deinit_queues:
-	ublk_deinit_queues(ub);
-out_destroy_dev:
-	__ublk_destroy_dev(ub);
-	return err;
-}
-
 static void ublk_remove(struct ublk_device *ub)
 {
 	ublk_stop_dev(ub);
@@ -1209,6 +1112,7 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	int ublksrv_pid = (int)header->data[0];
 	unsigned long dev_blocks = header->data[1];
 	struct ublk_device *ub;
+	struct gendisk *disk;
 	int ret = -EINVAL;
 
 	if (ublksrv_pid <= 0)
@@ -1223,22 +1127,62 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD);
 
 	mutex_lock(&ub->mutex);
-	if (disk_live(ub->ub_disk)) {
+	if (ub->dev_info.state == UBLK_S_DEV_LIVE ||
+	    test_bit(UB_STATE_USED, &ub->state)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
 
 	/* We may get disk size updated */
-	if (dev_blocks) {
+	if (dev_blocks)
 		ub->dev_info.dev_blocks = dev_blocks;
-		ublk_update_capacity(ub);
+
+	ub->tag_set.ops = &ublk_mq_ops;
+	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
+	ub->tag_set.queue_depth = ub->dev_info.queue_depth;
+	ub->tag_set.numa_node = NUMA_NO_NODE;
+	ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
+	ub->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+	ub->tag_set.driver_data = ub;
+	ret = blk_mq_alloc_tag_set(&ub->tag_set);
+	if (ret)
+		goto out_unlock;
+
+	disk = blk_mq_alloc_disk(&ub->tag_set, ub);
+	if (IS_ERR(disk)) {
+		ret = PTR_ERR(disk);
+		goto out_free_tag_set;
 	}
+	sprintf(disk->disk_name, "ublkb%d", ub->ub_number);
+	disk->fops = &ub_fops;
+	disk->private_data = ub;
+
+	blk_queue_logical_block_size(disk->queue, ub->dev_info.block_size);
+	blk_queue_physical_block_size(disk->queue, ub->dev_info.block_size);
+	blk_queue_io_min(disk->queue, ub->dev_info.block_size);
+	blk_queue_max_hw_sectors(disk->queue,
+		ub->dev_info.rq_max_blocks << (ub->bs_shift - 9));
+	disk->queue->limits.discard_granularity = PAGE_SIZE;
+	blk_queue_max_discard_sectors(disk->queue, UINT_MAX >> 9);
+	blk_queue_max_write_zeroes_sectors(disk->queue, UINT_MAX >> 9);
+
+	set_capacity(disk, ub->dev_info.dev_blocks << (ub->bs_shift - 9));
+
 	ub->dev_info.ublksrv_pid = ublksrv_pid;
-	ret = add_disk(ub->ub_disk);
+	ub->ub_disk = disk;
+	get_device(&ub->cdev_dev);
+	ret = add_disk(disk);
 	if (ret)
-		goto out_unlock;
+		goto out_put_disk;
 
+	set_bit(UB_STATE_USED, &ub->state);
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
+	goto out_unlock;
+
+out_put_disk:
+	put_disk(disk);
+out_free_tag_set:
+	blk_mq_free_tag_set(&ub->tag_set);
 out_unlock:
 	mutex_unlock(&ub->mutex);
 	ublk_put_device(ub);
@@ -1319,6 +1263,7 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	void __user *argp = (void __user *)(unsigned long)header->addr;
 	struct ublksrv_ctrl_dev_info info;
 	struct ublk_device *ub;
+	unsigned int max_rq_bytes;
 	int ret = -EINVAL;
 
 	if (header->len < sizeof(info) || !header->addr)
@@ -1352,12 +1297,35 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 		goto out_unlock;
 	}
 
+	mutex_init(&ub->mutex);
+	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
+	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
+
 	memcpy(&ub->dev_info, &info, sizeof(info));
 
 	/* update device id */
 	ub->dev_info.dev_id = ub->ub_number;
 
-	ret = ublk_add_dev(ub);
+	/* We are not ready to support zero copy */
+	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
+
+	ub->dev_info.nr_hw_queues = min_t(unsigned int,
+			ub->dev_info.nr_hw_queues, nr_cpu_ids);
+	ub->bs_shift = ilog2(ub->dev_info.block_size);
+
+	/* make max request buffer size aligned with PAGE_SIZE */
+	max_rq_bytes = ub->dev_info.rq_max_blocks << ub->bs_shift;
+	ub->dev_info.rq_max_blocks =
+		round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift;
+
+	ret = ublk_init_queues(ub);
+	if (ret) {
+		__ublk_destroy_dev(ub);
+		goto out_unlock;
+	}
+
+	/* add char dev so that ublksrv daemon can be setup */
+	ret = ublk_add_chdev(ub);
 	if (ret)
 		goto out_unlock;
 
-- 
2.30.2

Re: [PATCH 1/8] ublk: add a MAINTAINERS entry

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:25AM +0200, Christoph Hellwig wrote:
> Make get_maintainers.pl work for ublk.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH 2/8] ublk: remove UBLK_IO_F_PREFLUSH

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:26AM +0200, Christoph Hellwig wrote:
> REQ_PREFLUSH is turned into REQ_OP_FLUSH by the flush state machine
> and thus never seen by a blk-mq based driver.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 3/8] ublk: remove the empty open and release block device operations

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:27AM +0200, Christoph Hellwig wrote:
> No need to define empty versions, they can just be left out.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 4/8] ublk: simplify ublk_ch_open and ublk_ch_release

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:28AM +0200, Christoph Hellwig wrote:
> fops->open and fops->release are always paired.  Use simple atomic bit
> ops ot indicate if the device is opened instead of a count that can
> only be 0 and 1 and a useless cmpxchg loop in ublk_ch_release.
> 
> Also don't bother clearing file->private_data is the file is about to
> be freed anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 5/8] ublk: cleanup ublk_ctrl_uring_cmd

[Ziyang Zhang]

On 2022/7/21 13:16, Christoph Hellwig wrote:
> Move all per-command work into the per-command ublk_ctrl_* helpers
> instead of being split over those, ublk_ctrl_cmd_validate, and the main
> ublk_ctrl_uring_cmd handler.  To facilitate that, the old
> ublk_ctrl_stop_dev function that just contained two function calls is
> folded into both callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH 6/8] ublk: fold __ublk_create_dev into ublk_ctrl_add_dev

[Ziyang Zhang]

On 2022/7/21 13:16, Christoph Hellwig wrote:
> Fold __ublk_create_dev into its only caller to avoid the packing and
> unpacking of the return value into an ERR_PTR.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH 6/8] ublk: fold __ublk_create_dev into ublk_ctrl_add_dev

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:30AM +0200, Christoph Hellwig wrote:
> Fold __ublk_create_dev into its only caller to avoid the packing and
> unpacking of the return value into an ERR_PTR.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 7/8] ublk: rewrite ublk_ctrl_get_queue_affinity to not rely on hctx->cpumask

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:31AM +0200, Christoph Hellwig wrote:
> Looking at the hctxs and cpumap is not safe without at very last a RCU
> reference.  It also requires the queue to be set up before starting the
> device, which leads to rather awkware life time rules.
> 
> Instead rewrite ublk_ctrl_get_queue_affinity to call blk_mq_map_queues
> on an on-stack blk_mq_queue_map and build the cpumask from that.
> 
> Note: given that ublk has not made it into a released kernel it might
> make sense to change the ABI for this command to instead copy the
> qmap.mq_map array (a nr_cpu_ids sized array of unsigned integer
> values  where the CPU index directly points to the queue) to userspace.

qmap.mq_map is too big.

tag_set is embedded into ublk_device, and can be allocated once during
adding device, then ublk_ctrl_get_queue_affinity() can retrieve the info from
tag_set's map directly, then it is simplified a lot.

Also all info for building tag_set is setup during adding device, and
these info won't be changed after adding device, so it is reasonable
to allocate tagset just once.


Thanks,
Ming

Re: [PATCH 8/8] ublk: defer disk allocation

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:32AM +0200, Christoph Hellwig wrote:
> Defer allocating the gendisk and request_queue until UBLK_CMD_START_DEV
> is called.  This avoids funky life times where a disk is allocated
> and then can be added and removed multiple times, which has never been
> supported by the block layer.

As commented in last patch, tagset can be allocated once for retrieving
affinity reliably from tagset's map instead of building it via
blk_mq_map_queues() before allocating tagset, meantime the code gets
simplified.

Then you can still allocate queue/disk together.

Thanks,
Ming

Re: [PATCH 5/8] ublk: cleanup ublk_ctrl_uring_cmd

[Ming Lei]

On Thu, Jul 21, 2022 at 07:16:29AM +0200, Christoph Hellwig wrote:
> Move all per-command work into the per-command ublk_ctrl_* helpers
> instead of being split over those, ublk_ctrl_cmd_validate, and the main
> ublk_ctrl_uring_cmd handler.  To facilitate that, the old
> ublk_ctrl_stop_dev function that just contained two function calls is
> folded into both callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/block/ublk_drv.c | 234 +++++++++++++++++++--------------------
>  1 file changed, 116 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 1f7bbbc3276a2..af70c18796e70 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -813,13 +813,6 @@ static void ublk_stop_dev(struct ublk_device *ub)
>  	cancel_delayed_work_sync(&ub->monitor_work);
>  }
>  
> -static int ublk_ctrl_stop_dev(struct ublk_device *ub)
> -{
> -	ublk_stop_dev(ub);
> -	cancel_work_sync(&ub->stop_work);
> -	return 0;
> -}
> -
>  static inline bool ublk_queue_ready(struct ublk_queue *ubq)
>  {
>  	return ubq->nr_io_ready == ubq->q_depth;
> @@ -1205,8 +1198,8 @@ static int ublk_add_dev(struct ublk_device *ub)
>  
>  static void ublk_remove(struct ublk_device *ub)
>  {
> -	ublk_ctrl_stop_dev(ub);
> -
> +	ublk_stop_dev(ub);
> +	cancel_work_sync(&ub->stop_work);
>  	cdev_device_del(&ub->cdev, &ub->cdev_dev);
>  	put_device(&ub->cdev_dev);
>  }
> @@ -1227,36 +1220,45 @@ static struct ublk_device *ublk_get_device_from_id(int idx)
>  	return ub;
>  }
>  
> -static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd)
> +static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
>  {
>  	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> -	int ret = -EINVAL;
>  	int ublksrv_pid = (int)header->data[0];
>  	unsigned long dev_blocks = header->data[1];
> +	struct ublk_device *ub;
> +	int ret = -EINVAL;
>  
>  	if (ublksrv_pid <= 0)
> -		return ret;
> +		return -EINVAL;
> +
> +	ub = ublk_get_device_from_id(header->dev_id);
> +	if (!ub)
> +		return -EINVAL;
>  
>  	wait_for_completion_interruptible(&ub->completion);
>  
>  	schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD);
>  
>  	mutex_lock(&ub->mutex);
> -	if (!disk_live(ub->ub_disk)) {
> -		/* We may get disk size updated */
> -		if (dev_blocks) {
> -			ub->dev_info.dev_blocks = dev_blocks;
> -			ublk_update_capacity(ub);
> -		}
> -		ub->dev_info.ublksrv_pid = ublksrv_pid;
> -		ret = add_disk(ub->ub_disk);
> -		if (!ret)
> -			ub->dev_info.state = UBLK_S_DEV_LIVE;
> -	} else {
> +	if (disk_live(ub->ub_disk)) {
>  		ret = -EEXIST;
> +		goto out_unlock;
>  	}
> -	mutex_unlock(&ub->mutex);
>  
> +	/* We may get disk size updated */
> +	if (dev_blocks) {
> +		ub->dev_info.dev_blocks = dev_blocks;
> +		ublk_update_capacity(ub);
> +	}
> +	ub->dev_info.ublksrv_pid = ublksrv_pid;
> +	ret = add_disk(ub->ub_disk);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ub->dev_info.state = UBLK_S_DEV_LIVE;
> +out_unlock:
> +	mutex_unlock(&ub->mutex);
> +	ublk_put_device(ub);
>  	return ret;
>  }
>  
> @@ -1281,6 +1283,13 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
>  	unsigned long queue;
>  	unsigned int retlen;
>  	int ret = -EINVAL;
> +	
> +	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
> +		return -EINVAL;
> +	if (header->len & (sizeof(unsigned long)-1))
> +		return -EINVAL;
> +	if (!header->addr)
> +		return -EINVAL;
>  
>  	ub = ublk_get_device_from_id(header->dev_id);
>  	if (!ub)
> @@ -1311,38 +1320,64 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
>  	return ret;
>  }
>  
> -static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_dev_info *info,
> -		void __user *argp, int idx)
> +static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
>  {
> +	pr_devel("%s: dev id %d flags %llx\n", __func__,
> +			info->dev_id, info->flags[0]);
> +	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
> +			info->nr_hw_queues, info->queue_depth,
> +			info->block_size, info->dev_blocks);
> +}
> +
> +static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
> +{
> +	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> +	void __user *argp = (void __user *)(unsigned long)header->addr;
> +	struct ublksrv_ctrl_dev_info info;
>  	struct ublk_device *ub;
> -	int ret;
> +	int ret = -EINVAL;
> +
> +	if (header->len < sizeof(info) || !header->addr)
> +		return -EINVAL;
> +	if (header->queue_id != (u16)-1) {
> +		pr_warn("%s: queue_id is wrong %x\n",
> +			__func__, header->queue_id);
> +		return -EINVAL;
> +	}
> +	if (copy_from_user(&info, argp, sizeof(info)))
> +		return -EFAULT;
> +	ublk_dump_dev_info(&ub->dev_info);

The above line should have been ublk_dump_dev_info(&info), otherwise
looks fine:

Reviewed-by: Ming Lei <ming.lei@rehdat.com>

Thanks,
Ming

[PATCH 5/8] ublk: cleanup ublk_ctrl_uring_cmd

[Christoph Hellwig]

Move all per-command work into the per-command ublk_ctrl_* helpers
instead of being split over those, ublk_ctrl_cmd_validate, and the main
ublk_ctrl_uring_cmd handler.  To facilitate that, the old
ublk_ctrl_stop_dev function that just contained two function calls is
folded into both callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
---
 drivers/block/ublk_drv.c | 234 +++++++++++++++++++--------------------
 1 file changed, 116 insertions(+), 118 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 1f7bbbc3276a2..2032d677b9f18 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -813,13 +813,6 @@ static void ublk_stop_dev(struct ublk_device *ub)
 	cancel_delayed_work_sync(&ub->monitor_work);
 }
 
-static int ublk_ctrl_stop_dev(struct ublk_device *ub)
-{
-	ublk_stop_dev(ub);
-	cancel_work_sync(&ub->stop_work);
-	return 0;
-}
-
 static inline bool ublk_queue_ready(struct ublk_queue *ubq)
 {
 	return ubq->nr_io_ready == ubq->q_depth;
@@ -1205,8 +1198,8 @@ static int ublk_add_dev(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	ublk_ctrl_stop_dev(ub);
-
+	ublk_stop_dev(ub);
+	cancel_work_sync(&ub->stop_work);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
 	put_device(&ub->cdev_dev);
 }
@@ -1227,36 +1220,45 @@ static struct ublk_device *ublk_get_device_from_id(int idx)
 	return ub;
 }
 
-static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd)
+static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
-	int ret = -EINVAL;
 	int ublksrv_pid = (int)header->data[0];
 	unsigned long dev_blocks = header->data[1];
+	struct ublk_device *ub;
+	int ret = -EINVAL;
 
 	if (ublksrv_pid <= 0)
-		return ret;
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
 
 	wait_for_completion_interruptible(&ub->completion);
 
 	schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD);
 
 	mutex_lock(&ub->mutex);
-	if (!disk_live(ub->ub_disk)) {
-		/* We may get disk size updated */
-		if (dev_blocks) {
-			ub->dev_info.dev_blocks = dev_blocks;
-			ublk_update_capacity(ub);
-		}
-		ub->dev_info.ublksrv_pid = ublksrv_pid;
-		ret = add_disk(ub->ub_disk);
-		if (!ret)
-			ub->dev_info.state = UBLK_S_DEV_LIVE;
-	} else {
+	if (disk_live(ub->ub_disk)) {
 		ret = -EEXIST;
+		goto out_unlock;
 	}
-	mutex_unlock(&ub->mutex);
 
+	/* We may get disk size updated */
+	if (dev_blocks) {
+		ub->dev_info.dev_blocks = dev_blocks;
+		ublk_update_capacity(ub);
+	}
+	ub->dev_info.ublksrv_pid = ublksrv_pid;
+	ret = add_disk(ub->ub_disk);
+	if (ret)
+		goto out_unlock;
+
+	ub->dev_info.state = UBLK_S_DEV_LIVE;
+out_unlock:
+	mutex_unlock(&ub->mutex);
+	ublk_put_device(ub);
 	return ret;
 }
 
@@ -1281,6 +1283,13 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 	unsigned long queue;
 	unsigned int retlen;
 	int ret = -EINVAL;
+	
+	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
+		return -EINVAL;
+	if (header->len & (sizeof(unsigned long)-1))
+		return -EINVAL;
+	if (!header->addr)
+		return -EINVAL;
 
 	ub = ublk_get_device_from_id(header->dev_id);
 	if (!ub)
@@ -1311,38 +1320,64 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 	return ret;
 }
 
-static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_dev_info *info,
-		void __user *argp, int idx)
+static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
 {
+	pr_devel("%s: dev id %d flags %llx\n", __func__,
+			info->dev_id, info->flags[0]);
+	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
+			info->nr_hw_queues, info->queue_depth,
+			info->block_size, info->dev_blocks);
+}
+
+static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
+	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublksrv_ctrl_dev_info info;
 	struct ublk_device *ub;
-	int ret;
+	int ret = -EINVAL;
+
+	if (header->len < sizeof(info) || !header->addr)
+		return -EINVAL;
+	if (header->queue_id != (u16)-1) {
+		pr_warn("%s: queue_id is wrong %x\n",
+			__func__, header->queue_id);
+		return -EINVAL;
+	}
+	if (copy_from_user(&info, argp, sizeof(info)))
+		return -EFAULT;
+	ublk_dump_dev_info(&info);
+	if (header->dev_id != info.dev_id) {
+		pr_warn("%s: dev id not match %u %u\n",
+			__func__, header->dev_id, info.dev_id);
+		return -EINVAL;
+	}
 
 	ret = mutex_lock_killable(&ublk_ctl_mutex);
 	if (ret)
 		return ret;
 
-	ub = __ublk_create_dev(idx);
-	if (!IS_ERR_OR_NULL(ub)) {
-		memcpy(&ub->dev_info, info, sizeof(*info));
+	ub = __ublk_create_dev(header->dev_id);
+	if (IS_ERR(ub)) {
+		ret = PTR_ERR(ub);
+		goto out_unlock;
+	}
 
-		/* update device id */
-		ub->dev_info.dev_id = ub->ub_number;
+	memcpy(&ub->dev_info, &info, sizeof(info));
 
-		ret = ublk_add_dev(ub);
-		if (!ret) {
-			if (copy_to_user(argp, &ub->dev_info, sizeof(*info))) {
-				ublk_remove(ub);
-				ret = -EFAULT;
-			}
-		}
-	} else {
-		if (IS_ERR(ub))
-			ret = PTR_ERR(ub);
-		else
-			ret = -ENOMEM;
+	/* update device id */
+	ub->dev_info.dev_id = ub->ub_number;
+
+	ret = ublk_add_dev(ub);
+	if (ret)
+		goto out_unlock;
+
+	if (copy_to_user(argp, &ub->dev_info, sizeof(info))) {
+		ublk_remove(ub);
+		ret = -EFAULT;
 	}
+out_unlock:
 	mutex_unlock(&ublk_ctl_mutex);
-
 	return ret;
 }
 
@@ -1386,16 +1421,6 @@ static int ublk_ctrl_del_dev(int idx)
 	return ret;
 }
 
-
-static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
-{
-	pr_devel("%s: dev id %d flags %llx\n", __func__,
-			info->dev_id, info->flags[0]);
-	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
-			info->nr_hw_queues, info->queue_depth,
-			info->block_size, info->dev_blocks);
-}
-
 static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
@@ -1405,59 +1430,47 @@ static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
 			header->data[0], header->addr, header->len);
 }
 
-static int ublk_ctrl_cmd_validate(struct io_uring_cmd *cmd,
-		struct ublksrv_ctrl_dev_info *info)
+static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
-	u32 cmd_op = cmd->cmd_op;
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublk_device *ub;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
 
-	switch (cmd_op) {
-	case UBLK_CMD_GET_DEV_INFO:
-		if (header->len < sizeof(*info) || !header->addr)
-			return -EINVAL;
-		break;
-	case UBLK_CMD_ADD_DEV:
-		if (header->len < sizeof(*info) || !header->addr)
-			return -EINVAL;
-		if (copy_from_user(info, argp, sizeof(*info)) != 0)
-			return -EFAULT;
-		ublk_dump_dev_info(info);
-		if (header->dev_id != info->dev_id) {
-			printk(KERN_WARNING "%s: cmd %x, dev id not match %u %u\n",
-					__func__, cmd_op, header->dev_id,
-					info->dev_id);
-			return -EINVAL;
-		}
-		if (header->queue_id != (u16)-1) {
-			printk(KERN_WARNING "%s: cmd %x queue_id is wrong %x\n",
-					__func__, cmd_op, header->queue_id);
-			return -EINVAL;
-		}
-		break;
-	case UBLK_CMD_GET_QUEUE_AFFINITY:
-		if ((header->len * BITS_PER_BYTE) < nr_cpu_ids)
-			return -EINVAL;
-		if (header->len & (sizeof(unsigned long)-1))
-			return -EINVAL;
-		if (!header->addr)
-			return -EINVAL;
-	}
+	ublk_stop_dev(ub);
+	cancel_work_sync(&ub->stop_work);
 
+	ublk_put_device(ub);
 	return 0;
 }
 
-static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
-		unsigned int issue_flags)
+static int ublk_ctrl_get_dev_info(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	void __user *argp = (void __user *)(unsigned long)header->addr;
-	struct ublksrv_ctrl_dev_info info;
-	u32 cmd_op = cmd->cmd_op;
 	struct ublk_device *ub;
+	int ret = 0;
+
+	if (header->len < sizeof(struct ublksrv_ctrl_dev_info) || !header->addr)
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
+
+	if (copy_to_user(argp, &ub->dev_info, sizeof(ub->dev_info)))
+		ret = -EFAULT;
+	ublk_put_device(ub);
+
+	return ret;
+}
+
+static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
+		unsigned int issue_flags)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	int ret = -EINVAL;
 
 	ublk_ctrl_cmd_dump(cmd);
@@ -1465,38 +1478,23 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 	if (!(issue_flags & IO_URING_F_SQE128))
 		goto out;
 
-	ret = ublk_ctrl_cmd_validate(cmd, &info);
-	if (ret)
+	ret = -EPERM;
+	if (!capable(CAP_SYS_ADMIN))
 		goto out;
 
 	ret = -ENODEV;
-	switch (cmd_op) {
+	switch (cmd->cmd_op) {
 	case UBLK_CMD_START_DEV:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			ret = ublk_ctrl_start_dev(ub, cmd);
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_start_dev(cmd);
 		break;
 	case UBLK_CMD_STOP_DEV:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			ret = ublk_ctrl_stop_dev(ub);
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_stop_dev(cmd);
 		break;
 	case UBLK_CMD_GET_DEV_INFO:
-		ub = ublk_get_device_from_id(header->dev_id);
-		if (ub) {
-			if (copy_to_user(argp, &ub->dev_info, sizeof(info)))
-				ret = -EFAULT;
-			else
-				ret = 0;
-			ublk_put_device(ub);
-		}
+		ret = ublk_ctrl_get_dev_info(cmd);
 		break;
 	case UBLK_CMD_ADD_DEV:
-		ret = ublk_ctrl_add_dev(&info, argp, header->dev_id);
+		ret = ublk_ctrl_add_dev(cmd);
 		break;
 	case UBLK_CMD_DEL_DEV:
 		ret = ublk_ctrl_del_dev(header->dev_id);
-- 
2.30.2