[PATCH 0/3] poll() for DM events

[PATCH 0/3] poll() for DM events

[Andy Grover]

Hi all,

This patchset allows events for multiple DM devices to be monitored by
a single thread, instead of requiring one thread per device. This is
made possible by a new ioctl to create an association between an open
file descriptor to /dev/mapper/control and a DM device. A program can
open and associate multiple times, and then poll() all the fds.

Since we now track something per-fd - the association - our new ioctl
handler needs the struct file*. lookup_ioctl() requires all ioctl
handlers have the same arguments, so patch 1 changes all ioctl_fns to
add this.

Patch 2 implements support, but limits associated fds to just being
used with poll(), and the VERSION ioctl.

Patch 3 (optional) shows how we could use the associated device to do
additional ioctls without needing to specify the device in the
ioctl().

There's also a simple test program at:

https://github.com/agrover/dm-poll-test

Thanks -- Andy

Andy Grover (3):
  dm: Change ioctl handlers to take struct file*
  dm: Add support to poll for dm events
  dm: Allow associated fd to be used for TABLE_STATUS

 drivers/md/dm-core.h          |  10 +++
 drivers/md/dm-ioctl.c         | 149 +++++++++++++++++++++++++++++++++++-------
 drivers/md/dm.c               |  12 ++++
 include/uapi/linux/dm-ioctl.h |   6 +-
 4 files changed, 151 insertions(+), 26 deletions(-)

-- 
2.9.3

[PATCH 1/3] dm: Change ioctl handlers to take struct file*

[Andy Grover]

We will need access to struct file in a following commit in an ioctl
handler. Since lookup_ioctl() wants all ioctl handler functions to have
the same signature, change all signatures to take struct file.

Signed-off-by: Andy Grover <agrover@redhat.com>
---
 drivers/md/dm-ioctl.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index a5a9b17..17c9bf9 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -463,9 +463,9 @@ void dm_deferred_remove(void)
  * All the ioctl commands get dispatched to functions with this
  * prototype.
  */
-typedef int (*ioctl_fn)(struct dm_ioctl *param, size_t param_size);
+typedef int (*ioctl_fn)(struct file *filp, struct dm_ioctl *param, size_t param_size);
 
-static int remove_all(struct dm_ioctl *param, size_t param_size)
+static int remove_all(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	dm_hash_remove_all(true, !!(param->flags & DM_DEFERRED_REMOVE), false);
 	param->data_size = 0;
@@ -498,7 +498,7 @@ static void *get_result_buffer(struct dm_ioctl *param, size_t param_size,
 	return ((void *) param) + param->data_start;
 }
 
-static int list_devices(struct dm_ioctl *param, size_t param_size)
+static int list_devices(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	unsigned int i;
 	struct hash_cell *hc;
@@ -589,7 +589,7 @@ static void list_version_get_info(struct target_type *tt, void *param)
     info->vers = align_ptr(((void *) ++info->vers) + strlen(tt->name) + 1);
 }
 
-static int list_versions(struct dm_ioctl *param, size_t param_size)
+static int list_versions(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	size_t len, needed = 0;
 	struct dm_target_versions *vers;
@@ -731,7 +731,7 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
 	}
 }
 
-static int dev_create(struct dm_ioctl *param, size_t param_size)
+static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r, m = DM_ANY_MINOR;
 	struct mapped_device *md;
@@ -823,7 +823,7 @@ static struct mapped_device *find_device(struct dm_ioctl *param)
 	return md;
 }
 
-static int dev_remove(struct dm_ioctl *param, size_t param_size)
+static int dev_remove(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	struct hash_cell *hc;
 	struct mapped_device *md;
@@ -888,7 +888,7 @@ static int invalid_str(char *str, void *end)
 	return -EINVAL;
 }
 
-static int dev_rename(struct dm_ioctl *param, size_t param_size)
+static int dev_rename(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r;
 	char *new_data = (char *) param + param->data_start;
@@ -918,7 +918,7 @@ static int dev_rename(struct dm_ioctl *param, size_t param_size)
 	return 0;
 }
 
