| Message ID | 20250213161555.4914-1-cel@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RFC] NFS: CB_OFFLOAD should return DELAY when no copy state ID matches | expand |
On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > The NFSv4.2 protocol requires that a client match a CB_OFFLOAD > callback to a COPY reply containing the same copy state ID. However, > it's possible that the order of the callback and reply processing on > the client can cause the CB_OFFLOAD to be received and processed > /before/ the client has dealt with the COPY reply. > > Currently, in this case, the Linux NFS client will queue a fresh > struct nfs4_copy_state in the CB_OFFLOAD handler. > handle_async_copy() then checks for a matching nfs4_copy_state > before settling down to wait for a CB_OFFLOAD reply. > > But it would be simpler for the client's callback service to respond > to such a CB_OFFLOAD with "I'm not ready yet" and have the server > send the CB_OFFLOAD again later. This avoids the need for the > client's CB_OFFLOAD processing to allocate an extra struct > nfs4_copy_state -- in most cases that allocation will be tossed > immediately, and it's one less memory allocation that we have to > worry about accidentally leaking or accumulating over time. Why can't the server just fill an appropriate entry for csa_referring_call_lists<> in the CB_SEQUENCE operation for the CB_OFFLOAD callback? That's the mechanism that is intended to be used to avoid the above kind of race.
On 2/13/25 11:54 AM, Trond Myklebust wrote: > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> >> The NFSv4.2 protocol requires that a client match a CB_OFFLOAD >> callback to a COPY reply containing the same copy state ID. However, >> it's possible that the order of the callback and reply processing on >> the client can cause the CB_OFFLOAD to be received and processed >> /before/ the client has dealt with the COPY reply. >> >> Currently, in this case, the Linux NFS client will queue a fresh >> struct nfs4_copy_state in the CB_OFFLOAD handler. >> handle_async_copy() then checks for a matching nfs4_copy_state >> before settling down to wait for a CB_OFFLOAD reply. >> >> But it would be simpler for the client's callback service to respond >> to such a CB_OFFLOAD with "I'm not ready yet" and have the server >> send the CB_OFFLOAD again later. This avoids the need for the >> client's CB_OFFLOAD processing to allocate an extra struct >> nfs4_copy_state -- in most cases that allocation will be tossed >> immediately, and it's one less memory allocation that we have to >> worry about accidentally leaking or accumulating over time. > > Why can't the server just fill an appropriate entry for > csa_referring_call_lists<> in the CB_SEQUENCE operation for the > CB_OFFLOAD callback? That's the mechanism that is intended to be used > to avoid the above kind of race. Intriguing suggestion. It would be helpful if that were called out in RFC 7862. Should support for referring call lists be a requirement, then, for async COPY offload? I don't see a normative mandatory-to-implement statement for rcl's in RFC 8881, if that matters. Practically speaking, though, NFSD callback does not (yet) support referring call lists. It's been left as an exercise for some time. We simply haven't had a strong driver for it. Maybe we do now.
On Thu, 2025-02-13 at 12:06 -0500, Chuck Lever wrote: > On 2/13/25 11:54 AM, Trond Myklebust wrote: > > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > The NFSv4.2 protocol requires that a client match a CB_OFFLOAD > > > callback to a COPY reply containing the same copy state ID. > > > However, > > > it's possible that the order of the callback and reply processing > > > on > > > the client can cause the CB_OFFLOAD to be received and processed > > > /before/ the client has dealt with the COPY reply. > > > > > > Currently, in this case, the Linux NFS client will queue a fresh > > > struct nfs4_copy_state in the CB_OFFLOAD handler. > > > handle_async_copy() then checks for a matching nfs4_copy_state > > > before settling down to wait for a CB_OFFLOAD reply. > > > > > > But it would be simpler for the client's callback service to > > > respond > > > to such a CB_OFFLOAD with "I'm not ready yet" and have the server > > > send the CB_OFFLOAD again later. This avoids the need for the > > > client's CB_OFFLOAD processing to allocate an extra struct > > > nfs4_copy_state -- in most cases that allocation will be tossed > > > immediately, and it's one less memory allocation that we have to > > > worry about accidentally leaking or accumulating over time. > > > > Why can't the server just fill an appropriate entry for > > csa_referring_call_lists<> in the CB_SEQUENCE operation for the > > CB_OFFLOAD callback? That's the mechanism that is intended to be > > used > > to avoid the above kind of race. > > Intriguing suggestion. > > It would be helpful if that were called out in RFC 7862. Should > support > for referring call lists be a requirement, then, for async COPY > offload? > I don't see a normative mandatory-to-implement statement for rcl's in > RFC 8881, if that matters. No, but in practice it is impossible to resolve several types of races without it. Particularly for delegations. > Practically speaking, though, NFSD callback does not (yet) support > referring call lists. It's been left as an exercise for some time. We > simply haven't had a strong driver for it. Maybe we do now. > > It is also needed for the same kind of race with delegation recalls, layout recalls, CB_NOTIFY_DEVICEID and would also be helpful (although not as strongly required) for CB_NOTIFY_LOCK. IOW: there should be several other incentives for wanting to implement it in knfsd.
On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > > > The NFSv4.2 protocol requires that a client match a CB_OFFLOAD > > callback to a COPY reply containing the same copy state ID. However, > > it's possible that the order of the callback and reply processing on > > the client can cause the CB_OFFLOAD to be received and processed > > /before/ the client has dealt with the COPY reply. > > > > Currently, in this case, the Linux NFS client will queue a fresh > > struct nfs4_copy_state in the CB_OFFLOAD handler. > > handle_async_copy() then checks for a matching nfs4_copy_state > > before settling down to wait for a CB_OFFLOAD reply. > > > > But it would be simpler for the client's callback service to respond > > to such a CB_OFFLOAD with "I'm not ready yet" and have the server > > send the CB_OFFLOAD again later. This avoids the need for the > > client's CB_OFFLOAD processing to allocate an extra struct > > nfs4_copy_state -- in most cases that allocation will be tossed > > immediately, and it's one less memory allocation that we have to > > worry about accidentally leaking or accumulating over time. > > Why can't the server just fill an appropriate entry for > csa_referring_call_lists<> in the CB_SEQUENCE operation for the > CB_OFFLOAD callback? That's the mechanism that is intended to be used > to avoid the above kind of race. Let's say the linux server does implement the list but what about other implementations that will not. The client still needs an approach to handle CB_OFFLOAD/COPY reply. > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Thu, 2025-02-13 at 12:53 -0500, Olga Kornievskaia wrote: > On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust > <trondmy@hammerspace.com> wrote: > > > > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > The NFSv4.2 protocol requires that a client match a CB_OFFLOAD > > > callback to a COPY reply containing the same copy state ID. > > > However, > > > it's possible that the order of the callback and reply processing > > > on > > > the client can cause the CB_OFFLOAD to be received and processed > > > /before/ the client has dealt with the COPY reply. > > > > > > Currently, in this case, the Linux NFS client will queue a fresh > > > struct nfs4_copy_state in the CB_OFFLOAD handler. > > > handle_async_copy() then checks for a matching nfs4_copy_state > > > before settling down to wait for a CB_OFFLOAD reply. > > > > > > But it would be simpler for the client's callback service to > > > respond > > > to such a CB_OFFLOAD with "I'm not ready yet" and have the server > > > send the CB_OFFLOAD again later. This avoids the need for the > > > client's CB_OFFLOAD processing to allocate an extra struct > > > nfs4_copy_state -- in most cases that allocation will be tossed > > > immediately, and it's one less memory allocation that we have to > > > worry about accidentally leaking or accumulating over time. > > > > Why can't the server just fill an appropriate entry for > > csa_referring_call_lists<> in the CB_SEQUENCE operation for the > > CB_OFFLOAD callback? That's the mechanism that is intended to be > > used > > to avoid the above kind of race. > > Let's say the linux server does implement the list but what about > other implementations that will not. The client still needs an > approach to handle CB_OFFLOAD/COPY reply. > > There are several cases that need to be handled. Off the top of my head: 1. The reply to COPY hasn't yet been processed. 2. The COPY is complete, and the state has been forgotten. 3. The stateid presented by CB_OFFLOAD is one that was reused for a second COPY request after a previous one completed. The client will want to send different errors for either case (NFS4ERR_DELAY in the first and third case, NFS4ERR_BAD_STATEID in the second). With csa_referring_call_lists<>, the client can easily distinguish between the cases and return the right response. Without it, the client might end up returning NFS4ERR_BAD_STATEID in case 3, or NFS4ERR_DELAY in case 2, etc... So in practice, we want all servers to do the right thing if they want to avoid confusion over state. The client can't fix these races on its own.
On 2/13/25 1:44 PM, Trond Myklebust wrote: > On Thu, 2025-02-13 at 12:53 -0500, Olga Kornievskaia wrote: >> On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust >> <trondmy@hammerspace.com> wrote: >>> >>> On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: >>>> From: Chuck Lever <chuck.lever@oracle.com> >>>> >>>> The NFSv4.2 protocol requires that a client match a CB_OFFLOAD >>>> callback to a COPY reply containing the same copy state ID. >>>> However, >>>> it's possible that the order of the callback and reply processing >>>> on >>>> the client can cause the CB_OFFLOAD to be received and processed >>>> /before/ the client has dealt with the COPY reply. >>>> >>>> Currently, in this case, the Linux NFS client will queue a fresh >>>> struct nfs4_copy_state in the CB_OFFLOAD handler. >>>> handle_async_copy() then checks for a matching nfs4_copy_state >>>> before settling down to wait for a CB_OFFLOAD reply. >>>> >>>> But it would be simpler for the client's callback service to >>>> respond >>>> to such a CB_OFFLOAD with "I'm not ready yet" and have the server >>>> send the CB_OFFLOAD again later. This avoids the need for the >>>> client's CB_OFFLOAD processing to allocate an extra struct >>>> nfs4_copy_state -- in most cases that allocation will be tossed >>>> immediately, and it's one less memory allocation that we have to >>>> worry about accidentally leaking or accumulating over time. >>> >>> Why can't the server just fill an appropriate entry for >>> csa_referring_call_lists<> in the CB_SEQUENCE operation for the >>> CB_OFFLOAD callback? That's the mechanism that is intended to be >>> used >>> to avoid the above kind of race. >> >> Let's say the linux server does implement the list but what about >> other implementations that will not. The client still needs an >> approach to handle CB_OFFLOAD/COPY reply. >>> > > There are several cases that need to be handled. Off the top of my > head: > 1. The reply to COPY hasn't yet been processed. > 2. The COPY is complete, and the state has been forgotten. > 3. The stateid presented by CB_OFFLOAD is one that was reused for a > second COPY request after a previous one completed. > > The client will want to send different errors for either case > (NFS4ERR_DELAY in the first and third case, NFS4ERR_BAD_STATEID in the > second). > With csa_referring_call_lists<>, the client can easily distinguish > between the cases and return the right response. Without it, the client > might end up returning NFS4ERR_BAD_STATEID in case 3, or NFS4ERR_DELAY > in case 2, etc... > > So in practice, we want all servers to do the right thing if they want > to avoid confusion over state. The client can't fix these races on its > own. > We are currently living in a world where all NFSD-based servers do not return referring calls. I think we need to understand what the client should do in those cases.
On Thu, 2025-02-13 at 13:47 -0500, Chuck Lever wrote: > On 2/13/25 1:44 PM, Trond Myklebust wrote: > > On Thu, 2025-02-13 at 12:53 -0500, Olga Kornievskaia wrote: > > > On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust > > > <trondmy@hammerspace.com> wrote: > > > > > > > > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > > > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > > > > > The NFSv4.2 protocol requires that a client match a > > > > > CB_OFFLOAD > > > > > callback to a COPY reply containing the same copy state ID. > > > > > However, > > > > > it's possible that the order of the callback and reply > > > > > processing > > > > > on > > > > > the client can cause the CB_OFFLOAD to be received and > > > > > processed > > > > > /before/ the client has dealt with the COPY reply. > > > > > > > > > > Currently, in this case, the Linux NFS client will queue a > > > > > fresh > > > > > struct nfs4_copy_state in the CB_OFFLOAD handler. > > > > > handle_async_copy() then checks for a matching > > > > > nfs4_copy_state > > > > > before settling down to wait for a CB_OFFLOAD reply. > > > > > > > > > > But it would be simpler for the client's callback service to > > > > > respond > > > > > to such a CB_OFFLOAD with "I'm not ready yet" and have the > > > > > server > > > > > send the CB_OFFLOAD again later. This avoids the need for the > > > > > client's CB_OFFLOAD processing to allocate an extra struct > > > > > nfs4_copy_state -- in most cases that allocation will be > > > > > tossed > > > > > immediately, and it's one less memory allocation that we have > > > > > to > > > > > worry about accidentally leaking or accumulating over time. > > > > > > > > Why can't the server just fill an appropriate entry for > > > > csa_referring_call_lists<> in the CB_SEQUENCE operation for the > > > > CB_OFFLOAD callback? That's the mechanism that is intended to > > > > be > > > > used > > > > to avoid the above kind of race. > > > > > > Let's say the linux server does implement the list but what about > > > other implementations that will not. The client still needs an > > > approach to handle CB_OFFLOAD/COPY reply. > > > > > > > > There are several cases that need to be handled. Off the top of my > > head: > > 1. The reply to COPY hasn't yet been processed. > > 2. The COPY is complete, and the state has been forgotten. > > 3. The stateid presented by CB_OFFLOAD is one that was reused > > for a > > second COPY request after a previous one completed. > > > > The client will want to send different errors for either case > > (NFS4ERR_DELAY in the first and third case, NFS4ERR_BAD_STATEID in > > the > > second). > > With csa_referring_call_lists<>, the client can easily distinguish > > between the cases and return the right response. Without it, the > > client > > might end up returning NFS4ERR_BAD_STATEID in case 3, or > > NFS4ERR_DELAY > > in case 2, etc... > > > > So in practice, we want all servers to do the right thing if they > > want > > to avoid confusion over state. The client can't fix these races on > > its > > own. > > > > We are currently living in a world where all NFSD-based servers do > not > return referring calls. I think we need to understand what the client > should do in those cases. My answer is: not try to fix that which cannot be fixed. >
On Thu, 2025-02-13 at 18:49 +0000, Trond Myklebust wrote: > On Thu, 2025-02-13 at 13:47 -0500, Chuck Lever wrote: > > On 2/13/25 1:44 PM, Trond Myklebust wrote: > > > On Thu, 2025-02-13 at 12:53 -0500, Olga Kornievskaia wrote: > > > > On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust > > > > <trondmy@hammerspace.com> wrote: > > > > > > > > > > On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: > > > > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > > > > > > > The NFSv4.2 protocol requires that a client match a > > > > > > CB_OFFLOAD > > > > > > callback to a COPY reply containing the same copy state ID. > > > > > > However, > > > > > > it's possible that the order of the callback and reply > > > > > > processing > > > > > > on > > > > > > the client can cause the CB_OFFLOAD to be received and > > > > > > processed > > > > > > /before/ the client has dealt with the COPY reply. > > > > > > > > > > > > Currently, in this case, the Linux NFS client will queue a > > > > > > fresh > > > > > > struct nfs4_copy_state in the CB_OFFLOAD handler. > > > > > > handle_async_copy() then checks for a matching > > > > > > nfs4_copy_state > > > > > > before settling down to wait for a CB_OFFLOAD reply. > > > > > > > > > > > > But it would be simpler for the client's callback service > > > > > > to > > > > > > respond > > > > > > to such a CB_OFFLOAD with "I'm not ready yet" and have the > > > > > > server > > > > > > send the CB_OFFLOAD again later. This avoids the need for > > > > > > the > > > > > > client's CB_OFFLOAD processing to allocate an extra struct > > > > > > nfs4_copy_state -- in most cases that allocation will be > > > > > > tossed > > > > > > immediately, and it's one less memory allocation that we > > > > > > have > > > > > > to > > > > > > worry about accidentally leaking or accumulating over time. > > > > > > > > > > Why can't the server just fill an appropriate entry for > > > > > csa_referring_call_lists<> in the CB_SEQUENCE operation for > > > > > the > > > > > CB_OFFLOAD callback? That's the mechanism that is intended to > > > > > be > > > > > used > > > > > to avoid the above kind of race. > > > > > > > > Let's say the linux server does implement the list but what > > > > about > > > > other implementations that will not. The client still needs an > > > > approach to handle CB_OFFLOAD/COPY reply. > > > > > > > > > > > There are several cases that need to be handled. Off the top of > > > my > > > head: > > > 1. The reply to COPY hasn't yet been processed. > > > 2. The COPY is complete, and the state has been forgotten. > > > 3. The stateid presented by CB_OFFLOAD is one that was reused > > > for a > > > second COPY request after a previous one completed. > > > > > > The client will want to send different errors for either case > > > (NFS4ERR_DELAY in the first and third case, NFS4ERR_BAD_STATEID > > > in > > > the > > > second). > > > With csa_referring_call_lists<>, the client can easily > > > distinguish > > > between the cases and return the right response. Without it, the > > > client > > > might end up returning NFS4ERR_BAD_STATEID in case 3, or > > > NFS4ERR_DELAY > > > in case 2, etc... > > > > > > So in practice, we want all servers to do the right thing if they > > > want > > > to avoid confusion over state. The client can't fix these races > > > on > > > its > > > own. > > > > > > > We are currently living in a world where all NFSD-based servers do > > not > > return referring calls. I think we need to understand what the > > client > > should do in those cases. > > > My answer is: not try to fix that which cannot be fixed. > > Put differently: it is a lot easier to just implement this properly on the server instead of doing a load of contortions on the client. All that needs to be done is to store 3 extra pieces of information when you create the stateid (the session id, slot id and sequence id for the operation that created the stateid, i.e. 24 bytes of information). When you update the stateid, you also replace the stored extra information with the new session id, slot id and sequence id for the operation that caused the stateid's sequence number to be bumped. Then in the CB_OFFLOAD callback, when you look up the stateid, you also look up the 3 stored values and put them in the CB_SEQUENCE op. That's literally all that is needed for the client to be able to figure out the order in which it needs to process the operations.
On 2/13/25 2:12 PM, Trond Myklebust wrote: > On Thu, 2025-02-13 at 18:49 +0000, Trond Myklebust wrote: >> On Thu, 2025-02-13 at 13:47 -0500, Chuck Lever wrote: >>> On 2/13/25 1:44 PM, Trond Myklebust wrote: >>>> On Thu, 2025-02-13 at 12:53 -0500, Olga Kornievskaia wrote: >>>>> On Thu, Feb 13, 2025 at 11:59 AM Trond Myklebust >>>>> <trondmy@hammerspace.com> wrote: >>>>>> >>>>>> On Thu, 2025-02-13 at 11:15 -0500, cel@kernel.org wrote: >>>>>>> From: Chuck Lever <chuck.lever@oracle.com> >>>>>>> >>>>>>> The NFSv4.2 protocol requires that a client match a >>>>>>> CB_OFFLOAD >>>>>>> callback to a COPY reply containing the same copy state ID. >>>>>>> However, >>>>>>> it's possible that the order of the callback and reply >>>>>>> processing >>>>>>> on >>>>>>> the client can cause the CB_OFFLOAD to be received and >>>>>>> processed >>>>>>> /before/ the client has dealt with the COPY reply. >>>>>>> >>>>>>> Currently, in this case, the Linux NFS client will queue a >>>>>>> fresh >>>>>>> struct nfs4_copy_state in the CB_OFFLOAD handler. >>>>>>> handle_async_copy() then checks for a matching >>>>>>> nfs4_copy_state >>>>>>> before settling down to wait for a CB_OFFLOAD reply. >>>>>>> >>>>>>> But it would be simpler for the client's callback service >>>>>>> to >>>>>>> respond >>>>>>> to such a CB_OFFLOAD with "I'm not ready yet" and have the >>>>>>> server >>>>>>> send the CB_OFFLOAD again later. This avoids the need for >>>>>>> the >>>>>>> client's CB_OFFLOAD processing to allocate an extra struct >>>>>>> nfs4_copy_state -- in most cases that allocation will be >>>>>>> tossed >>>>>>> immediately, and it's one less memory allocation that we >>>>>>> have >>>>>>> to >>>>>>> worry about accidentally leaking or accumulating over time. >>>>>> >>>>>> Why can't the server just fill an appropriate entry for >>>>>> csa_referring_call_lists<> in the CB_SEQUENCE operation for >>>>>> the >>>>>> CB_OFFLOAD callback? That's the mechanism that is intended to >>>>>> be >>>>>> used >>>>>> to avoid the above kind of race. >>>>> >>>>> Let's say the linux server does implement the list but what >>>>> about >>>>> other implementations that will not. The client still needs an >>>>> approach to handle CB_OFFLOAD/COPY reply. >>>>>> >>>> >>>> There are several cases that need to be handled. Off the top of >>>> my >>>> head: >>>> 1. The reply to COPY hasn't yet been processed. >>>> 2. The COPY is complete, and the state has been forgotten. >>>> 3. The stateid presented by CB_OFFLOAD is one that was reused >>>> for a >>>> second COPY request after a previous one completed. >>>> >>>> The client will want to send different errors for either case >>>> (NFS4ERR_DELAY in the first and third case, NFS4ERR_BAD_STATEID >>>> in >>>> the >>>> second). >>>> With csa_referring_call_lists<>, the client can easily >>>> distinguish >>>> between the cases and return the right response. Without it, the >>>> client >>>> might end up returning NFS4ERR_BAD_STATEID in case 3, or >>>> NFS4ERR_DELAY >>>> in case 2, etc... >>>> >>>> So in practice, we want all servers to do the right thing if they >>>> want >>>> to avoid confusion over state. The client can't fix these races >>>> on >>>> its >>>> own. >>>> >>> >>> We are currently living in a world where all NFSD-based servers do >>> not >>> return referring calls. I think we need to understand what the >>> client >>> should do in those cases. >> >> >> My answer is: not try to fix that which cannot be fixed. >>> > > Put differently: it is a lot easier to just implement this properly on > the server instead of doing a load of contortions on the client. > > All that needs to be done is to store 3 extra pieces of information > when you create the stateid (the session id, slot id and sequence id > for the operation that created the stateid, i.e. 24 bytes of > information). When you update the stateid, you also replace the stored > extra information with the new session id, slot id and sequence id for > the operation that caused the stateid's sequence number to be bumped. > > Then in the CB_OFFLOAD callback, when you look up the stateid, you also > look up the 3 stored values and put them in the CB_SEQUENCE op. That's > literally all that is needed for the client to be able to figure out > the order in which it needs to process the operations. Yes, that part is obvious, and I'm in the middle of doing just that. But I'm not fixing a /new/ problem. The issue I'm getting at is what to do on the client to handle the existing cohort of NFSD servers that do /not/ currently support referring call lists. That is 100% of the population of today's NFSD servers. I think your answer is to leave the client CB_OFFLOAD implementation as it is: it saves unrecognized copy state IDs, expecting that later a COPY response will match it. That works well enough, but... I'm wondering what happens if a bad network actor injects CB_OFFLOAD calls with bogus copy state IDs. Those nfs4_copy_state structures won't be freed until the mount is gone, and there doesn't appear to be a limit on how many can be created. IMO we need to remove that client logic, eventually. I don't think it should be left in the client.
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c index 8397c43358bd..cd2c3d196f22 100644 --- a/fs/nfs/callback_proc.c +++ b/fs/nfs/callback_proc.c @@ -712,14 +712,10 @@ __be32 nfs4_callback_offload(void *data, void *dummy, struct cb_process_state *cps) { struct cb_offloadargs *args = data; + struct nfs4_copy_state *tmp_copy; struct nfs_server *server; - struct nfs4_copy_state *copy, *tmp_copy; bool found = false; - copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_KERNEL); - if (!copy) - return cpu_to_be32(NFS4ERR_DELAY); - spin_lock(&cps->clp->cl_lock); rcu_read_lock(); list_for_each_entry_rcu(server, &cps->clp->cl_superblocks, @@ -737,17 +733,11 @@ __be32 nfs4_callback_offload(void *data, void *dummy, } out: rcu_read_unlock(); - if (!found) { - memcpy(©->stateid, &args->coa_stateid, NFS4_STATEID_SIZE); - nfs4_copy_cb_args(copy, args); - list_add_tail(©->copies, &cps->clp->pending_cb_stateids); - } else - kfree(copy); spin_unlock(&cps->clp->cl_lock); trace_nfs4_cb_offload(&args->coa_fh, &args->coa_stateid, args->wr_count, args->error, args->wr_writeverf.committed); - return 0; + return found ? cpu_to_be32(NFS4_OK) : cpu_to_be32(NFS4ERR_DELAY); } #endif /* CONFIG_NFS_V4_2 */ diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c index 6e00bef97915..b332eafac8a2 100644 --- a/fs/nfs/nfs42proc.c +++ b/fs/nfs/nfs42proc.c @@ -197,11 +197,11 @@ static int handle_async_copy(struct nfs42_copy_res *res, nfs4_stateid *src_stateid, bool *restart) { - struct nfs4_copy_state *copy, *tmp_copy = NULL, *iter; struct nfs_open_context *dst_ctx = nfs_file_open_context(dst); struct nfs_open_context *src_ctx = nfs_file_open_context(src); struct nfs_client *clp = dst_server->nfs_client; unsigned long timeout = clp->cl_lease_time >> 1; + struct nfs4_copy_state *copy; int status = NFS4_OK; bool retry = false; u64 copied; @@ -211,28 +211,10 @@ static int handle_async_copy(struct nfs42_copy_res *res, return -ENOMEM; spin_lock(&dst_server->nfs_client->cl_lock); - list_for_each_entry(iter, - &dst_server->nfs_client->pending_cb_stateids, - copies) { - if (memcmp(&res->write_res.stateid, &iter->stateid, - NFS4_STATEID_SIZE)) - continue; - tmp_copy = iter; - list_del(&iter->copies); - break; - } - if (tmp_copy) { - spin_unlock(&dst_server->nfs_client->cl_lock); - kfree(copy); - copy = tmp_copy; - goto out; - } - memcpy(©->stateid, &res->write_res.stateid, NFS4_STATEID_SIZE); init_completion(©->completion); copy->parent_dst_state = dst_ctx->state; copy->parent_src_state = src_ctx->state; - list_add_tail(©->copies, &dst_server->ss_copies); spin_unlock(&dst_server->nfs_client->cl_lock); @@ -255,7 +237,6 @@ static int handle_async_copy(struct nfs42_copy_res *res, *restart = true; goto out_cancel; } -out: res->write_res.count = copy->count; /* Copy out the updated write verifier provided by CB_OFFLOAD. */ memcpy(&res->write_res.verifier, ©->verf, sizeof(copy->verf));