All of lore.kernel.org
 help / color / mirror / Atom feed
* loop: make autoclear operation synchronous again
@ 2022-01-29  7:14 Tetsuo Handa
  2022-01-29  7:14 ` [PATCH 1/7] loop: revert "make autoclear operation asynchronous" Tetsuo Handa
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Tetsuo Handa @ 2022-01-29  7:14 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig, Jan Kara, Ming Lei; +Cc: linux-block

This is based on ideas from https://lkml.kernel.org/r/20220128130022.1750906-1-hch@lst.de
but also kills "disk->open_mutex => lo->lo_mutex" dependency.

Since __loop_clr_fd() runs synchronously from lo_release(),
the reported regressions should be fixed.

We can apply Christoph's

  [PATCH 1/8] loop: de-duplicate the idle worker freeing code
  [PATCH 2/8] loop: initialize the worker tracking fields once
  [PATCH 3/8] loop: remove the racy bd_inode->i_mapping->nrpages asserts

patches after this series.

I'm surprised with a lot of "if (!release)" usage in __loop_clr_fd() needed
for avoid waiting for I/O request. By the way, does bdev_disk_changed() from
__loop_clr_fd() involve I/O request which we are trying to avoid?

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH for 5.17] loop: revert "make autoclear operation asynchronous"
@ 2022-02-11  7:15 Tetsuo Handa
  2022-02-11 12:52 ` Jens Axboe
  0 siblings, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2022-02-11  7:15 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Tetsuo Handa, Christoph Hellwig, Jan Kara,
	kernel test robot

The kernel test robot is reporting that xfstest which does

  umount ext2 on xfs
  umount xfs

sequence started failing, for commit 322c4293ecc58110 ("loop: make
autoclear operation asynchronous") removed a guarantee that fput() of
backing file is processed before lo_release() from close() returns to
user mode.

And syzbot is reporting that deferring destroy_workqueue() from
__loop_clr_fd() to a WQ context did not help [1]. Revert that commit.

Link: https://syzkaller.appspot.com/bug?extid=831661966588c802aae9 [1]
Reported-by: kernel test robot <oliver.sang@intel.com>
Acked-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reported-by: syzbot <syzbot+831661966588c802aae9@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/block/loop.c | 65 ++++++++++++++++++++------------------------
 drivers/block/loop.h |  1 -
 2 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 01cbbfc4e9e2..150012ffb387 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1082,7 +1082,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	return error;
 }
 
-static void __loop_clr_fd(struct loop_device *lo)
+static void __loop_clr_fd(struct loop_device *lo, bool release)
 {
 	struct file *filp;
 	gfp_t gfp = lo->old_gfp_mask;
@@ -1144,6 +1144,8 @@ static void __loop_clr_fd(struct loop_device *lo)
 	/* let user-space know about this change */
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
+	/* This is safe: open() is still holding a reference. */
+	module_put(THIS_MODULE);
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
 	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
@@ -1151,52 +1153,44 @@ static void __loop_clr_fd(struct loop_device *lo)
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN) {
 		int err;
 
-		mutex_lock(&lo->lo_disk->open_mutex);
+		/*
+		 * open_mutex has been held already in release path, so don't
+		 * acquire it if this function is called in such case.
+		 *
+		 * If the reread partition isn't from release path, lo_refcnt
+		 * must be at least one and it can only become zero when the
+		 * current holder is released.
+		 */
+		if (!release)
+			mutex_lock(&lo->lo_disk->open_mutex);
 		err = bdev_disk_changed(lo->lo_disk, false);
-		mutex_unlock(&lo->lo_disk->open_mutex);
+		if (!release)
+			mutex_unlock(&lo->lo_disk->open_mutex);
 		if (err)
 			pr_warn("%s: partition scan of loop%d failed (rc=%d)\n",
 				__func__, lo->lo_number, err);
 		/* Device is gone, no point in returning error */
 	}
 
