Re: [PATCH net] Revert "rxrpc: Allow failed client calls to be retried"

2019-01-15 Thread David Miller
From: David Howells 
Date: Thu, 10 Jan 2019 16:59:13 +

> The changes introduced to allow rxrpc calls to be retried creates an issue
> when it comes to refcounting afs_call structs.  The problem is that when
> rxrpc_send_data() queues the last packet for an asynchronous call, the
> following sequence can occur:
 ...
> Synchronising the clean up after rxrpc_kernel_send_data() returns an error
> with the asynchronous cleanup is then tricky to get right.
> 
> Mostly revert commit c038a58ccfd6704d4d7d60ed3d6a0fca13cf13a4.  The two API
> functions the original commit added aren't currently used.  This makes
> rxrpc_kernel_send_data() always return successfully if it queued the data
> it was given.
> 
> Note that this doesn't affect synchronous calls since their Rx notification
> function merely pokes a wait queue and does not refcounting.  The
> asynchronous call notification function *has* to do refcounting and pass a
> ref over the work item to avoid the need to sync the workqueue in call
> cleanup.
> 
> Signed-off-by: David Howells 

Applied.


[PATCH net] Revert "rxrpc: Allow failed client calls to be retried"

2019-01-10 Thread David Howells
The changes introduced to allow rxrpc calls to be retried creates an issue
when it comes to refcounting afs_call structs.  The problem is that when
rxrpc_send_data() queues the last packet for an asynchronous call, the
following sequence can occur:

 (1) The notify_end_tx callback is invoked which causes the state in the
 afs_call to be changed from AFS_CALL_CL_REQUESTING or
 AFS_CALL_SV_REPLYING.

 (2) afs_deliver_to_call() can then process event notifications from rxrpc
 on the async_work queue.

 (3) Delivery of events, such as an abort from the server, can cause the
 afs_call state to be changed to AFS_CALL_COMPLETE on async_work.

 (4) For an asynchronous call, afs_process_async_call() notes that the call
 is complete and tried to clean up all the refs on async_work.

 (5) rxrpc_send_data() might return the amount of data transferred
 (success) or an error - which could in turn reflect a local error or a
 received error.

Synchronising the clean up after rxrpc_kernel_send_data() returns an error
with the asynchronous cleanup is then tricky to get right.

Mostly revert commit c038a58ccfd6704d4d7d60ed3d6a0fca13cf13a4.  The two API
functions the original commit added aren't currently used.  This makes
rxrpc_kernel_send_data() always return successfully if it queued the data
it was given.

Note that this doesn't affect synchronous calls since their Rx notification
function merely pokes a wait queue and does not refcounting.  The
asynchronous call notification function *has* to do refcounting and pass a
ref over the work item to avoid the need to sync the workqueue in call
cleanup.

Signed-off-by: David Howells 
---

 Documentation/networking/rxrpc.txt |   45 -
 include/net/af_rxrpc.h |   16 --
 net/rxrpc/af_rxrpc.c   |   70 --
 net/rxrpc/ar-internal.h|   19 ---
 net/rxrpc/call_object.c|   97 
 net/rxrpc/conn_client.c|5 --
 net/rxrpc/sendmsg.c|   24 -
 7 files changed, 24 insertions(+), 252 deletions(-)

diff --git a/Documentation/networking/rxrpc.txt 
b/Documentation/networking/rxrpc.txt
index c9d052e0cf51..2df5894353d6 100644
--- a/Documentation/networking/rxrpc.txt
+++ b/Documentation/networking/rxrpc.txt
@@ -1000,51 +1000,6 @@ The kernel interface functions are as follows:
  size should be set when the call is begun.  tx_total_len may not be less
  than zero.
 
- (*) Check to see the completion state of a call so that the caller can assess
- whether it needs to be retried.
-
-   enum rxrpc_call_completion {
-   RXRPC_CALL_SUCCEEDED,
-   RXRPC_CALL_REMOTELY_ABORTED,
-   RXRPC_CALL_LOCALLY_ABORTED,
-   RXRPC_CALL_LOCAL_ERROR,
-   RXRPC_CALL_NETWORK_ERROR,
-   };
-
-   int rxrpc_kernel_check_call(struct socket *sock, struct rxrpc_call 
*call,
-   enum rxrpc_call_completion *_compl,
-   u32 *_abort_code);
-
- On return, -EINPROGRESS will be returned if the call is still ongoing; if
- it is finished, *_compl will be set to indicate the manner of completion,
- *_abort_code will be set to any abort code that occurred.  0 will be
- returned on a successful completion, -ECONNABORTED will be returned if the
- client failed due to a remote abort and anything else will return an
- appropriate error code.
-
- The caller should look at this information to decide if it's worth
- retrying the call.
-
- (*) Retry a client call.
-
-   int rxrpc_kernel_retry_call(struct socket *sock,
-   struct rxrpc_call *call,
-   struct sockaddr_rxrpc *srx,
-   struct key *key);
-
- This attempts to partially reinitialise a call and submit it again while
- reusing the original call's Tx queue to avoid the need to repackage and
- re-encrypt the data to be sent.  call indicates the call to retry, srx the
- new address to send it to and key the encryption key to use for signing or
- encrypting the packets.
-
- For this to work, the first Tx data packet must still be in the transmit
- queue, and currently this is only permitted for local and network errors
- and the call must not have been aborted.  Any partially constructed Tx
- packet is left as is and can continue being filled afterwards.
-
- It returns 0 if the call was requeued and an error otherwise.
-
  (*) Get call RTT.
 
u64 rxrpc_kernel_get_rtt(struct socket *sock, struct rxrpc_call *call);
diff --git a/include/net/af_rxrpc.h b/include/net/af_rxrpc.h
index 1adefe42c0a6..2bfb87eb98ce 100644
--- a/include/net/af_rxrpc.h
+++ b/include/net/af_rxrpc.h
@@ -20,18 +20,6 @@ struct sock;
 struct socket;
 struct rxrpc_call;
 
-/*
- * Call completion condition (state == RX