On Tue, Feb 20, 2018 at 1:48 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > On Fri, Feb 16, 2018 at 03:53:05PM -0500, Olga Kornievskaia wrote: >> It seems wrong but I'm not sure exactly why. I thinking what if the >> server receives all state closing operations (delegreturn, unlock, >> close) and the copy is still going. A file is being changed after the >> file is closed seem weird. But as I mentioned, properly working client >> should be unlocking/closing the file until after the copy is done (or >> until offload_cancel is sent after ctrl-c). >> >> Who is responsible for making sure that the file isn't being accessed >> after it's closed? Is it server's or client's? Like client is making >> sure all the writes are done before it does the close. Should it be >> client's responsibility as well to make sure there are no on-going >> copies before doing the close? >> >> Is the question here about just delegations or are you questioning >> whether or not CLOSE/UNLOCK need to be delayed until the copy is >> finished? > > Yeah, I kinda suspect there's a bug in the CLOSE and UNLOCK case too. > > I don't know if the client really causes trouble for anyone but itself > if it allows IO to go on past close, unlock, or delegreturn. > > If it's still going on after another client acquires a conflicting open, > lock, or delegation, that could be a problem: a client that holds a > mandatory lock, or an open with DENY_WRITE, or a delegation, has a right > not to expect the file to change underneath it. We should already be > safe against that in the delegation case thanks to the vfs checks in > fs/locks.c:check_conflicting_open(). > > In the DENY_WRITE case I bet there's still bug. Maybe in the mandatory > lock case, too. But those cases are rare and already have other > problems. > > So, I guess I don't care too much unless someone's seeing another > problem. > > Still, killing off a long-running copy is probably a good precaution? Now that I got back into the code and remembered it again, copy stateid is never associated with a delegation stateid. Copy is associated with the destination file's stateid which is opened for write. Since linux server does not give out write delegations, then the stateid is either an open stateid or lock stateid (in my testing I varied not locking and locking the destination file so to get open stateid as well as lock stateid to be used in the copy). If you want me to keep the code for killing off copy in the delegreturn because in the future there might be write delegations, I can but I recall the rule of thumb is to keep it simple, thus I'm inclined to remove it. -- 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