-static int dev_set_geometry(struct dm_ioctl *param, size_t param_size)
+static int dev_set_geometry(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r = -EINVAL, x;
 	struct mapped_device *md;
@@ -1067,7 +1067,7 @@ static int do_resume(struct dm_ioctl *param)
  * Set or unset the suspension state of a device.
  * If the device already is in the requested state we just return its status.
  */
-static int dev_suspend(struct dm_ioctl *param, size_t param_size)
+static int dev_suspend(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	if (param->flags & DM_SUSPEND_FLAG)
 		return do_suspend(param);
@@ -1079,7 +1079,7 @@ static int dev_suspend(struct dm_ioctl *param, size_t param_size)
  * Copies device info back to user space, used by
  * the create and info ioctls.
  */
-static int dev_status(struct dm_ioctl *param, size_t param_size)
+static int dev_status(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	struct mapped_device *md;
 
@@ -1170,7 +1170,7 @@ static void retrieve_status(struct dm_table *table,
 /*
  * Wait for a device to report an event
  */
-static int dev_wait(struct dm_ioctl *param, size_t param_size)
+static int dev_wait(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r = 0;
 	struct mapped_device *md;
@@ -1276,7 +1276,7 @@ static bool is_valid_type(unsigned cur, unsigned new)
 	return false;
 }
 
-static int table_load(struct dm_ioctl *param, size_t param_size)
+static int table_load(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r;
 	struct hash_cell *hc;
@@ -1363,7 +1363,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
 	return r;
 }
 
-static int table_clear(struct dm_ioctl *param, size_t param_size)
+static int table_clear(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	struct hash_cell *hc;
 	struct mapped_device *md;
@@ -1437,7 +1437,7 @@ static void retrieve_deps(struct dm_table *table,
 	param->data_size = param->data_start + needed;
 }
 
-static int table_deps(struct dm_ioctl *param, size_t param_size)
+static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	struct mapped_device *md;
 	struct dm_table *table;
@@ -1463,7 +1463,7 @@ static int table_deps(struct dm_ioctl *param, size_t param_size)
  * Return the status of a device as a text string for each
  * target.
  */
-static int table_status(struct dm_ioctl *param, size_t param_size)
+static int table_status(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	struct mapped_device *md;
 	struct dm_table *table;
@@ -1518,7 +1518,7 @@ static int message_for_md(struct mapped_device *md, unsigned argc, char **argv,
 /*
  * Pass a message to the target that's at the supplied device offset.
  */
-static int target_message(struct dm_ioctl *param, size_t param_size)
+static int target_message(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
 	int r, argc;
 	char **argv;
@@ -1794,7 +1794,7 @@ static int validate_params(uint cmd, struct dm_ioctl *param)
 	return 0;
 }
 
-static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
+static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *user)
 {
 	int r = 0;
 	int ioctl_flags;
@@ -1848,7 +1848,7 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 		goto out;
 
 	param->data_size = sizeof(*param);
-	r = fn(param, input_param_size);
+	r = fn(file, param, input_param_size);
 
 	if (unlikely(param->flags & DM_BUFFER_FULL_FLAG) &&
 	    unlikely(ioctl_flags & IOCTL_FLAGS_NO_PARAMS))
@@ -1867,7 +1867,7 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 
 static long dm_ctl_ioctl(struct file *file, uint command, ulong u)
 {
-	return (long)ctl_ioctl(command, (struct dm_ioctl __user *)u);
+	return (long)ctl_ioctl(file, command, (struct dm_ioctl __user *)u);
 }
 
 #ifdef CONFIG_COMPAT
-- 
2.9.3

[PATCH 2/3] dm: Add support to poll for dm events

[Andy Grover]

Instead of requiring a thread for each device to sleep in ioctl(DEV_WAIT)
to receive dm events, allow a single thread to:

1) Open /dev/mapper/control multiple times
2) Associate each of these open file descriptors with different DM devices
3) poll() on all of these
4) When an event occurs, use TABLE_STATUS ioctl to get device's state

This requires:

a) Implementing open, release, and poll file operations
b) Defining a per-file context struct called dm_file
c) Adding a new DEV_ASSOC ioctl to associate a file with a single dm device
d) Handle if fd w/association is closed or referenced device is destroyed

Associating a fd with a device for polling limits the use of other
ioctls besides VERSION, using that fd. There is no mechanism to query
associations, userspace must track this itself.

Signed-off-by: Andy Grover <agrover@redhat.com>
---
 drivers/md/dm-core.h          | 10 +++++
 drivers/md/dm-ioctl.c         | 98 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/md/dm.c               | 12 ++++++
 include/uapi/linux/dm-ioctl.h |  6 ++-
 4 files changed, 122 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 40ceba1..8aeb292 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -24,6 +24,12 @@ struct dm_kobject_holder {
 	struct completion completion;
 };
 
+struct dm_file {
+	struct mapped_device *md;
+	uint32_t event_nr;
+	struct list_head assoc_files_node;
+};
+
 /*
  * DM core internal structure that used directly by dm.c and dm-rq.c
  * DM targets must _not_ deference a mapped_device to directly access its members!
@@ -80,6 +86,10 @@ struct mapped_device {
 	struct list_head uevent_list;
 	spinlock_t uevent_lock; /* Protect access to uevent_list */
 
+	/* Track the fds set to track this device */
+	struct mutex assoc_files_list_lock;
+	struct list_head assoc_files_list;
+
 	/* the number of internal suspends */
 	unsigned internal_suspend_count;
 
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 17c9bf9..c5fc53c 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1207,6 +1207,45 @@ static int dev_wait(struct file *filp, struct dm_ioctl *param, size_t param_size
 	return r;
 }
 
