On 2022/01/10 19:30, Jan Kara wrote: > Eek, I agree that the patch may improve the situation but is the complexity > and ugliness really worth it? If we are clear about Now your patch will fix this lockdep complaint but we still would wait for the write to complete through blk_mq_freeze_queue() (just lockdep is not clever enough to detect this). So IHMO if there was a deadlock before, it will be still there with your changes. in https://lkml.kernel.org/r/20211223134050.GD19129@xxxxxxxxxxxxxx , we can go with the revert approach. I want to call blk_mq_freeze_queue() without disk->open_mutex held. But currently lo_release() is calling blk_mq_freeze_queue() with disk->open_mutex held. My patch is going towards doing locklessly where possible. > I mean using task work in > loop_schedule_rundown() makes a lot of sense because the loop > > while :; do mount -o loop,ro isofs.iso isofs/; umount isofs/; done > > will not fail because of dangling work in the workqueue after umount -> > __loop_clr_fd(). Using task work from lo_release() is for handling close() => umount() sequence. Using task work in lo_open() is for handling close() => open() sequence. The mount in while :; do mount -o loop,ro isofs.iso isofs/; umount isofs/; done fails unless lo_open() waits for __loop_clr_fd() to complete. > But when other processes like systemd-udevd start to mess > with the loop device, then you have no control whether the following mount > will see isofs.iso busy or not - it depends on when systemd-udevd decides > to close the loop device. Right. But regarding that part the main problem is that the script is not checking for errors. What we are expected to do is to restore barrier which existed before commit 322c4293ecc58110 ("loop: make autoclear operation asynchronous"). > What your waiting in lo_open() achieves is only > that if __loop_clr_fd() from systemd-udevd happens to run at the same time > as lo_open() from mount, then we won't see EBUSY. My waiting in lo_open() is to fix a regression that while :; do mount -o loop,ro isofs.iso isofs/; umount isofs/; done started to fail. > But IMO that is not worth > the complexity in lo_open() because if systemd-udevd happens to close the > loop device a millisecond later, you will get EBUSY anyway (and you would > get it even in the past) Or am I missing something? Excuse me, but lo_open() does not return EBUSY? What operation are you talking about? If lo_open() from /bin/mount happened earlier than close() from systemd-udevd happens, __loop_clr_fd() from systemd-udevd does not happen because lo_open() incremented lo->lo_refcnt, and autoclear will not happen until /bin/mount calls close(). If you are talking about close() => umount() sequence, do_umount() can fail with EBUSY if /bin/umount happened earlier than close() from systemd-udevd happens. But that is out of scope for use of task work in lo_open().
