[2/2] loop: use interruptible lock in ioctls

Commit Message

Omar Sandoval March 26, 2018, 11:16 p.m. UTC

From: Omar Sandoval <[e-mail]>

Even after the previous patch to drop lo_ctl_mutex while calling
vfs_getattr(), there are other cases where we can end up sleeping for a
long time while holding lo_ctl_mutex. Let's avoid the uninterruptible
sleep from the ioctls.

Signed-off-by: Omar Sandoval <[e-mail]>
---
 drivers/block/loop.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Matthew Wilcox (Oracle) March 27, 2018, 12:04 a.m. UTC | #1

On Mon, Mar 26, 2018 at 04:16:26PM -0700, Omar Sandoval wrote:
> Even after the previous patch to drop lo_ctl_mutex while calling
> vfs_getattr(), there are other cases where we can end up sleeping for a
> long time while holding lo_ctl_mutex. Let's avoid the uninterruptible
> sleep from the ioctls.

Umm ... you want these ioctls to return -EINTR just because you resized
an xterm?  I think you really meant mutex_lock_killable().

Omar Sandoval March 27, 2018, 4:35 a.m. UTC | #2

On Mon, Mar 26, 2018 at 05:04:21PM -0700, Matthew Wilcox wrote:
> On Mon, Mar 26, 2018 at 04:16:26PM -0700, Omar Sandoval wrote:
> > Even after the previous patch to drop lo_ctl_mutex while calling
> > vfs_getattr(), there are other cases where we can end up sleeping for a
> > long time while holding lo_ctl_mutex. Let's avoid the uninterruptible
> > sleep from the ioctls.
> 
> Umm ... you want these ioctls to return -EINTR just because you resized
> an xterm?  I think you really meant mutex_lock_killable().
> 

Thanks, yeah, will fix.

David Sterba March 28, 2018, 1:18 p.m. UTC | #3

On Mon, Mar 26, 2018 at 05:04:21PM -0700, Matthew Wilcox wrote:
> On Mon, Mar 26, 2018 at 04:16:26PM -0700, Omar Sandoval wrote:
> > Even after the previous patch to drop lo_ctl_mutex while calling
> > vfs_getattr(), there are other cases where we can end up sleeping for a
> > long time while holding lo_ctl_mutex. Let's avoid the uninterruptible
> > sleep from the ioctls.
> 
> Umm ... you want these ioctls to return -EINTR just because you resized
> an xterm?  I think you really meant mutex_lock_killable().

Does that really happen? In some cases (namely user called ioctls) I'd
like to allow ctrl-c to interrupt a mutex_lock or other _interruptible
actions. I'd be very surprised if resizing terminal stops the command
I've just started.

Matthew Wilcox (Oracle) March 28, 2018, 4:57 p.m. UTC | #4

On Wed, Mar 28, 2018 at 03:18:52PM +0200, David Sterba wrote:
> On Mon, Mar 26, 2018 at 05:04:21PM -0700, Matthew Wilcox wrote:
> > On Mon, Mar 26, 2018 at 04:16:26PM -0700, Omar Sandoval wrote:
> > > Even after the previous patch to drop lo_ctl_mutex while calling
> > > vfs_getattr(), there are other cases where we can end up sleeping for a
> > > long time while holding lo_ctl_mutex. Let's avoid the uninterruptible
> > > sleep from the ioctls.
> > 
> > Umm ... you want these ioctls to return -EINTR just because you resized
> > an xterm?  I think you really meant mutex_lock_killable().
> 
> Does that really happen? In some cases (namely user called ioctls) I'd
> like to allow ctrl-c to interrupt a mutex_lock or other _interruptible
> actions. I'd be very surprised if resizing terminal stops the command
> I've just started.

Resizing an xterm causes the xterm to send its child SIGWINCH.  ctrl-c
will cause fatal_signal_pending() to become true, and mutex_lock_killable
to return -EINTR.  mutex_lock_interruptible() will return -EINTR for any
signal (SIGPOLL, SIGWINCH, etc).

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 93a60bda7608..28673cfdd3a3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1360,7 +1360,10 @@  static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	struct loop_device *lo = bdev->bd_disk->private_data;
 	int err;
 
-	mutex_lock_nested(&lo->lo_ctl_mutex, 1);
+	err = mutex_lock_interruptible_nested(&lo->lo_ctl_mutex, 1);
+	if (err)
+		goto out_unlocked;
+
 	switch (cmd) {
 	case LOOP_SET_FD:
 		err = loop_set_fd(lo, mode, bdev, arg);
@@ -1545,16 +1548,20 @@  static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 
 	switch(cmd) {
 	case LOOP_SET_STATUS:
-		mutex_lock(&lo->lo_ctl_mutex);
-		err = loop_set_status_compat(
-			lo, (const struct compat_loop_info __user *) arg);
-		mutex_unlock(&lo->lo_ctl_mutex);
+		err = mutex_lock_interruptible(&lo->lo_ctl_mutex);
+		if (!err) {
+			err = loop_set_status_compat(lo,
+						     (const struct compat_loop_info __user *)arg);
+			mutex_unlock(&lo->lo_ctl_mutex);
+		}
 		break;
 	case LOOP_GET_STATUS:
-		mutex_lock(&lo->lo_ctl_mutex);
-		err = loop_get_status_compat(
-			lo, (struct compat_loop_info __user *) arg);
-		/* loop_get_status() unlocks lo_ctl_mutex */
+		err = mutex_lock_interruptible(&lo->lo_ctl_mutex);
+		if (!err) {
+			err = loop_get_status_compat(lo,
+						     (struct compat_loop_info __user *)arg);
+			/* loop_get_status() unlocks lo_ctl_mutex */
+		}
 		break;
 	case LOOP_SET_CAPACITY:
 	case LOOP_CLR_FD:
@@ -1959,7 +1966,9 @@  static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		ret = loop_lookup(&lo, parm);
 		if (ret < 0)
 			break;
-		mutex_lock(&lo->lo_ctl_mutex);
+		ret = mutex_lock_interruptible(&lo->lo_ctl_mutex);
+		if (ret)
+			break;
 		if (lo->lo_state != Lo_unbound) {
 			ret = -EBUSY;
 			mutex_unlock(&lo->lo_ctl_mutex);