scst_user: avoid global locks

git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6221 d57e44dd-8a1f-0410-8b47-8ef2f437770f

doc/scst_user_spec.sgml

@@ -151,16 +151,8 @@ function, or close on its file descriptor.
 <sect1> SCST_USER_UNREGISTER_DEVICE
 
 <p>
-SCST_USER_UNREGISTER_DEVICE unregisters the corresponding virtual user
+SCST_USER_UNREGISTER_DEVICE is obsolete and should not be used. Just
-space device. It doesn't have any parameters.
+close the device's fd instead.
-
-During execution of SCST_USER_UNREGISTER_DEVICE at least one another
-thread must process all coming subcommands, otherwise after timeout it
-will fail with EBUSY error. The processing should stop on receiving
-ENODEV error.
-
-SCST_USER_UNREGISTER_DEVICE returns 0 on success or -1 in case of error,
-and errno is set appropriately.
 
 
 <sect1> SCST_USER_SET_OPTIONS/SCST_USER_GET_OPTIONS
@@ -290,6 +282,10 @@ thread, which also handles incoming commands, otherwise there could be a
 but nobody handles them. This "deadlock" will be resolved only when
 initiator, which sent those commands, aborts them after timeout.
 
+<bf/IMPORTANT!/ It is duty of the caller to serialize
+SCST_USER_SET_OPTIONS invocations. The kernel code does not have any
+locking around modifying above properties.
+
 
 <sect1> SCST_USER_REPLY_AND_GET_CMD
 

scst/src/dev_handlers/scst_user.c

@@ -47,8 +47,6 @@
 #define DEV_USER_ATTACH_TIMEOUT		(5*HZ)
 
 struct scst_user_dev {
-	struct rw_semaphore dev_rwsem;
-
 	/*
 	 * Must be kept here, because it's needed on the cleanup time,
 	 * when corresponding scst_dev is already dead.
@@ -58,7 +56,10 @@ struct scst_user_dev {
 	/* Protected by udev_cmd_threads.cmd_list_lock */
 	struct list_head ready_cmd_list;
 
-	/* Protected by dev_rwsem or don't need any protection */
+	/*
+	 * Don't need any protection or assignment in SCST_USER_SET_OPTIONS
+	 * supposed to be serialized by the caller
+	 */
 	unsigned int blocking:1;
 	unsigned int cleanup_done:1;
 	unsigned int tst:3;
@@ -218,8 +219,6 @@ static struct kmem_cache *user_dev_cachep;
 static struct kmem_cache *user_cmd_cachep;
 static struct kmem_cache *user_get_cmd_cachep;
 
-static DEFINE_MUTEX(dev_priv_mutex);
-
 static const struct file_operations dev_user_fops = {
 	.poll		= dev_user_poll,
 	.unlocked_ioctl	= dev_user_ioctl,
@@ -1707,21 +1706,16 @@ static int dev_user_reply_cmd(struct file *file, void __user *arg)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (unlikely(res != 0)) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	rc = copy_from_user(&reply, arg, sizeof(reply));
 	if (unlikely(rc != 0)) {
 		PRINT_ERROR("Failed to copy %d user's bytes", rc);
 		res = -EFAULT;
-		goto out_up;
+		goto out;
 	}
 
 	TRACE_DBG("Reply for dev %s", dev->name);
@@ -1730,10 +1724,7 @@ static int dev_user_reply_cmd(struct file *file, void __user *arg)
 
 	res = dev_user_process_reply(dev, &reply);
 	if (unlikely(res < 0))
-		goto out_up;
+		goto out;
-
-out_up:
-	up_read(&dev->dev_rwsem);
 
 out:
 	TRACE_EXIT_RES(res);
@@ -1750,21 +1741,16 @@ static int dev_user_get_ext_cdb(struct file *file, void __user *arg)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (unlikely(res != 0)) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	rc = copy_from_user(&get, arg, sizeof(get));
 	if (unlikely(rc != 0)) {
 		PRINT_ERROR("Failed to copy %d user's bytes", rc);
 		res = -EFAULT;
-		goto out_up;
+		goto out;
 	}
 
 	TRACE_MGMT_DBG("Get ext cdb for dev %s", dev->name);
@@ -1826,16 +1812,13 @@ out_cmd_put:
 out_put:
 	ucmd_put(ucmd);
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
 
 out_unlock:
 	spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock);
-	goto out_up;
+	goto out;
 }
 
 static int dev_user_process_scst_commands(struct scst_user_dev *dev)
