[PATCH 0/2] loop: don't hang on lo_ctl_mutex in ioctls

[PATCH 0/2] loop: don't hang on lo_ctl_mutex in ioctls

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi, Jens,

We hit an issue where a loop device on NFS (yes, I know) got stuck and a
bunch of losetup processes got stuck in uninterruptible sleep waiting
for lo_ctl_mutex as a result. Calling into the filesystem while holding
lo_ctl_mutex isn't necessary, and there's no reason to be
uninterruptible here. These two patches fix this issue, based on your
for-next branch.

Thanks!

Omar Sandoval (2):
  loop: don't call into filesystem while holding lo_ctl_mutex
  loop: use interruptible lock in ioctls

 drivers/block/loop.c | 65 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 23 deletions(-)

-- 
2.16.2

[PATCH 1/2] loop: don't call into filesystem while holding lo_ctl_mutex

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

We hit an issue where a loop device on NFS was stuck in
loop_get_status() doing vfs_getattr() after the NFS server died, which
caused a pile-up of uninterruptible processes waiting on lo_ctl_mutex.
There's no reason to hold this lock while we wait on the filesystem;
let's drop it so that other processes can do their thing. We need to
grab a reference on lo_backing_file while we use it, and we can get rid
of the check on lo_device, which has been unnecessary since commit
a34c0ae9ebd6 ("[PATCH] loop: remove the bio remapping capability") in
the linux-history tree.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ed6fafdc5377..93a60bda7608 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1167,21 +1167,17 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 static int
 loop_get_status(struct loop_device *lo, struct loop_info64 *info)
 {
-	struct file *file = lo->lo_backing_file;
+	struct file *file;
 	struct kstat stat;
-	int error;
+	int ret;
 
-	if (lo->lo_state != Lo_bound)
+	if (lo->lo_state != Lo_bound) {
+		mutex_unlock(&lo->lo_ctl_mutex);
 		return -ENXIO;
-	error = vfs_getattr(&file->f_path, &stat,
-			    STATX_INO, AT_STATX_SYNC_AS_STAT);
-	if (error)
-		return error;
+	}
+
 	memset(info, 0, sizeof(*info));
 	info->lo_number = lo->lo_number;
-	info->lo_device = huge_encode_dev(stat.dev);
-	info->lo_inode = stat.ino;
-	info->lo_rdevice = huge_encode_dev(lo->lo_device ? stat.rdev : stat.dev);
 	info->lo_offset = lo->lo_offset;
 	info->lo_sizelimit = lo->lo_sizelimit;
 	info->lo_flags = lo->lo_flags;
@@ -1194,7 +1190,19 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info)
 		memcpy(info->lo_encrypt_key, lo->lo_encrypt_key,
 		       lo->lo_encrypt_key_size);
 	}
-	return 0;
+
+	/* Drop lo_ctl_mutex while we call into the filesystem. */
+	file = get_file(lo->lo_backing_file);
+	mutex_unlock(&lo->lo_ctl_mutex);
+	ret = vfs_getattr(&file->f_path, &stat, STATX_INO,
+			  AT_STATX_SYNC_AS_STAT);
+	if (!ret) {
+		info->lo_device = huge_encode_dev(stat.dev);
+		info->lo_inode = stat.ino;
+		info->lo_rdevice = huge_encode_dev(stat.rdev);
+	}
+	fput(file);
+	return ret;
 }
 
 static void
@@ -1374,7 +1382,8 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS:
 		err = loop_get_status_old(lo, (struct loop_info __user *) arg);
-		break;
+		/* loop_get_status() unlocks lo_ctl_mutex */
+		goto out_unlocked;
 	case LOOP_SET_STATUS64:
 		err = -EPERM;
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
@@ -1383,7 +1392,8 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_GET_STATUS64:
 		err = loop_get_status64(lo, (struct loop_info64 __user *) arg);
-		break;
+		/* loop_get_status() unlocks lo_ctl_mutex */
+		goto out_unlocked;
 	case LOOP_SET_CAPACITY:
 		err = -EPERM;
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
@@ -1544,7 +1554,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 		mutex_lock(&lo->lo_ctl_mutex);
 		err = loop_get_status_compat(
 			lo, (struct compat_loop_info __user *) arg);
-		mutex_unlock(&lo->lo_ctl_mutex);
+		/* loop_get_status() unlocks lo_ctl_mutex */
 		break;
 	case LOOP_SET_CAPACITY:
 	case LOOP_CLR_FD:
-- 
2.16.2

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

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <osandov@fb.com>
---
 drivers/block/loop.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

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);
-- 
2.16.2

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

[Matthew Wilcox]

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().

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

[Omar Sandoval]

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.

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

[David Sterba]

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.

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

[Matthew Wilcox]

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).

Re: [PATCH 0/2] loop: don't hang on lo_ctl_mutex in ioctls

[Jens Axboe]

On 3/26/18 10:39 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi, Jens,
> 
> We hit an issue where a loop device on NFS (yes, I know) got stuck and a
> bunch of losetup processes got stuck in uninterruptible sleep waiting
> for lo_ctl_mutex as a result. Calling into the filesystem while holding
> lo_ctl_mutex isn't necessary, and there's no reason to be
> uninterruptible here. These two patches fix this issue, based on your
> for-next branch.

Applied for 4.17, thanks Omar.

-- 
Jens Axboe

[PATCH 0/2] loop: don't hang on lo_ctl_mutex in ioctls

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi, Jens,

We hit an issue where a loop device on NFS (yes, I know) got stuck and a
bunch of losetup processes got stuck in uninterruptible sleep waiting
for lo_ctl_mutex as a result. Calling into the filesystem while holding
lo_ctl_mutex isn't necessary, and there's no reason to be
uninterruptible here. These two patches fix this issue, based on your
for-next branch.

Changed from v1: use mutex_lock_killable() instead of
mutex_lock_interruptible().

Thanks!

Omar Sandoval (2):
  loop: don't call into filesystem while holding lo_ctl_mutex
  loop: use killable lock in ioctls

 drivers/block/loop.c | 65 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 23 deletions(-)

-- 
2.16.2