| Message ID | CA+1xoqe+T=iUtUeFdeqmRvMZ7TMCgkLs_1QYv-4YNtn-jCoMCQ@mail.gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Tetsuo, I am sorry if my patch is causing you grief! On Fri, Apr 06, 2012 at 04:44:37AM -0700, Tetsuo Handa wrote: > Tetsuo Handa wrote: > > Most suspicious change is net/9p/client.c because it is changing handling of > > ERESTARTSYS case. > > > > --- linux-3.3.1/net/9p/client.c > > +++ linux-next/net/9p/client.c > > @@ -740,10 +740,18 @@ > > c->status = Disconnected; > > goto reterr; > > } > > +again: > > /* Wait for the response */ > > err = wait_event_interruptible(*req->wq, > > req->status >= REQ_STATUS_RCVD); > > > > + if ((err == -ERESTARTSYS) && (c->status == Connected) > > + && (type == P9_TFLUSH)) { > > + sigpending = 1; > > + clear_thread_flag(TIF_SIGPENDING); > > + goto again; > > + } > > + > > I think this loop is bad with regard to response to SIGKILL. > If wait_event_interruptible() was interrupted by SIGKILL, it will > spin until req->status >= REQ_STATUS_RCVD becomes true. > Rather, > > if ((c->status == Connected) && (type == P9_TFLUSH)) > err = wait_event_killable(*req->wq, > req->status >= REQ_STATUS_RCVD); > else > err = wait_event_interruptible(*req->wq, > req->status >= REQ_STATUS_RCVD); > > would be safer. Does that work? What prevents p9_client_rpc() from recursing via p9_client_flush() on receipt of SIGKILL? > > error: > > /* > > * Fid is not valid even after a failed clunk > > + * If interrupted, retry once then give up and > > + * leak fid until umount. > > */ > > - p9_fid_destroy(fid); > > + if (err == -ERESTARTSYS) { > > + if (retries++ == 0) > > + goto again; > > I think it is possible that the process is interrupted again upon retrying. > I suspect the handling of err == -ERESTARTSYS case when retries != 0. > It is returning without calling p9_fid_destroy(), which will be > unexpected behaviour for the various callers. Yes but in the unlikely event that this happens, the effect is a small memory leak for the duration of the mount. On the other hand if the fid is destroyed without successfully informing the server, then subsequent operations that involve new file references will fail when that fid number is reused, and the mount becomes unusable. > > + } else > > + p9_fid_destroy(fid); > > return err; > > } > > EXPORT_SYMBOL(p9_client_clunk); Regards, Jim -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jim Garlick wrote: > > I think this loop is bad with regard to response to SIGKILL. > > If wait_event_interruptible() was interrupted by SIGKILL, it will > > spin until req->status >= REQ_STATUS_RCVD becomes true. > > Rather, > > > > if ((c->status == Connected) && (type == P9_TFLUSH)) > > err = wait_event_killable(*req->wq, > > req->status >= REQ_STATUS_RCVD); > > else > > err = wait_event_interruptible(*req->wq, > > req->status >= REQ_STATUS_RCVD); > > > > would be safer. > > Does that work? What prevents p9_client_rpc() from recursing via > p9_client_flush() on receipt of SIGKILL? Sorry, I'm not a 9p user and I can't test whether that works or not. But at least, continuing the loop even after SIGKILL is not good. If you have to wait for req->status >= REQ_STATUS_RCVD becomes true, can you use a kernel thread that waits req->status >= REQ_STATUS_RCVD to become true and delegate the job of notifying the server from a userspace task to the kernel thread? > Yes but in the unlikely event that this happens, the effect is a small > memory leak for the duration of the mount. On the other hand if the > fid is destroyed without successfully informing the server, then > subsequent operations that involve new file references will fail > when that fid number is reused, and the mount becomes unusable. I don't know whether Sasha's problem is caused by this patch or not. But p9_client_clunk() is called from many functions in fs/9p/ directory. They are assuming that p9_client_clunk() will call p9_fid_destroy() but this patch is breaking that assumption. I think this is the cause of hang which Sasha is experiencing because Sasha's trace shows that call_usermodehelper() is blocked by functions in fs/9p/ directory. Seems inconsistency state problem. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Yes but in the unlikely event that this happens, the effect is a small >> memory leak for the duration of the mount. On the other hand if the >> fid is destroyed without successfully informing the server, then >> subsequent operations that involve new file references will fail >> when that fid number is reused, and the mount becomes unusable. > > I don't know whether Sasha's problem is caused by this patch or not. > But p9_client_clunk() is called from many functions in fs/9p/ directory. > They are assuming that p9_client_clunk() will call p9_fid_destroy() but > this patch is breaking that assumption. I think this is the cause of hang which > Sasha is experiencing because Sasha's trace shows that call_usermodehelper() is > blocked by functions in fs/9p/ directory. Seems inconsistency state problem. I'd be happy to try out any other patches or help debugging this issue. Which behavior did this patch fix exactly? can I just revert it and try running without it? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c index 1a91efa..8ecc377 100644 --- a/lib/kobject_uevent.c +++ b/lib/kobject_uevent.c @@ -309,7 +309,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action, goto exit; retval = call_usermodehelper(argv[0], argv, - env->envp, UMH_WAIT_EXEC); + env->envp, UMH_NO_WAIT); }