@@ -1979,15 +1962,10 @@ static int dev_user_reply_get_cmd(struct file *file, void __user *arg)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (unlikely(res != 0)) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	/* get_user() can't be used with 64-bit values on x86_32 */
 	rc = copy_from_user(&ureply, (uint64_t __user *)
@@ -1996,7 +1974,7 @@ static int dev_user_reply_get_cmd(struct file *file, void __user *arg)
 	if (unlikely(rc != 0)) {
 		PRINT_ERROR("Failed to copy %d user's bytes", rc);
 		res = -EFAULT;
-		goto out_up;
+		goto out;
 	}
 
 	TRACE_DBG("ureply %lld (dev %s)", (unsigned long long int)ureply,
@@ -2005,7 +1983,7 @@ static int dev_user_reply_get_cmd(struct file *file, void __user *arg)
 	cmd = kmem_cache_alloc(user_get_cmd_cachep, GFP_KERNEL);
 	if (unlikely(cmd == NULL)) {
 		res = -ENOMEM;
-		goto out_up;
+		goto out;
 	}
 
 	if (ureply != 0) {
@@ -2069,16 +2047,13 @@ again:
 	} else
 		spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock);
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
 
 out_free:
 	kmem_cache_free(user_get_cmd_cachep, cmd);
-	goto out_up;
+	goto out;
 }
 
 static long dev_user_ioctl(struct file *file, unsigned int cmd,
@@ -2187,15 +2162,10 @@ static unsigned int dev_user_poll(struct file *file, poll_table *wait)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (unlikely(res != 0)) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	spin_lock_irq(&dev->udev_cmd_threads.cmd_list_lock);
 
@@ -2222,8 +2192,6 @@ static unsigned int dev_user_poll(struct file *file, poll_table *wait)
 out_unlock:
 	spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock);
 
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_HRES(res);
 	return res;
@@ -2977,7 +2945,6 @@ static int dev_user_register_dev(struct file *file,
 		goto out_put;
 	}
 