+	/*
+	 * lo->lo_state is set to Lo_unbound here after above partscan has
+	 * finished. There cannot be anybody else entering __loop_clr_fd() as
+	 * Lo_rundown state protects us from all the other places trying to
+	 * change the 'lo' device.
+	 */
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART;
-
-	fput(filp);
-}
-
-static void loop_rundown_completed(struct loop_device *lo)
-{
 	mutex_lock(&lo->lo_mutex);
 	lo->lo_state = Lo_unbound;
 	mutex_unlock(&lo->lo_mutex);
-	module_put(THIS_MODULE);
-}
-
-static void loop_rundown_workfn(struct work_struct *work)
-{
-	struct loop_device *lo = container_of(work, struct loop_device,
-					      rundown_work);
-	struct block_device *bdev = lo->lo_device;
-	struct gendisk *disk = lo->lo_disk;
-
-	__loop_clr_fd(lo);
-	kobject_put(&bdev->bd_device.kobj);
-	module_put(disk->fops->owner);
-	loop_rundown_completed(lo);
-}
 
-static void loop_schedule_rundown(struct loop_device *lo)
-{
-	struct block_device *bdev = lo->lo_device;
-	struct gendisk *disk = lo->lo_disk;
-
-	__module_get(disk->fops->owner);
-	kobject_get(&bdev->bd_device.kobj);
-	INIT_WORK(&lo->rundown_work, loop_rundown_workfn);
-	queue_work(system_long_wq, &lo->rundown_work);
+	/*
+	 * Need not hold lo_mutex to fput backing file. Calling fput holding
+	 * lo_mutex triggers a circular lock dependency possibility warning as
+	 * fput can take open_mutex which is usually taken before lo_mutex.
+	 */
+	fput(filp);
 }
 
 static int loop_clr_fd(struct loop_device *lo)
@@ -1228,8 +1222,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_state = Lo_rundown;
 	mutex_unlock(&lo->lo_mutex);
 
-	__loop_clr_fd(lo);
-	loop_rundown_completed(lo);
+	__loop_clr_fd(lo, false);
 	return 0;
 }
 
@@ -1754,7 +1747,7 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
 		 */
-		loop_schedule_rundown(lo);
+		__loop_clr_fd(lo, true);
 		return;
 	} else if (lo->lo_state == Lo_bound) {
 		/*
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 918a7a2dc025..082d4b6bfc6a 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -56,7 +56,6 @@ struct loop_device {
 	struct gendisk		*lo_disk;
 	struct mutex		lo_mutex;
 	bool			idr_visible;
-	struct work_struct      rundown_work;
 };
 
 struct loop_cmd {
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-11 12:52 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-29  7:14 loop: make autoclear operation synchronous again Tetsuo Handa
2022-01-29  7:14 ` [PATCH 1/7] loop: revert "make autoclear operation asynchronous" Tetsuo Handa
2022-02-08 14:47   ` [PATCH for 5.17] " Tetsuo Handa
2022-02-09  8:16     ` Christoph Hellwig
2022-01-29  7:14 ` [PATCH 2/7] loop: clarify __module_get()/module_put() usage Tetsuo Handa
2022-01-29  7:14 ` [PATCH 3/7] loop: don't hold lo->lo_mutex from __loop_clr_fd() Tetsuo Handa
2022-01-29  7:14 ` [PATCH 4/7] loop: don't call blk_mq_freeze_queue()/blk_mq_unfreeze_queue() from lo_release() Tetsuo Handa
2022-01-29  7:14 ` [PATCH 5/7] loop: don't call destroy_workqueue() " Tetsuo Handa
2022-01-29  7:14 ` [PATCH 6/7] loop: don't hold lo->lo_mutex from lo_open()/lo_release() Tetsuo Handa
2022-01-29  7:15 ` [PATCH 7/7] loop: use WQ_MEM_RECLAIM flag Tetsuo Handa
  -- strict thread matches above, loose matches on Subject: below --
2022-02-11  7:15 [PATCH for 5.17] loop: revert "make autoclear operation asynchronous" Tetsuo Handa
2022-02-11 12:52 ` Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.