On 17 Feb 2017, at 14:30, Trond Myklebust wrote: > On Fri, 2017-02-17 at 14:15 -0500, Benjamin Coddington wrote: >> On 17 Feb 2017, at 14:00, Trond Myklebust wrote: >> >>> On Fri, 2017-02-17 at 13:46 -0500, Benjamin Coddington wrote: >>>> NFS attempts to wait for read and write completion before >>>> unlocking >>>> in >>>> order to ensure that the data returned was protected by the >>>> lock. When >>>> this waiting is interrupted by a signal, the unlock may never be >>>> sent, and >>>> messages similar to the following are seen in the kernel ring >>>> buffer: >>>> >>>> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3: >>>> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 >>>> fl_type=0x0 >>>> fl_pid=20183 >>>> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 >>>> fl_type=0x0 >>>> fl_pid=20185 >>>> >>>> For NFSv3, the missing unlock will cause the server to refuse >>>> conflicting >>>> locks indefinitely. For NFSv4, the leftover lock will be removed >>>> by >>>> the >>>> server after the lease timeout. >>>> >>>> This patch fixes this for NFSv3 by skipping the wait in order to >>>> immediately send the unlock if the FL_CLOSE flag is set when >>>> signaled. For >>>> NFSv4, this approach may cause the server to see the I/O as >>>> arriving >>>> with >>>> an old stateid, so, for the v4 case the fix is different: the >>>> wait on >>>> I/O >>>> completion is moved into nfs4_locku_ops' >>>> rpc_call_prepare(). This >>>> will >>>> cause the sleep to happen in rpciod context, and a signal to the >>>> originally >>>> waiting process will not cause the unlock to be skipped. >>> >>> NACK. I/O waits in rpciod contexts are NOT acceptable. rpciod is >>> part >>> of the memory reclaim chain, so having it sleep on I/O is deadlock >>> prone. >>> >>> Why is there a need to wait for I/O completion in the first place >>> if >>> the user has killed the task that held the lock? 'kill -9' will >>> cause >>> corruption; that's a fact that no amount of paper will cover over. >> >> To avoid an unnecessary recovery situation where the server asks us >> to resend >> I/O due to an invalid stateid. >> > > I agree we shouldn't recover in this situation. It would be better to > jettison the failed write, and invalidate the page. Can we make use of > nfs_wb_page_cancel() together with generic_error_remove_page()? Probably we can piggy-back on NFS_LOCK_LOST, then -EIO would get passed up and the page would make it into generic_error_remove_page(). Any outstanding writes are likely already transmitted or scheduled to be tranmitted by now, and the error recovery path for incorrect stateids doesn't intersect with nfs_wb_page_cancel(), rather it re-schedules the RPC. But, after looking at this further, I'm not sure how much work should be done here. It's a fairly unlikely situation already, and if we assert that a fatal signal means writes don't have to complete at all, I don't see the harm in having them complete outside the lock. Adding extra complexity to bypass recovery for this specific situation would be optimal, but unnecessary. Ben -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
