On Fri, Jan 7, 2022 at 12:04 PM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Jan Stancek is reporting that commit 322c4293ecc58110 ("loop: make
> autoclear operation asynchronous") broke LTP tests which run /bin/mount
> and /bin/umount in close succession like
>
> while :; do mount -o loop,ro isofs.iso isofs/; umount isofs/; done
>
> . This is because since /usr/lib/systemd/systemd-udevd asynchronously
> opens the loop device which /bin/mount and /bin/umount are operating,
> autoclear from lo_release() is likely triggered by systemd-udevd than
> mount or umount. And unfortunately, /bin/mount fails if read of superblock
> (for examining filesystem type) returned an error due to the backing file
> being cleared by __loop_clr_fd(). It turned out that disk->open_mutex was
> by chance serving as a barrier for serializing "__loop_clr_fd() from
> lo_release()" and "vfs_read() after lo_open()", and we need to restore
> this barrier (without reintroducing circular locking dependency).
>
> Also, the kernel test robot is reporting that that commit broke xfstest
> which does
>
> umount ext2 on xfs
> umount xfs
>
> sequence.
>
> One of approaches for fixing these problems is to revert that commit and
> instead remove destroy_workqueue() from __loop_clr_fd(), for it turned out
> that we did not need to call flush_workqueue() from __loop_clr_fd() since
> blk_mq_freeze_queue() blocks until all pending "struct work_struct" are
> processed by loop_process_work(). But we are not sure whether it is safe
> to wait blk_mq_freeze_queue() etc. with disk->open_mutex held; it could
> be simply because dependency is not clear enough for fuzzers to cover and
> lockdep to detect.
>
> Therefore, before taking revert approach, let's try if we can use task
> work approach which is called without locks held while the caller can
> wait for completion of that task work before returning to user mode.
>
> This patch tries to make lo_open()/lo_release() to locklessly wait for
> __loop_clr_fd() by inserting a task work into lo_open()/lo_release() if
> possible.
>
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Reported-by: Jan Stancek <jstancek@xxxxxxxxxx>
v2 looked OK in my tests, thanks.
Tested-by: Jan Stancek <jstancek@xxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> Need to also wait on lo_open(), per Jan's testcase.
>
> drivers/block/loop.c | 70 ++++++++++++++++++++++++++++++++++++++++----
> drivers/block/loop.h | 5 +++-
> 2 files changed, 68 insertions(+), 7 deletions(-)
>