-	init_rwsem(&dev->dev_rwsem);
 	INIT_LIST_HEAD(&dev->ready_cmd_list);
 	if (file->f_flags & O_NONBLOCK) {
 		TRACE_DBG("%s", "Non-blocking operations");
@@ -3086,15 +3053,19 @@ static int dev_user_register_dev(struct file *file,
 		goto out_unreg_handler;
 	}
 
-	mutex_lock(&dev_priv_mutex);
+	spin_lock(&dev_list_lock);
 	if (file->private_data != NULL) {
-		mutex_unlock(&dev_priv_mutex);
+		spin_unlock(&dev_list_lock);
 		PRINT_ERROR("%s", "Device already registered");
 		res = -EINVAL;
 		goto out_unreg_drv;
 	}
+	/*
+	 * Assumption here is that the private_data reading is atomic,
+	 * hence could be lockless and without ACCESS_ONCE().
+	 */
 	file->private_data = dev;
-	mutex_unlock(&dev_priv_mutex);
+	spin_unlock(&dev_list_lock);
 
 out:
 	TRACE_EXIT_RES(res);
@@ -3129,57 +3100,9 @@ out_put:
 
 static int dev_user_unregister_dev(struct file *file)
 {
-	int res;
+	PRINT_WARNING("SCST_USER_UNREGISTER_DEVICE is obsolete and NOOP. "
-	struct scst_user_dev *dev;
+		"Closing fd should be used instead.");
-
+	return 0;
-	TRACE_ENTRY();
-
-	mutex_lock(&dev_priv_mutex);
-	dev = file->private_data;
-	res = dev_user_check_reg(dev);
-	if (res != 0) {
-		mutex_unlock(&dev_priv_mutex);
-		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
-
-	res = scst_suspend_activity(SCST_SUSPEND_TIMEOUT_USER);
-	if (res != 0)
-		goto out_up;
-
-	up_read(&dev->dev_rwsem);
-
-	mutex_lock(&dev_priv_mutex);
-	dev = file->private_data;
-	if (dev == NULL) {
-		mutex_unlock(&dev_priv_mutex);
-		goto out_resume;
-	}
-
-	dev->blocking = 0;
-	wake_up_all(&dev->udev_cmd_threads.cmd_list_waitQ);
-
-	down_write(&dev->dev_rwsem);
-	file->private_data = NULL;
-	mutex_unlock(&dev_priv_mutex);
-
-	dev_user_exit_dev(dev);
-
-	up_write(&dev->dev_rwsem); /* to make lockdep happy */
-
-	kmem_cache_free(user_dev_cachep, dev);
-
-out_resume:
-	scst_resume_activity();
-
-out:
-	TRACE_EXIT_RES(res);
-	return res;
-
-out_up:
-	up_read(&dev->dev_rwsem);
-	goto out;
 }
 
 static int dev_user_flush_cache(struct file *file)
@@ -3189,28 +3112,20 @@ static int dev_user_flush_cache(struct file *file)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (res != 0) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	res = scst_suspend_activity(SCST_SUSPEND_TIMEOUT_USER);
 	if (res != 0)
-		goto out_up;
+		goto out;
 
 	sgv_pool_flush(dev->pool);
 	sgv_pool_flush(dev->pool_clust);
 
 	scst_resume_activity();
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
@@ -3223,20 +3138,13 @@ static int dev_user_capacity_changed(struct file *file)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (res != 0) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	scst_capacity_data_changed(dev->sdev);
 
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
@@ -3256,21 +3164,16 @@ static int dev_user_prealloc_buffer(struct file *file, void __user *arg)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (unlikely(res != 0)) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	rc = copy_from_user(&pre.in, arg, sizeof(pre.in));
 	if (unlikely(rc != 0)) {
 		PRINT_ERROR("Failed to copy %d user's bytes", rc);
 		res = -EFAULT;
-		goto out_up;
+		goto out;
 	}
 
 	TRACE_MEM("Prealloc buffer with size %dKB for dev %s",
@@ -3283,7 +3186,7 @@ static int dev_user_prealloc_buffer(struct file *file, void __user *arg)
 	ucmd = dev_user_alloc_ucmd(dev, GFP_KERNEL);
 	if (ucmd == NULL) {
 		res = -ENOMEM;
-		goto out_up;
+		goto out;
 	}
 
 	ucmd->buff_cached = 1;
@@ -3336,9 +3239,6 @@ static int dev_user_prealloc_buffer(struct file *file, void __user *arg)
 out_put:
 	ucmd_put(ucmd);
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
@@ -3433,27 +3333,19 @@ static int dev_user_set_opt(struct file *file, const struct scst_user_opt *opt)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (res != 0) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	res = scst_suspend_activity(SCST_SUSPEND_TIMEOUT_USER);
 	if (res != 0)
-		goto out_up;
+		goto out;
 
 	res = __dev_user_set_opt(dev, opt);
 
 	scst_resume_activity();
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
@@ -3467,15 +3359,10 @@ static int dev_user_get_opt(struct file *file, void __user *arg)
 
 	TRACE_ENTRY();
 
-	mutex_lock(&dev_priv_mutex);
 	dev = file->private_data;
 	res = dev_user_check_reg(dev);
-	if (res != 0) {
+	if (unlikely(res != 0))
-		mutex_unlock(&dev_priv_mutex);
 		goto out;
-	}
-	down_read(&dev->dev_rwsem);
-	mutex_unlock(&dev_priv_mutex);
 
 	opt.parse_type = dev->parse_type;
 	opt.on_free_cmd_type = dev->on_free_cmd_type;
@@ -3501,12 +3388,9 @@ static int dev_user_get_opt(struct file *file, void __user *arg)
 	if (unlikely(rc != 0)) {
 		PRINT_ERROR("Failed to copy to user %d bytes", rc);
 		res = -EFAULT;
-		goto out_up;
+		goto out;
 	}
 
-out_up:
-	up_read(&dev->dev_rwsem);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;

usr/fileio/fileio.c

@@ -476,6 +476,7 @@ out_unreg:
 	alarm(0);
 	for (i = 0; i < num_devs; i++) {
 		if (unreg_before_close) {
+			/* Just to see the obsolete call message */
 			res = ioctl(devs[i].scst_usr_fd, SCST_USER_UNREGISTER_DEVICE, NULL);
 			if (res != 0) {
 				res = errno;