[PATCH 4/5] rxrpc, afs: Fix collection of reply timestamp and calculation of callback expiry
David Howells
dhowells at redhat.com
Tue Aug 30 13:04:46 PDT 2022
rxrpc and kafs between them try to use the receive timestamp on the first
data packet (ie. the one with sequence number 1) as a base from which to
calculate the time at which the callback promise from the server expires.
This doesn't work if we receive the last packet before grabbing the
timestamp as the call state gets changed to COMPLETE and
rxrpc_kernel_get_reply_time() returns false, leaving call->reply_time set
at 0.
xdr_decode_YFSCallBack() then uses this anyway, without checking to see if
the value got set, resulting in a callback that has already expired - which
means that kafs will keep issuing FS.FetchStatus calls to try and get a new
promise.
Fix this by just checking to see if the timestamp is 0 and returning it if
it is not in rxrpc_kernel_get_reply_time() and then making
xdr_decode_YFSCallBack() check the call->have_reply_time flag and using the
current time if it's not set.
Further, get the receive timestamp from whichever packet arrives first, no
matter the sequence number (the sequence number will be able to wrap in
future).
Also, the rxrpc_kernel_get_reply_time() can be generalised as it also
records the timestamp for the first request DATA packet on an incoming
service call, so rename it to rxrpc_kernel_get_rx_timestamp() and change
the names used in kafs to match.
This can be tested by doing an ls on a directory and then doing it again
whilst running a packet trace and seeing if a FetchStatus is generated the
second time (which it shouldn't).
Fixes: 781070551c26 ("afs: Fix calculation of callback expiry time")
Fixes: 2070a3e44962 ("rxrpc: Allow the reply time to be obtained on a client call")
Signed-off-by: David Howells <dhowells at redhat.com>
---
Documentation/networking/rxrpc.rst | 15 ++++++-------
fs/afs/flock.c | 2 +-
fs/afs/fsclient.c | 2 +-
fs/afs/internal.h | 4 ++-
fs/afs/rxrpc.c | 10 ++++----
fs/afs/yfsclient.c | 8 ++++---
include/net/af_rxrpc.h | 4 ++-
net/rxrpc/ar-internal.h | 4 +++
net/rxrpc/input.c | 3 +++
net/rxrpc/recvmsg.c | 43 +++++++++---------------------------
10 files changed, 41 insertions(+), 54 deletions(-)
diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
index 39c2249c7aa7..fcde8297df53 100644
--- a/Documentation/networking/rxrpc.rst
+++ b/Documentation/networking/rxrpc.rst
@@ -1055,16 +1055,15 @@ The kernel interface functions are as follows:
first function to change. Note that this must be called in TASK_RUNNING
state.
- (#) Get reply timestamp::
+ (#) Get the timestamp of the first DATA packet received::
- bool rxrpc_kernel_get_reply_time(struct socket *sock,
- struct rxrpc_call *call,
- ktime_t *_ts)
+ bool rxrpc_kernel_get_rx_timestamp(struct socket *sock,
+ struct rxrpc_call *call,
+ ktime_t *_ts)
- This allows the timestamp on the first DATA packet of the reply of a
- client call to be queried, provided that it is still in the Rx ring. If
- successful, the timestamp will be stored into ``*_ts`` and true will be
- returned; false will be returned otherwise.
+ This allows the timestamp on the first DATA packet received on a call to
+ be queried. If successful, the timestamp will be stored into ``*_ts`` and
+ true will be returned; false will be returned otherwise.
(#) Get remote client epoch::
diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index c4210a3964d8..81b5d0c39683 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -76,7 +76,7 @@ void afs_lock_op_done(struct afs_call *call)
if (call->error == 0) {
spin_lock(&vnode->lock);
trace_afs_flock_ev(vnode, NULL, afs_flock_timestamp, 0);
- vnode->locked_at = call->reply_time;
+ vnode->locked_at = call->rx_timestamp;
afs_schedule_lock_extension(vnode);
spin_unlock(&vnode->lock);
}
diff --git a/fs/afs/fsclient.c b/fs/afs/fsclient.c
index 4943413d9c5f..ef79ee720444 100644
--- a/fs/afs/fsclient.c
+++ b/fs/afs/fsclient.c
@@ -131,7 +131,7 @@ static void xdr_decode_AFSFetchStatus(const __be32 **_bp,
static time64_t xdr_decode_expiry(struct afs_call *call, u32 expiry)
{
- return ktime_divns(call->reply_time, NSEC_PER_SEC) + expiry;
+ return ktime_divns(call->rx_timestamp, NSEC_PER_SEC) + expiry;
}
static void xdr_decode_AFSCallBack(const __be32 **_bp,
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 64ad55494349..9df0cb75504b 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -137,7 +137,7 @@ struct afs_call {
bool need_attention; /* T if RxRPC poked us */
bool async; /* T if asynchronous */
bool upgrade; /* T to request service upgrade */
- bool have_reply_time; /* T if have got reply_time */
+ bool have_rx_timestamp; /* T if have got rx_timestamp */
bool intr; /* T if interruptible */
bool unmarshalling_error; /* T if an unmarshalling error occurred */
u16 service_id; /* Actual service ID (after upgrade) */
@@ -151,7 +151,7 @@ struct afs_call {
} __attribute__((packed));
__be64 tmp64;
};
- ktime_t reply_time; /* Time of first reply packet */
+ ktime_t rx_timestamp; /* Time of first data packet */
};
struct afs_call_type {
diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index d5c4785c862d..f67cdc54ffce 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -501,11 +501,11 @@ static void afs_deliver_to_call(struct afs_call *call)
return;
}
- if (!call->have_reply_time &&
- rxrpc_kernel_get_reply_time(call->net->socket,
- call->rxcall,
- &call->reply_time))
- call->have_reply_time = true;
+ if (!call->have_rx_timestamp &&
+ rxrpc_kernel_get_rx_timestamp(call->net->socket,
+ call->rxcall,
+ &call->rx_timestamp))
+ call->have_rx_timestamp = true;
ret = call->type->deliver(call);
state = READ_ONCE(call->state);
diff --git a/fs/afs/yfsclient.c b/fs/afs/yfsclient.c
index fdc7d675b4b0..ea777ed7f528 100644
--- a/fs/afs/yfsclient.c
+++ b/fs/afs/yfsclient.c
@@ -230,10 +230,12 @@ static void xdr_decode_YFSCallBack(const __be32 **_bp,
{
struct yfs_xdr_YFSCallBack *x = (void *)*_bp;
struct afs_callback *cb = &scb->callback;
- ktime_t cb_expiry;
+ ktime_t cb_expiry, base = call->rx_timestamp;
- cb_expiry = call->reply_time;
- cb_expiry = ktime_add(cb_expiry, xdr_to_u64(x->expiration_time) * 100);
+ if (!call->have_rx_timestamp)
+ base = ktime_get_real();
+
+ cb_expiry = ktime_add(base, xdr_to_u64(x->expiration_time) * 100);
cb->expires_at = ktime_divns(cb_expiry, NSEC_PER_SEC);
scb->have_cb = true;
*_bp += xdr_size(x);
diff --git a/include/net/af_rxrpc.h b/include/net/af_rxrpc.h
index cee5f83c0f11..50f9f09027ee 100644
--- a/include/net/af_rxrpc.h
+++ b/include/net/af_rxrpc.h
@@ -66,8 +66,8 @@ int rxrpc_kernel_charge_accept(struct socket *, rxrpc_notify_rx_t,
void rxrpc_kernel_set_tx_length(struct socket *, struct rxrpc_call *, s64);
bool rxrpc_kernel_check_life(const struct socket *, const struct rxrpc_call *);
u32 rxrpc_kernel_get_epoch(struct socket *, struct rxrpc_call *);
-bool rxrpc_kernel_get_reply_time(struct socket *, struct rxrpc_call *,
- ktime_t *);
+bool rxrpc_kernel_get_rx_timestamp(struct socket *, struct rxrpc_call *,
+ ktime_t *);
bool rxrpc_kernel_call_is_complete(struct rxrpc_call *);
void rxrpc_kernel_set_max_life(struct socket *, struct rxrpc_call *,
unsigned long);
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 62c70709d798..26bab72ad8fb 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -639,9 +639,13 @@ struct rxrpc_call {
rxrpc_seq_t tx_hard_ack; /* Dead slot in buffer; the first transmitted but
* not hard-ACK'd packet follows this.
*/
+ /* Transmitted data tracking. */
rxrpc_seq_t tx_top; /* Highest Tx slot allocated. */
u16 tx_backoff; /* Delay to insert due to Tx failure */
+ /* Received data tracking */
+ ktime_t rx_data_tstamp; /* Timestamp of first DATA packet received */
+
/* TCP-style slow-start congestion control [RFC5681]. Since the SMSS
* is fixed, we keep these numbers in terms of segments (ie. DATA
* packets) rather than bytes.
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 721d847ba92b..9cc533590fd4 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -453,6 +453,9 @@ static void rxrpc_input_data(struct rxrpc_call *call, struct sk_buff *skb)
!rxrpc_receiving_reply(call))
goto unlock;
+ if (!call->rx_data_tstamp)
+ call->rx_data_tstamp = skb_get_ktime(skb);
+
hard_ack = READ_ONCE(call->rx_hard_ack);
nr_subpackets = sp->nr_subpackets;
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index 250f23bc1c07..f6a86980ff94 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -773,44 +773,23 @@ int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
EXPORT_SYMBOL(rxrpc_kernel_recv_data);
/**
- * rxrpc_kernel_get_reply_time - Get timestamp on first reply packet
+ * rxrpc_kernel_get_rx_timestamp - Get time of first DATA packet received
* @sock: The socket that the call exists on
* @call: The call to query
* @_ts: Where to put the timestamp
*
- * Retrieve the timestamp from the first DATA packet of the reply if it is
- * in the ring. Returns true if successful, false if not.
+ * Retrieve the timestamp from the first DATA packet received on a call.
+ * Returns true if successful, false if not.
*/
-bool rxrpc_kernel_get_reply_time(struct socket *sock, struct rxrpc_call *call,
- ktime_t *_ts)
+bool rxrpc_kernel_get_rx_timestamp(struct socket *sock, struct rxrpc_call *call,
+ ktime_t *_ts)
{
- struct sk_buff *skb;
- rxrpc_seq_t hard_ack, top, seq;
- bool success = false;
+ ktime_t tstamp = READ_ONCE(call->rx_data_tstamp);
- mutex_lock(&call->user_mutex);
-
- if (READ_ONCE(call->state) != RXRPC_CALL_CLIENT_RECV_REPLY)
- goto out;
+ if (tstamp == 0)
+ return false;
- hard_ack = call->rx_hard_ack;
- if (hard_ack != 0)
- goto out;
-
- seq = hard_ack + 1;
- top = smp_load_acquire(&call->rx_top);
- if (after(seq, top))
- goto out;
-
- skb = call->rxtx_buffer[seq & RXRPC_RXTX_BUFF_MASK];
- if (!skb)
- goto out;
-
- *_ts = skb_get_ktime(skb);
- success = true;
-
-out:
- mutex_unlock(&call->user_mutex);
- return success;
+ *_ts = tstamp;
+ return true;
}
-EXPORT_SYMBOL(rxrpc_kernel_get_reply_time);
+EXPORT_SYMBOL(rxrpc_kernel_get_rx_timestamp);
More information about the linux-afs
mailing list