+/*
+ * Associates a given open file with a specific mapped device.
+ * This lets the file be used to poll for events on the device.
+ */
+static int dev_assoc_set(struct file *filp, struct dm_ioctl *param, size_t param_size)
+{
+	struct dm_file *priv = filp->private_data;
+	struct mapped_device *md;
+
+	/* Can only associate once */
+	if (priv)
+		return -EINVAL;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&priv->assoc_files_node);
+
+	md = find_device(param);
+	if (!md) {
+		kfree(priv);
+		return -ENXIO;
+	}
+
+	priv->md = md;
+	priv->event_nr = dm_get_event_nr(priv->md);
+
+	mutex_lock(&md->assoc_files_list_lock);
+	list_add_tail(&priv->assoc_files_node, &md->assoc_files_list);
+	mutex_unlock(&md->assoc_files_list_lock);
+
+	filp->private_data = priv;
+
+	/* No dm_put(), we're keeping a reference to the mapped_device */
+
+	return 0;
+}
+
 static inline fmode_t get_mode(struct dm_ioctl *param)
 {
 	fmode_t mode = FMODE_READ | FMODE_WRITE;
@@ -1635,7 +1674,8 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 		{DM_LIST_VERSIONS_CMD, 0, list_versions},
 
 		{DM_TARGET_MSG_CMD, 0, target_message},
-		{DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry}
+		{DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry},
+		{DM_DEV_ASSOC_CMD, IOCTL_FLAGS_NO_PARAMS, dev_assoc_set},
 	};
 
 	if (unlikely(cmd >= ARRAY_SIZE(_ioctls)))
@@ -1828,6 +1868,13 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	if (cmd == DM_VERSION_CMD)
 		return 0;
 
