| Message ID | 20200922112126.16919-8-oneukum@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/7] CDC-WDM: fix hangs in flush() in multithreaded cases | expand |
On 2020/09/22 20:21, Oliver Neukum wrote: > There is no need for flush() to be uninterruptible. close(2) > is allowed to return -EINTR. > > 30 seconds is quite long a time to sleep in an uninterruptible state. > Change it to an interruptible sleep. Doesn't this conflict with Making the wait for IO interruptible would not solve the issue. While it would avoid a hang, it would not allow any progress and we would end up with an unclosable fd. in [RFC 2/7] ? I suggested killable version, for giving up upon SIGKILL implies automatically closing fds by terminating that userspace process.
Am Dienstag, den 22.09.2020, 20:38 +0900 schrieb Tetsuo Handa: > On 2020/09/22 20:21, Oliver Neukum wrote: > > There is no need for flush() to be uninterruptible. close(2) > > is allowed to return -EINTR. > > > > 30 seconds is quite long a time to sleep in an uninterruptible state. > > Change it to an interruptible sleep. > > Doesn't this conflict with > > Making the wait for IO interruptible would not solve the > issue. While it would avoid a hang, it would not allow any progress > and we would end up with an unclosable fd. > > in [RFC 2/7] ? I suggested killable version, for giving up upon SIGKILL > implies automatically closing fds by terminating that userspace process. No. That we state in an earlier patch that an issue needs a timeout instead of an interruptible sleep, does not mean that another issue could not be solved by using an interruptible sleep. Regards Oliver
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 6ea03c12380c..b9cca1cb5058 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -622,7 +622,7 @@ static int wdm_flush(struct file *file, fl_owner_t id) struct wdm_device *desc = file->private_data; int rv; - rv = wait_event_timeout(desc->wait, + rv = wait_event_interruptible_timeout(desc->wait, /* * needs both flags. We cannot do with one * because resetting it would cause a race @@ -642,6 +642,8 @@ static int wdm_flush(struct file *file, fl_owner_t id) return -ENODEV; if (!rv) return -EIO; + if (rv < 0) + return -EINTR; spin_lock_irq(&desc->iuspin); rv = desc->werr;
There is no need for flush() to be uninterruptible. close(2) is allowed to return -EINTR. 30 seconds is quite long a time to sleep in an uninterruptible state. Change it to an interruptible sleep. Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/usb/class/cdc-wdm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)