+	/*
+	 * Cannot use files that have been associated for most ioctls.
+	 * See dev_assoc_set().
+	 */
+	if (file->private_data)
+		return -EINVAL;
+
 	fn = lookup_ioctl(cmd, &ioctl_flags);
 	if (!fn) {
 		DMWARN("dm_ctl_ioctl: unknown command 0x%x", command);
@@ -1879,8 +1926,55 @@ static long dm_compat_ctl_ioctl(struct file *file, uint command, ulong u)
 #define dm_compat_ctl_ioctl NULL
 #endif
 
+int dm_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = NULL;
+	return nonseekable_open(inode, filp);
+}
+
+int dm_release(struct inode *inode, struct file *filp)
+{
+	struct dm_file *priv = filp->private_data;
+
+	if (priv->md) {
+		mutex_lock(&priv->md->assoc_files_list_lock);
+		list_del(&priv->assoc_files_node);
+		mutex_unlock(&priv->md->assoc_files_list_lock);
+
+		dm_put(priv->md);
+	}
+
+	kfree(priv);
+
+	return 0;
+}
+
+static unsigned int dm_poll(struct file *filp, poll_table *wait)
+{
+	struct dm_file *priv = filp->private_data;
+	unsigned int mask = 0;
+	uint32_t cur_event;
+
+	/* fd has not been associated with a device with DEV_EVENT_SET */
+	if (!priv->md)
+		return 0;
+
+	poll_wait(filp, &priv->md->eventq, wait);
+
+	cur_event = dm_get_event_nr(priv->md);
+
+	if (priv->event_nr < cur_event) {
+		priv->event_nr = cur_event;
+		mask |= POLLIN;
+	}
+
+	return mask;
+}
+
 static const struct file_operations _ctl_fops = {
-	.open = nonseekable_open,
+	.open    = dm_open,
+	.release = dm_release,
+	.poll    = dm_poll,
 	.unlocked_ioctl	 = dm_ctl_ioctl,
 	.compat_ioctl = dm_compat_ctl_ioctl,
 	.owner	 = THIS_MODULE,
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3086da5..e847094 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1489,6 +1489,8 @@ static struct mapped_device *alloc_dev(int minor)
 	INIT_LIST_HEAD(&md->uevent_list);
 	INIT_LIST_HEAD(&md->table_devices);
 	spin_lock_init(&md->uevent_lock);
+	mutex_init(&md->assoc_files_list_lock);
+	INIT_LIST_HEAD(&md->assoc_files_list);
 
 	md->queue = blk_alloc_queue_node(GFP_KERNEL, numa_node_id);
 	if (!md->queue)
@@ -1878,6 +1880,7 @@ static void __dm_destroy(struct mapped_device *md, bool wait)
 	struct request_queue *q = dm_get_md_queue(md);
 	struct dm_table *map;
 	int srcu_idx;
+	struct dm_file *dm_f, *tmp;
 
 	might_sleep();
 
@@ -1905,6 +1908,15 @@ static void __dm_destroy(struct mapped_device *md, bool wait)
 	dm_put_live_table(md, srcu_idx);
 	mutex_unlock(&md->suspend_lock);
 
+	/* Disassociate event fds linked to this md */
+	mutex_lock(&md->assoc_files_list_lock);
+	list_for_each_entry_safe(dm_f, tmp, &md->assoc_files_list, assoc_files_node) {
+		dm_f->md = NULL;
+		dm_f->event_nr = 0;
+		list_del(&dm_f->assoc_files_node);
+	}
+	mutex_unlock(&md->assoc_files_list_lock);
+
 	/*
 	 * Rare, but there may be I/O requests still going to complete,
 	 * for example.  Wait for all references to disappear.
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 4bf9f1e..b3731b8 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -240,7 +240,8 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
-	DM_DEV_SET_GEOMETRY_CMD
+	DM_DEV_SET_GEOMETRY_CMD,
+	DM_DEV_ASSOC_CMD,
 };
 
 #define DM_IOCTL 0xfd
@@ -255,6 +256,7 @@ enum {
 #define DM_DEV_SUSPEND   _IOWR(DM_IOCTL, DM_DEV_SUSPEND_CMD, struct dm_ioctl)
 #define DM_DEV_STATUS    _IOWR(DM_IOCTL, DM_DEV_STATUS_CMD, struct dm_ioctl)
 #define DM_DEV_WAIT      _IOWR(DM_IOCTL, DM_DEV_WAIT_CMD, struct dm_ioctl)
+#define DM_DEV_ASSOC     _IOWR(DM_IOCTL, DM_DEV_ASSOC_CMD, struct dm_ioctl)
 
 #define DM_TABLE_LOAD    _IOWR(DM_IOCTL, DM_TABLE_LOAD_CMD, struct dm_ioctl)
 #define DM_TABLE_CLEAR   _IOWR(DM_IOCTL, DM_TABLE_CLEAR_CMD, struct dm_ioctl)
@@ -267,7 +269,7 @@ enum {
 #define DM_DEV_SET_GEOMETRY	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	35
+#define DM_VERSION_MINOR	36
 #define DM_VERSION_PATCHLEVEL	0
 #define DM_VERSION_EXTRA	"-ioctl (2016-06-23)"
 
-- 
2.9.3

[PATCH 3/3] dm: Allow associated fd to be used for TABLE_STATUS

[Andy Grover]

Allows TABLE_STATUS to be used without specifying the dm device in the
ioctl, if it has previously been associated with a device.

Signed-off-by: Andy Grover <agrover@redhat.com>
---
 drivers/md/dm-ioctl.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index c5fc53c..c4c47a6 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1504,13 +1504,19 @@ static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_si
  */
 static int table_status(struct file *filp, struct dm_ioctl *param, size_t param_size)
 {
+	struct dm_file *priv = filp->private_data;
 	struct mapped_device *md;
 	struct dm_table *table;
 	int srcu_idx;
 
-	md = find_device(param);
-	if (!md)
-		return -ENXIO;
+	if (priv && priv->md) {
+		md = priv->md;
+		dm_get(md);
+	} else {
+		md = find_device(param);
+		if (!md)
+			return -ENXIO;
+	}
 
 	__dev_status(md, param);
 
@@ -1868,12 +1874,6 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	if (cmd == DM_VERSION_CMD)
 		return 0;
 
-	/*
-	 * Cannot use files that have been associated for most ioctls.
-	 * See dev_assoc_set().
-	 */
-	if (file->private_data)
-		return -EINVAL;
 
 	fn = lookup_ioctl(cmd, &ioctl_flags);
 	if (!fn) {
@@ -1882,6 +1882,13 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	}
 
 	/*
+	 * Cannot use files that have been associated for ioctls other than
+	 * VERSION and TABLE_STATUS.
+	 */
+	if (file->private_data && cmd != DM_TABLE_STATUS_CMD)
+		return -EINVAL;
+
+	/*
 	 * Copy the parameters into kernel space.
 	 */
 	r = copy_params(user, &param_kernel, ioctl_flags, &param, &param_flags);
-- 
2.9.3

Re: [PATCH 2/3] dm: Add support to poll for dm events

[Andy Grover]

Please see below for some bugs I spotted.

The surest way to find bugs is to post patches to a public list :-/

On 01/09/2017 10:20 AM, Andy Grover wrote:
> +int dm_release(struct inode *inode, struct file *filp)

> +	if (priv->md) {

should be "if (priv && priv->md)"

> +static unsigned int dm_poll(struct file *filp, poll_table *wait)

> +	if (!priv->md)

should be "if (!priv || !priv->md)"

Regards -- Andy