smbdirect client/server credit fixes
Subsystems (4)
Stefan Metzmacher
reviewerv2 cover smbdirect client/server credit fixes
Hi,
here is my current state of the regression fixes we have in 6.18 and 6.19-rc6.
We may add some more detailed Fixes tags, but for now at least everything is marked as stable for 6.18.
I tested the patches and fixed a few minor but important things in the client patches.
v2:
- we should not add new_credits twice to sc->recv_io.credits.count in the client patches
- also the need to use request->wr instead of a struct ib_send_wr on the stack for the client side negotiate request.
Stefan Metzmacher (20):
smb: smbdirect: introduce smbdirect_socket.recv_io.credits.available
smb: smbdirect: introduce smbdirect_socket.send_io.bcredits.*
smb: server: make use of smbdirect_socket.recv_io.credits.available
smb: server: let recv_done() queue a refill when the peer is low on
credits
smb: server: make use of smbdirect_socket.send_io.bcredits
smb: server: fix last send credit problem causing disconnects
smb: server: let send_done handle a completion without
IB_SEND_SIGNALED
smb: client: make use of smbdirect_socket.recv_io.credits.available
smb: client: let recv_done() queue a refill when the peer is low on
credits
smb: client: let smbd_post_send() make use of request->wr
smb: client: remove pointless sc->recv_io.credits.count rollback
smb: client: remove pointless sc->send_io.pending handling in
smbd_post_send_iter()
smb: client: port and use the wait_for_credits logic used by server
smb: client: split out smbd_ib_post_send()
smb: client: introduce and use smbd_{alloc,free}_send_io()
smb: client: use smbdirect_send_batch processing
smb: client: make use of smbdirect_socket.send_io.bcredits
smb: client: fix last send credit problem causing disconnects
smb: client: let smbd_post_send_negotiate_req() use smbd_post_send()
smb: client: let send_done handle a completion without
IB_SEND_SIGNALED
fs/smb/client/smbdirect.c | 523 ++++++++++++++++----- fs/smb/common/smbdirect/smbdirect_socket.h | 18 + fs/smb/server/transport_rdma.c | 147 +++++- 3 files changed, 551 insertions(+), 137 deletions(-)
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 1/20 smb: smbdirect: introduce smbdirect_socket.recv_io.credits.available
The logic off managing recv credits by counting posted recv_io and granted credits is racy.
That's because the peer might already consumed a credit, but between receiving the incoming recv at the hardware and processing the completion in the 'recv_done' functions we likely have a window where we grant credits, which don't really exist.
So we better have a decicated counter for the
available credits, which will be incremented
when we posted new recv buffers and drained when
we grant the credits to the peer.
--- fs/smb/common/smbdirect/smbdirect_socket.h | 2 ++ 1 file changed, 2 insertions(+)
fs/smb/common/smbdirect/smbdirect_socket.h +2 -0
@@ -239,6 +239,7 @@ struct smbdirect_socket {*/ struct {u16 target; +atomic_t available;atomic_t count; } credits;
@@ -387,6 +388,7 @@ static __always_inline void smbdirect_socket_init(struct smbdirect_socket *sc)INIT_WORK (&sc->recv_io.posted.refill_work, __smbdirect_socket_disabled_work);disable_work_sync (&sc->recv_io.posted.refill_work); +atomic_set (&sc->recv_io.credits.available,0 );atomic_set (&sc->recv_io.credits.count,0 );INIT_LIST_HEAD (&sc->recv_io.reassembly.list);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 2/20 smb: smbdirect: introduce smbdirect_socket.send_io.bcredits.*
In reply to Stefan MetzmacherIt turns out that our code will corrupt the stream of reassabled data transfer messages when we trigger an immendiate (empty) send.
In order to fix this we'll have a single 'batch' credit per connection. And code getting that credit is free to use as much messages until remaining_length reaches 0, then the batch credit it given back and the next logical send can happen.
--- fs/smb/common/smbdirect/smbdirect_socket.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
fs/smb/common/smbdirect/smbdirect_socket.h +16 -0
@@ -162,6 +162,17 @@ struct smbdirect_socket {mempool_t *pool; } mem; +/* +* This is a coordination for smbdirect_send_batch. +* +* There's only one possible credit, which means +* only one instance is running at a time. +*/ +struct { +atomic_t count; +wait_queue_head_t wait_queue; + } bcredits; +/* * The local credit state for ib_post_send() */
@@ -371,6 +382,9 @@ static __always_inline void smbdirect_socket_init(struct smbdirect_socket *sc)INIT_DELAYED_WORK (&sc->idle.timer_work, __smbdirect_socket_disabled_work);disable_delayed_work_sync (&sc->idle.timer_work); +atomic_set (&sc->send_io.bcredits.count,0 ); +init_waitqueue_head (&sc->send_io.bcredits.wait_queue); +atomic_set (&sc->send_io.lcredits.count,0 );init_waitqueue_head (&sc->send_io.lcredits.wait_queue);
@@ -485,6 +499,8 @@ struct smbdirect_send_batch {*/ bool need_invalidate_rkey;u32 remote_key; + +int credit; };struct smbdirect_recv_io {
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 3/20 smb: server: make use of smbdirect_socket.recv_io.credits.available
In reply to Stefan MetzmacherThe logic off managing recv credits by counting posted recv_io and granted credits is racy.
That's because the peer might already consumed a credit, but between receiving the incoming recv at the hardware and processing the completion in the 'recv_done' functions we likely have a window where we grant credits, which don't really exist.
So we better have a decicated counter for the
available credits, which will be incremented
when we posted new recv buffers and drained when
we grant the credits to the peer.
This fixes regression Namjae reported with
the 6.18 release.
--- fs/smb/server/transport_rdma.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
fs/smb/server/transport_rdma.c +25 -5
@@ -1028,6 +1028,8 @@ static void smb_direct_post_recv_credits(struct work_struct *work) } } +atomic_add (credits, &sc->recv_io.credits.available); +if (credits)queue_work (sc->workqueue, &sc->idle.immediate_work); }
@@ -1074,19 +1076,37 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)static int manage_credits_prior_sending (struct smbdirect_socket *sc) { +int missing; +int available;int new_credits;if (atomic_read (&sc->recv_io.credits.count) >= sc->recv_io.credits.target)return 0 ; - new_credits =atomic_read (&sc->recv_io.posted.count); -if (new_credits ==0 ) + missing = (int )sc->recv_io.credits.target -atomic_read (&sc->recv_io.credits.count); + available =atomic_xchg (&sc->recv_io.credits.available,0 ); + new_credits = (u16 )min3 (U16_MAX , missing, available); +if (new_credits <=0 ) { +/* +* If credits are available, but not granted +* we need to re-add them again. +*/ +if (available) +atomic_add (available, &sc->recv_io.credits.available);return 0 ; + } - new_credits -=atomic_read (&sc->recv_io.credits.count); -if (new_credits <=0 ) -return 0 ; +if (new_credits < available) { +/* +* Readd the remaining available again. +*/ + available -= new_credits; +atomic_add (available, &sc->recv_io.credits.available); + } +/* +* Remember we granted the credits +*/ atomic_add (new_credits, &sc->recv_io.credits.count);return new_credits; }
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 4/20 smb: server: let recv_done() queue a refill when the peer is low on credits
In reply to Stefan MetzmacherIn captures I saw that Windows was granting 191 credits in a batch when its peer posted a lot of messages. We are asking for a credit target of 255 and 191 is 252*3/4.
So we also use that logic in order to fill the
recv buffers available to the peer.
--- fs/smb/server/transport_rdma.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
fs/smb/server/transport_rdma.c +4 -2
@@ -644,6 +644,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)struct smbdirect_data_transfer *data_transfer = (struct smbdirect_data_transfer *)recvmsg->packet;u32 remaining_data_length, data_offset, data_length; +int current_recv_credits;u16 old_recv_credit_target; if (wc->byte_len <
@@ -682,7 +683,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc) }atomic_dec (&sc->recv_io.posted.count); -atomic_dec (&sc->recv_io.credits.count); + current_recv_credits =atomic_dec_return (&sc->recv_io.credits.count); old_recv_credit_target = sc->recv_io.credits.target; sc->recv_io.credits.target =
@@ -702,7 +703,8 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)wake_up (&sc->send_io.credits.wait_queue);if (data_length) { -if (sc->recv_io.credits.target > old_recv_credit_target) +if (current_recv_credits <= (sc->recv_io.credits.target /4 ) || + sc->recv_io.credits.target > old_recv_credit_target)queue_work (sc->workqueue, &sc->recv_io.posted.refill_work);enqueue_reassembly (sc, recvmsg, (int )data_length);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 5/20 smb: server: make use of smbdirect_socket.send_io.bcredits
In reply to Stefan MetzmacherIt turns out that our code will corrupt the stream of reassabled data transfer messages when we trigger an immendiate (empty) send.
In order to fix this we'll have a single 'batch' credit per connection. And code getting that credit is free to use as much messages until remaining_length reaches 0, then the batch credit it given back and the next logical send can happen.
--- fs/smb/server/transport_rdma.c | 53 ++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-)
fs/smb/server/transport_rdma.c +51 -2
@@ -221,6 +221,7 @@ static void smb_direct_disconnect_wake_up_all(struct smbdirect_socket *sc)* in order to notice the broken connection. */ wake_up_all (&sc->status_wait); +wake_up_all (&sc->send_io.bcredits.wait_queue);wake_up_all (&sc->send_io.lcredits.wait_queue);wake_up_all (&sc->send_io.credits.wait_queue);wake_up_all (&sc->send_io.pending.zero_wait_queue);
@@ -1152,6 +1153,7 @@ static void smb_direct_send_ctx_init(struct smbdirect_send_batch *send_ctx, send_ctx->wr_cnt =0 ; send_ctx->need_invalidate_rkey = need_invalidate_rkey; send_ctx->remote_key = remote_key; + send_ctx->credit =0 ; } static int smb_direct_flush_send_list(struct smbdirect_socket *sc,
@@ -1159,10 +1161,10 @@ static int smb_direct_flush_send_list(struct smbdirect_socket *sc, bool is_last) {struct smbdirect_send_io *first, *last; -int ret; +int ret =0 ;if (list_empty (&send_ctx->msg_list)) -return 0 ; + goto release_credit; first = list_first_entry(&send_ctx->msg_list, struct smbdirect_send_io,
@@ -1204,6 +1206,13 @@ static int smb_direct_flush_send_list(struct smbdirect_socket *sc,smb_direct_free_sendmsg (sc, last); } +release_credit: +if (is_last && !ret && send_ctx->credit) { +atomic_add (send_ctx->credit, &sc->send_io.bcredits.count); + send_ctx->credit =0 ; +wake_up (&sc->send_io.bcredits.wait_queue); + } +return ret; }
@@ -1229,6 +1238,25 @@ static int wait_for_credits(struct smbdirect_socket *sc, }while (true); } +static int wait_for_send_bcredit (struct smbdirect_socket *sc, +struct smbdirect_send_batch *send_ctx) +{ +int ret; + +if (send_ctx->credit) +return 0 ; + + ret =wait_for_credits (sc, + &sc->send_io.bcredits.wait_queue, + &sc->send_io.bcredits.count, +1 ); +if (ret) +return ret; + + send_ctx->credit =1 ; +return 0 ; +} +static int wait_for_send_lcredit (struct smbdirect_socket *sc,struct smbdirect_send_batch *send_ctx) {
@@ -1432,6 +1460,16 @@ static int smb_direct_post_send_data(struct smbdirect_socket *sc,struct smbdirect_send_io *msg;int data_length;struct scatterlist sg[SMBDIRECT_SEND_IO_MAX_SGE -1 ]; +struct smbdirect_send_batch _send_ctx; + +if (!send_ctx) { +smb_direct_send_ctx_init (&_send_ctx, false,0 ); + send_ctx = &_send_ctx; + } + + ret =wait_for_send_bcredit (sc, send_ctx); +if (ret) + goto bcredit_failed; ret =wait_for_send_lcredit (sc, send_ctx);if (ret)
@@ -1483,6 +1521,13 @@ static int smb_direct_post_send_data(struct smbdirect_socket *sc, ret =post_sendmsg (sc, send_ctx, msg);if (ret) goto err; + +if (send_ctx == &_send_ctx) { + ret =smb_direct_flush_send_list (sc, send_ctx, true); +if (ret) + goto err; + } +return 0 ; err:smb_direct_free_sendmsg (sc, msg);
@@ -1491,6 +1536,9 @@ static int smb_direct_post_send_data(struct smbdirect_socket *sc, credit_failed:atomic_inc (&sc->send_io.lcredits.count); lcredit_failed: +atomic_add (send_ctx->credit, &sc->send_io.bcredits.count); + send_ctx->credit =0 ; +bcredit_failed:return ret; }
@@ -1962,6 +2010,7 @@ static int smb_direct_send_negotiate_response(struct smbdirect_socket *sc, resp->max_fragmented_size =cpu_to_le32 (sp->max_fragmented_recv_size); +atomic_set (&sc->send_io.bcredits.count,1 ); sc->recv_io.expected =SMBDIRECT_EXPECT_DATA_TRANSFER ; sc->status =SMBDIRECT_SOCKET_CONNECTED ; }
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 6/20 smb: server: fix last send credit problem causing disconnects
In reply to Stefan MetzmacherWhen we are about to use the last send credit that was granted to us by the peer, we need to wait until we are ourself able to grant at least one credit to the peer. Otherwise it might not be possible for the peer to grant more credits.
The following sections in MS-SMBD are related to this:
3.1.5.1 Sending Upper Layer Messages
...
If Connection.SendCredits is 1 and the CreditsGranted field of the message is 0, stop processing.
...
3.1.5.9 Managing Credits Prior to Sending
...
If Connection.ReceiveCredits is zero, or if Connection.SendCredits is one and the Connection.SendQueue is not empty, the sender MUST allocate and post at least one receive of size Connection.MaxReceiveSize and MUST increment Connection.ReceiveCredits by the number allocated and posted. If no receives are posted, the processing MUST return a value of zero to indicate to the caller that no Send message can be currently performed.
...
This problem was found by running this on Windows 2025 against ksmbd with required smb signing:
'frametest.exe -r 4k -t 20 -n 2000' after 'frametest.exe -w 4k -t 20 -n 2000'.
--- fs/smb/server/transport_rdma.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)
fs/smb/server/transport_rdma.c +30 -2
@@ -1033,6 +1033,15 @@ static void smb_direct_post_recv_credits(struct work_struct *work)atomic_add (credits, &sc->recv_io.credits.available); +/* +* If the last send credit is waiting for credits +* it can grant we need to wake it up +*/ +if (credits && +atomic_read (&sc->send_io.bcredits.count) ==0 && +atomic_read (&sc->send_io.credits.count) ==0 ) +wake_up (&sc->send_io.credits.wait_queue); +if (credits)queue_work (sc->workqueue, &sc->idle.immediate_work); }
@@ -1306,6 +1315,7 @@ static int calc_rw_credits(struct smbdirect_socket *sc,static int smb_direct_create_header (struct smbdirect_socket *sc,int size,int remaining_data_length, +int new_credits,struct smbdirect_send_io **sendmsg_out) {struct smbdirect_socket_parameters *sp = &sc->parameters;
@@ -1321,7 +1331,7 @@ static int smb_direct_create_header(struct smbdirect_socket *sc,/* Fill in the packet header */ packet = (struct smbdirect_data_transfer *)sendmsg->packet; packet->credits_requested =cpu_to_le16 (sp->send_credit_target); - packet->credits_granted =cpu_to_le16 (manage_credits_prior_sending (sc)); + packet->credits_granted =cpu_to_le16 (new_credits); packet->flags =0 ;if (manage_keep_alive_before_sending (sc))
@@ -1461,6 +1471,7 @@ static int smb_direct_post_send_data(struct smbdirect_socket *sc,int data_length;struct scatterlist sg[SMBDIRECT_SEND_IO_MAX_SGE -1 ];struct smbdirect_send_batch _send_ctx; +int new_credits;if (!send_ctx) {smb_direct_send_ctx_init (&_send_ctx, false,0 );
@@ -1479,12 +1490,29 @@ static int smb_direct_post_send_data(struct smbdirect_socket *sc,if (ret) goto credit_failed; + new_credits =manage_credits_prior_sending (sc); +if (new_credits ==0 && +atomic_read (&sc->send_io.credits.count) ==0 && +atomic_read (&sc->recv_io.credits.count) ==0 ) { +queue_work (sc->workqueue, &sc->recv_io.posted.refill_work); + ret =wait_event_interruptible (sc->send_io.credits.wait_queue, +atomic_read (&sc->send_io.credits.count) >=1 || +atomic_read (&sc->recv_io.credits.available) >=1 || + sc->status !=SMBDIRECT_SOCKET_CONNECTED ); +if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) + ret = -ENOTCONN ; +if (ret <0 ) + goto credit_failed; + + new_credits =manage_credits_prior_sending (sc); + } + data_length =0 ;for (i =0 ; i < niov; i++) data_length += iov[i].iov_len; ret =smb_direct_create_header (sc, data_length, remaining_data_length, - &msg); + new_credits, &msg);if (ret) goto header_failed;
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 7/20 smb: server: let send_done handle a completion without IB_SEND_SIGNALED
In reply to Stefan MetzmacherWith smbdirect_send_batch processing we likely have requests without IB_SEND_SIGNALED, which will be destroyed in the final request that has IB_SEND_SIGNALED set.
If the connection is broken all requests are signaled even without explicit IB_SEND_SIGNALED.
--- fs/smb/server/transport_rdma.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
fs/smb/server/transport_rdma.c +26 -0
@@ -1059,6 +1059,31 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)ib_wc_status_msg (wc->status), wc->status, wc->opcode); +if (unlikely (!(sendmsg->wr.send_flags &IB_SEND_SIGNALED ))) { +/* +* This happens when smbdirect_send_io is a sibling +* before the final message, it is signaled on +* error anyway, so we need to skip +* smbdirect_connection_free_send_io here, +* otherwise is will destroy the memory +* of the siblings too, which will cause +* use after free problems for the others +* triggered from ib_drain_qp(). +*/ +if (wc->status !=IB_WC_SUCCESS ) + goto skip_free; + +/* +* This should not happen! +* But we better just close the +* connection... +*/ +pr_err ("unexpected send completion wc->status=%s (%d) wc->opcode=%d\n" , +ib_wc_status_msg (wc->status), wc->status, wc->opcode); +smb_direct_disconnect_rdma_connection (sc); +return ; + } +/* * Free possible siblings and then the main send_io */
@@ -1072,6 +1097,7 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc) lcredits +=1 ;if (wc->status !=IB_WC_SUCCESS || wc->opcode !=IB_WC_SEND ) { +skip_free:pr_err ("Send error. status='%s (%d)', opcode=%d\n" ,ib_wc_status_msg (wc->status), wc->status, wc->opcode);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 8/20 smb: client: make use of smbdirect_socket.recv_io.credits.available
In reply to Stefan MetzmacherThe logic off managing recv credits by counting posted recv_io and granted credits is racy.
That's because the peer might already consumed a credit, but between receiving the incoming recv at the hardware and processing the completion in the 'recv_done' functions we likely have a window where we grant credits, which don't really exist.
So we better have a decicated counter for the
available credits, which will be incremented
when we posted new recv buffers and drained when
we grant the credits to the peer.
--- fs/smb/client/smbdirect.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
fs/smb/client/smbdirect.c +28 -6
@@ -618,6 +618,7 @@ static void smbd_post_send_credits(struct work_struct *work)struct smbdirect_recv_io *response;struct smbdirect_socket *sc = container_of(work, struct smbdirect_socket, recv_io.posted.refill_work); +int posted =0 ;if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) {return ;
@@ -640,9 +641,12 @@ static void smbd_post_send_credits(struct work_struct *work) }atomic_inc (&sc->recv_io.posted.count); + posted +=1 ; } } +atomic_add (posted, &sc->recv_io.credits.available); +/* Promptly send an immediate packet as defined in [MS-SMBD] 3.1.1.1 */ if (atomic_read (&sc->recv_io.credits.count) < sc->recv_io.credits.target -1 ) {
@@ -1033,19 +1037,38 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc)*/ static int manage_credits_prior_sending (struct smbdirect_socket *sc) { +int missing; +int available;int new_credits;if (atomic_read (&sc->recv_io.credits.count) >= sc->recv_io.credits.target)return 0 ; - new_credits =atomic_read (&sc->recv_io.posted.count); -if (new_credits ==0 ) + missing = (int )sc->recv_io.credits.target -atomic_read (&sc->recv_io.credits.count); + available =atomic_xchg (&sc->recv_io.credits.available,0 ); + new_credits = (u16 )min3 (U16_MAX , missing, available); +if (new_credits <=0 ) { +/* +* If credits are available, but not granted +* we need to re-add them again. +*/ +if (available) +atomic_add (available, &sc->recv_io.credits.available);return 0 ; + } - new_credits -=atomic_read (&sc->recv_io.credits.count); -if (new_credits <=0 ) -return 0 ; +if (new_credits < available) { +/* +* Readd the remaining available again. +*/ + available -= new_credits; +atomic_add (available, &sc->recv_io.credits.available); + } +/* +* Remember we granted the credits +*/ +atomic_add (new_credits, &sc->recv_io.credits.count);return new_credits; }
@@ -1217,7 +1240,6 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, packet->credits_requested =cpu_to_le16 (sp->send_credit_target); new_credits =manage_credits_prior_sending (sc); -atomic_add (new_credits, &sc->recv_io.credits.count); packet->credits_granted =cpu_to_le16 (new_credits); packet->flags =0 ;
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 9/20 smb: client: let recv_done() queue a refill when the peer is low on credits
In reply to Stefan MetzmacherIn captures I saw that Windows was granting 191 credits in a batch when its peer posted a lot of messages. We are asking for a credit target of 255 and 191 is 252*3/4.
So we also use that logic in order to fill the
recv buffers available to the peer.
--- fs/smb/client/smbdirect.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
fs/smb/client/smbdirect.c +5 -2
@@ -663,6 +663,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc) container_of(wc->wr_cqe, struct smbdirect_recv_io, cqe);struct smbdirect_socket *sc = response->socket;struct smbdirect_socket_parameters *sp = &sc->parameters; +int current_recv_credits;u16 old_recv_credit_target;u32 data_offset =0 ;u32 data_length =0 ;
@@ -747,7 +748,8 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc) }atomic_dec (&sc->recv_io.posted.count); -atomic_dec (&sc->recv_io.credits.count); + current_recv_credits =atomic_dec_return (&sc->recv_io.credits.count); + old_recv_credit_target = sc->recv_io.credits.target; sc->recv_io.credits.target =le16_to_cpu (data_transfer->credits_requested);
@@ -783,7 +785,8 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)* reassembly queue and wake up the reading thread */ if (data_length) { -if (sc->recv_io.credits.target > old_recv_credit_target) +if (current_recv_credits <= (sc->recv_io.credits.target /4 ) || + sc->recv_io.credits.target > old_recv_credit_target)queue_work (sc->workqueue, &sc->recv_io.posted.refill_work);enqueue_reassembly (sc, response, data_length);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 10/20 smb: client: let smbd_post_send() make use of request->wr
In reply to Stefan MetzmacherWe don't need a stack variable in addition.
--- fs/smb/client/smbdirect.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
fs/smb/client/smbdirect.c +7 -8
@@ -1105,7 +1105,6 @@ static int manage_keep_alive_before_sending(struct smbdirect_socket *sc)static int smbd_post_send (struct smbdirect_socket *sc,struct smbdirect_send_io *request) { -struct ib_send_wr send_wr;int rc, i;for (i =0 ; i < request->num_sge; i++) {
@@ -1121,14 +1120,14 @@ static int smbd_post_send(struct smbdirect_socket *sc, request->cqe.done = send_done; - send_wr.next =NULL ; - send_wr.wr_cqe = &request->cqe; - send_wr.sg_list = request->sge; - send_wr.num_sge = request->num_sge; - send_wr.opcode =IB_WR_SEND ; - send_wr.send_flags =IB_SEND_SIGNALED ; + request->wr.next =NULL ; + request->wr.wr_cqe = &request->cqe; + request->wr.sg_list = request->sge; + request->wr.num_sge = request->num_sge; + request->wr.opcode =IB_WR_SEND ; + request->wr.send_flags =IB_SEND_SIGNALED ; - rc =ib_post_send (sc->ib.qp, &send_wr,NULL ); + rc =ib_post_send (sc->ib.qp, &request->wr,NULL );if (rc) {log_rdma_send (ERR ,"ib_post_send failed rc=%d\n" , rc);smbd_disconnect_rdma_connection (sc);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 11/20 smb: client: remove pointless sc->recv_io.credits.count rollback
In reply to Stefan MetzmacherWe either reach this code path before we call new_credits = manage_credits_prior_sending(sc), which means new_credits is still 0
or the connection is already broken as smbd_post_send() already called smbd_disconnect_rdma_connection().
This will also simplify further changes.
--- fs/smb/client/smbdirect.c | 3 --- 1 file changed, 3 deletions(-)
fs/smb/client/smbdirect.c +0 -3
@@ -1288,9 +1288,6 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, DMA_TO_DEVICE);mempool_free (request, sc->send_io.mem.pool); -/* roll back the granted receive credits */ -atomic_sub (new_credits, &sc->recv_io.credits.count); - err_alloc:atomic_inc (&sc->send_io.credits.count);wake_up (&sc->send_io.credits.wait_queue);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 12/20 smb: client: remove pointless sc->send_io.pending handling in smbd_post_send_iter()
In reply to Stefan MetzmacherIf we reach this the connection is already broken as smbd_post_send() already called smbd_disconnect_rdma_connection().
This will also simplify further changes.
--- fs/smb/client/smbdirect.c | 5 ----- 1 file changed, 5 deletions(-)
fs/smb/client/smbdirect.c +0 -5
@@ -1274,11 +1274,6 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,if (!rc)return 0 ; -if (atomic_dec_and_test (&sc->send_io.pending.count)) -wake_up (&sc->send_io.pending.zero_wait_queue); - -wake_up (&sc->send_io.pending.dec_wait_queue); - err_dma:for (i =0 ; i < request->num_sge; i++)if (request->sge[i].addr)
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 13/20 smb: client: port and use the wait_for_credits logic used by server
In reply to Stefan MetzmacherThis simplifies the logic and prepares the use of smbdirect_send_batch in order to make sure all messages in a multi fragment send are grouped together.
We'll add the smbdirect_send_batch processin in a later patch.
--- fs/smb/client/smbdirect.c | 70 ++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 27 deletions(-)
fs/smb/client/smbdirect.c +43 -27
@@ -1137,6 +1137,44 @@ static int smbd_post_send(struct smbdirect_socket *sc,return rc; } +static int wait_for_credits (struct smbdirect_socket *sc, +wait_queue_head_t *waitq,atomic_t *total_credits, +int needed) +{ +int ret; + +do { +if (atomic_sub_return (needed, total_credits) >=0 ) +return 0 ; + +atomic_add (needed, total_credits); + ret =wait_event_interruptible (*waitq, +atomic_read (total_credits) >= needed || + sc->status !=SMBDIRECT_SOCKET_CONNECTED ); + +if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) +return -ENOTCONN ; +else if (ret <0 ) +return ret; + }while (true); +} + +static int wait_for_send_lcredit (struct smbdirect_socket *sc) +{ +return wait_for_credits (sc, + &sc->send_io.lcredits.wait_queue, + &sc->send_io.lcredits.count, +1 ); +} + +static int wait_for_send_credits (struct smbdirect_socket *sc) +{ +return wait_for_credits (sc, + &sc->send_io.credits.wait_queue, + &sc->send_io.credits.count, +1 ); +} +static int smbd_post_send_iter (struct smbdirect_socket *sc,struct iov_iter *iter,int *_remaining_data_length)
@@ -1149,41 +1187,19 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,struct smbdirect_data_transfer *packet;int new_credits =0 ; -wait_lcredit: -/* Wait for local send credits */ - rc =wait_event_interruptible (sc->send_io.lcredits.wait_queue, -atomic_read (&sc->send_io.lcredits.count) >0 || - sc->status !=SMBDIRECT_SOCKET_CONNECTED ); -if (rc) - goto err_wait_lcredit; - -if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) { -log_outgoing (ERR ,"disconnected not sending on wait_credit\n" ); + rc =wait_for_send_lcredit (sc); +if (rc) { +log_outgoing (ERR ,"disconnected not sending on wait_lcredit\n" ); rc = -EAGAIN ; goto err_wait_lcredit; } -if (unlikely (atomic_dec_return (&sc->send_io.lcredits.count) <0 )) { -atomic_inc (&sc->send_io.lcredits.count); - goto wait_lcredit; - } -wait_credit: -/* Wait for send credits. A SMBD packet needs one credit */ - rc =wait_event_interruptible (sc->send_io.credits.wait_queue, -atomic_read (&sc->send_io.credits.count) >0 || - sc->status !=SMBDIRECT_SOCKET_CONNECTED ); -if (rc) - goto err_wait_credit; - -if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) { + rc =wait_for_send_credits (sc); +if (rc) {log_outgoing (ERR ,"disconnected not sending on wait_credit\n" ); rc = -EAGAIN ; goto err_wait_credit; } -if (unlikely (atomic_dec_return (&sc->send_io.credits.count) <0 )) { -atomic_inc (&sc->send_io.credits.count); - goto wait_credit; - } request =mempool_alloc (sc->send_io.mem.pool,GFP_KERNEL );if (!request) {
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 14/20 smb: client: split out smbd_ib_post_send()
In reply to Stefan MetzmacherThis is like smb_direct_post_send() in the server and will simplify porting the smbdirect_send_batch and credit related logic from the server.
--- fs/smb/client/smbdirect.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
fs/smb/client/smbdirect.c +17 -16
@@ -1101,11 +1101,26 @@ static int manage_keep_alive_before_sending(struct smbdirect_socket *sc)return 0 ; } +static int smbd_ib_post_send (struct smbdirect_socket *sc, +struct ib_send_wr *wr) +{ +int ret; + +atomic_inc (&sc->send_io.pending.count); + ret =ib_post_send (sc->ib.qp, wr,NULL ); +if (ret) { +pr_err ("failed to post send: %d\n" , ret); +smbd_disconnect_rdma_connection (sc); + ret = -EAGAIN ; + } +return ret; +} +/* Post the send request */ static int smbd_post_send (struct smbdirect_socket *sc,struct smbdirect_send_io *request) { -int rc, i; +int i;for (i =0 ; i < request->num_sge; i++) { log_rdma_send(INFO,
@@ -1126,15 +1141,7 @@ static int smbd_post_send(struct smbdirect_socket *sc, request->wr.num_sge = request->num_sge; request->wr.opcode =IB_WR_SEND ; request->wr.send_flags =IB_SEND_SIGNALED ; - - rc =ib_post_send (sc->ib.qp, &request->wr,NULL ); -if (rc) { -log_rdma_send (ERR ,"ib_post_send failed rc=%d\n" , rc); -smbd_disconnect_rdma_connection (sc); - rc = -EAGAIN ; - } - -return rc; +return smbd_ib_post_send (sc, &request->wr); } static int wait_for_credits(struct smbdirect_socket *sc,
@@ -1280,12 +1287,6 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,le32_to_cpu (packet->data_length), le32_to_cpu(packet->remaining_data_length)); -/* -* Now that we got a local and a remote credit -* we add us as pending -*/ -atomic_inc (&sc->send_io.pending.count); - rc =smbd_post_send (sc, request);if (!rc)return 0 ;
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 15/20 smb: client: introduce and use smbd_{alloc,free}_send_io()
In reply to Stefan MetzmacherThis is basically a copy of smb_direct_{alloc,free}_sendmsg() in the server, with just using ib_dma_unmap_page() in all cases, which is the same as ib_dma_unmap_single().
We'll use this logic in common code in future.
(I basically backported it from my branch that
as already has everything in common).
--- fs/smb/client/smbdirect.c | 87 ++++++++++++++++++++++++++------------- 1 file changed, 58 insertions(+), 29 deletions(-)
fs/smb/client/smbdirect.c +58 -29
@@ -493,10 +493,54 @@ static inline void *smbdirect_recv_io_payload(struct smbdirect_recv_io *responsereturn (void *)response->packet; } +static struct smbdirect_send_io *smbd_alloc_send_io (struct smbdirect_socket *sc) +{ +struct smbdirect_send_io *msg; + + msg =mempool_alloc (sc->send_io.mem.pool,GFP_KERNEL ); +if (!msg) +return ERR_PTR (-ENOMEM ); + msg->socket = sc; +INIT_LIST_HEAD (&msg->sibling_list); + msg->num_sge =0 ; + +return msg; +} + +static void smbd_free_send_io (struct smbdirect_send_io *msg) +{ +struct smbdirect_socket *sc = msg->socket; +size_t i; + +/* +* The list needs to be empty! +* The caller should take care of it. +*/ +WARN_ON_ONCE (!list_empty (&msg->sibling_list)); + +/* +* Note we call ib_dma_unmap_page(), even if some sges are mapped using +* ib_dma_map_single(). +* +* The difference between _single() and _page() only matters for the +* ib_dma_map_*() case. +* +* For the ib_dma_unmap_*() case it does not matter as both take the +* dma_addr_t and dma_unmap_single_attrs() is just an alias to +* dma_unmap_page_attrs(). +*/ +for (i =0 ; i < msg->num_sge; i++) +ib_dma_unmap_page (sc->ib.dev, + msg->sge[i].addr, + msg->sge[i].length, +DMA_TO_DEVICE ); + +mempool_free (msg, sc->send_io.mem.pool); +} +/* Called when a RDMA send is done */ static void send_done (struct ib_cq *cq,struct ib_wc *wc) { -int i;struct smbdirect_send_io *request = container_of(wc->wr_cqe, struct smbdirect_send_io, cqe);struct smbdirect_socket *sc = request->socket;
@@ -505,12 +549,8 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)log_rdma_send (INFO ,"smbdirect_send_io 0x%p completed wc->status=%s\n" , request,ib_wc_status_msg (wc->status)); -for (i =0 ; i < request->num_sge; i++) -ib_dma_unmap_single (sc->ib.dev, - request->sge[i].addr, - request->sge[i].length, -DMA_TO_DEVICE ); -mempool_free (request, sc->send_io.mem.pool); +/* Note this frees wc->wr_cqe, but not wc */ +smbd_free_send_io (request); lcredits +=1 ;if (wc->status !=IB_WC_SUCCESS || wc->opcode !=IB_WC_SEND ) {
@@ -963,15 +1003,13 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc) {struct smbdirect_socket_parameters *sp = &sc->parameters;struct ib_send_wr send_wr; -int rc = -ENOMEM ; +int rc;struct smbdirect_send_io *request;struct smbdirect_negotiate_req *packet; - request =mempool_alloc (sc->send_io.mem.pool,GFP_KERNEL ); -if (!request) -return rc; - - request->socket = sc; + request =smbd_alloc_send_io (sc); +if (IS_ERR (request)) +return PTR_ERR (request); packet =smbdirect_send_io_payload (request); packet->min_version =cpu_to_le16 (SMBDIRECT_V1 );
@@ -983,7 +1021,6 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc) packet->max_fragmented_size =cpu_to_le32 (sp->max_fragmented_recv_size); - request->num_sge =1 ; request->sge[0 ].addr =ib_dma_map_single ( sc->ib.dev, (void *)packet,sizeof (*packet),DMA_TO_DEVICE );
@@ -991,6 +1028,7 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc) rc = -EIO ; goto dma_mapping_failed; } + request->num_sge =1 ; request->sge[0 ].length =sizeof (*packet); request->sge[0 ].lkey = sc->ib.pd->local_dma_lkey;
@@ -1020,13 +1058,11 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc)/* if we reach here, post send failed */ log_rdma_send (ERR ,"ib_post_send failed rc=%d\n" , rc);atomic_dec (&sc->send_io.pending.count); -ib_dma_unmap_single (sc->ib.dev, request->sge[0 ].addr, - request->sge[0 ].length,DMA_TO_DEVICE );smbd_disconnect_rdma_connection (sc); dma_mapping_failed: -mempool_free (request, sc->send_io.mem.pool); +smbd_free_send_io (request);return rc; }
@@ -1187,7 +1223,7 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, int *_remaining_data_length) {struct smbdirect_socket_parameters *sp = &sc->parameters; -int i, rc; +int rc;int header_length;int data_length;struct smbdirect_send_io *request;
@@ -1208,13 +1244,12 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, goto err_wait_credit; } - request =mempool_alloc (sc->send_io.mem.pool,GFP_KERNEL ); -if (!request) { - rc = -ENOMEM ; + request =smbd_alloc_send_io (sc); +if (IS_ERR (request)) { + rc =PTR_ERR (request); goto err_alloc; } - request->socket = sc;memset (request->sge,0 ,sizeof (request->sge));/* Map the packet to DMA */
@@ -1292,13 +1327,7 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,return 0 ; err_dma: -for (i =0 ; i < request->num_sge; i++) -if (request->sge[i].addr) -ib_dma_unmap_single (sc->ib.dev, - request->sge[i].addr, - request->sge[i].length, -DMA_TO_DEVICE ); -mempool_free (request, sc->send_io.mem.pool); +smbd_free_send_io (request); err_alloc:atomic_inc (&sc->send_io.credits.count);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 16/20 smb: client: use smbdirect_send_batch processing
In reply to Stefan MetzmacherThis will allow us to use similar logic as we have in the server soon, so that we can share common code later.
--- fs/smb/client/smbdirect.c | 149 ++++++++++++++++++++++++++++++++++---- 1 file changed, 135 insertions(+), 14 deletions(-)
fs/smb/client/smbdirect.c +135 -14
@@ -544,11 +544,20 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)struct smbdirect_send_io *request = container_of(wc->wr_cqe, struct smbdirect_send_io, cqe);struct smbdirect_socket *sc = request->socket; +struct smbdirect_send_io *sibling, *next;int lcredits =0 ;log_rdma_send (INFO ,"smbdirect_send_io 0x%p completed wc->status=%s\n" , request,ib_wc_status_msg (wc->status)); +/* +* Free possible siblings and then the main send_io +*/ +list_for_each_entry_safe (sibling, next, &request->sibling_list, sibling_list) { +list_del_init (&sibling->sibling_list); +smbd_free_send_io (sibling); + lcredits +=1 ; + }/* Note this frees wc->wr_cqe, but not wc */ smbd_free_send_io (request); lcredits +=1 ;
@@ -1154,7 +1163,8 @@ static int smbd_ib_post_send(struct smbdirect_socket *sc,/* Post the send request */ static int smbd_post_send (struct smbdirect_socket *sc, -struct smbdirect_send_io *request) +struct smbdirect_send_batch *batch, +struct smbdirect_send_io *request) {int i;
@@ -1170,16 +1180,95 @@ static int smbd_post_send(struct smbdirect_socket *sc, } request->cqe.done = send_done; - request->wr.next =NULL ; - request->wr.wr_cqe = &request->cqe; request->wr.sg_list = request->sge; request->wr.num_sge = request->num_sge; request->wr.opcode =IB_WR_SEND ; + +if (batch) { + request->wr.wr_cqe =NULL ; + request->wr.send_flags =0 ; +if (!list_empty (&batch->msg_list)) { +struct smbdirect_send_io *last; + + last =list_last_entry (&batch->msg_list, + struct smbdirect_send_io, + sibling_list); + last->wr.next = &request->wr; + } +list_add_tail (&request->sibling_list, &batch->msg_list); + batch->wr_cnt++; +return 0 ; + } + + request->wr.wr_cqe = &request->cqe; request->wr.send_flags =IB_SEND_SIGNALED ;return smbd_ib_post_send (sc, &request->wr); } +static void smbd_send_batch_init (struct smbdirect_send_batch *batch, +bool need_invalidate_rkey, +unsigned int remote_key) +{ +INIT_LIST_HEAD (&batch->msg_list); + batch->wr_cnt =0 ; + batch->need_invalidate_rkey = need_invalidate_rkey; + batch->remote_key = remote_key; +} + +static int smbd_send_batch_flush (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch, +bool is_last) +{ +struct smbdirect_send_io *first, *last; +int ret =0 ; + +if (list_empty (&batch->msg_list)) +return 0 ; + + first =list_first_entry (&batch->msg_list, + struct smbdirect_send_io, + sibling_list); + last =list_last_entry (&batch->msg_list, + struct smbdirect_send_io, + sibling_list); + +if (batch->need_invalidate_rkey) { + first->wr.opcode =IB_WR_SEND_WITH_INV ; + first->wr.ex.invalidate_rkey = batch->remote_key; + batch->need_invalidate_rkey = false; + batch->remote_key =0 ; + } + + last->wr.send_flags =IB_SEND_SIGNALED ; + last->wr.wr_cqe = &last->cqe; + +/* +* Remove last from batch->msg_list +* and splice the rest of batch->msg_list +* to last->sibling_list. +* +* batch->msg_list is a valid empty list +* at the end. +*/ +list_del_init (&last->sibling_list); +list_splice_tail_init (&batch->msg_list, &last->sibling_list); + batch->wr_cnt =0 ; + + ret =smbd_ib_post_send (sc, &first->wr); +if (ret) { +struct smbdirect_send_io *sibling, *next; + +list_for_each_entry_safe (sibling, next, &last->sibling_list, sibling_list) { +list_del_init (&sibling->sibling_list); +smbd_free_send_io (sibling); + } +smbd_free_send_io (last); + } + +return ret; +} +static int wait_for_credits (struct smbdirect_socket *sc,wait_queue_head_t *waitq,atomic_t *total_credits,int needed)
@@ -1202,16 +1291,35 @@ static int wait_for_credits(struct smbdirect_socket *sc, }while (true); } -static int wait_for_send_lcredit (struct smbdirect_socket *sc) +static int wait_for_send_lcredit (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch) { +if (batch && (atomic_read (&sc->send_io.lcredits.count) <=1 )) { +int ret; + + ret =smbd_send_batch_flush (sc, batch, false); +if (ret) +return ret; + } +return wait_for_credits (sc, &sc->send_io.lcredits.wait_queue, &sc->send_io.lcredits.count,1 ); } -static int wait_for_send_credits (struct smbdirect_socket *sc) +static int wait_for_send_credits (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch) { +if (batch && + (batch->wr_cnt >=16 ||atomic_read (&sc->send_io.credits.count) <=1 )) { +int ret; + + ret =smbd_send_batch_flush (sc, batch, false); +if (ret) +return ret; + } + return wait_for_credits(sc, &sc->send_io.credits.wait_queue, &sc->send_io.credits.count,
@@ -1219,6 +1327,7 @@ static int wait_for_send_credits(struct smbdirect_socket *sc) }static int smbd_post_send_iter (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch,struct iov_iter *iter,int *_remaining_data_length) {
@@ -1230,14 +1339,14 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,struct smbdirect_data_transfer *packet;int new_credits =0 ; - rc =wait_for_send_lcredit (sc); + rc =wait_for_send_lcredit (sc, batch);if (rc) {log_outgoing (ERR ,"disconnected not sending on wait_lcredit\n" ); rc = -EAGAIN ; goto err_wait_lcredit; } - rc =wait_for_send_credits (sc); + rc =wait_for_send_credits (sc, batch);if (rc) {log_outgoing (ERR ,"disconnected not sending on wait_credit\n" ); rc = -EAGAIN ;
@@ -1322,7 +1431,7 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,le32_to_cpu (packet->data_length), le32_to_cpu(packet->remaining_data_length)); - rc =smbd_post_send (sc, request); + rc =smbd_post_send (sc, batch, request);if (!rc)return 0 ;
@@ -1351,10 +1460,11 @@ static int smbd_post_send_empty(struct smbdirect_socket *sc)int remaining_data_length =0 ; sc->statistics.send_empty++; -return smbd_post_send_iter (sc,NULL , &remaining_data_length); +return smbd_post_send_iter (sc,NULL ,NULL , &remaining_data_length); }static int smbd_post_send_full_iter (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch,struct iov_iter *iter,int *_remaining_data_length) {
@@ -1367,7 +1477,7 @@ static int smbd_post_send_full_iter(struct smbdirect_socket *sc,*/ while (iov_iter_count (iter) >0 ) { - rc =smbd_post_send_iter (sc, iter, _remaining_data_length); + rc =smbd_post_send_iter (sc, batch, iter, _remaining_data_length);if (rc <0 )break ; }
@@ -2289,8 +2399,10 @@ int smbd_send(struct TCP_Server_Info *server,struct smbdirect_socket_parameters *sp = &sc->parameters;struct smb_rqst *rqst;struct iov_iter iter; +struct smbdirect_send_batch batch;unsigned int remaining_data_length, klen;int rc, i, rqst_idx; +int error =0 ;if (sc->status !=SMBDIRECT_SOCKET_CONNECTED )return -EAGAIN ;
@@ -2315,6 +2427,7 @@ int smbd_send(struct TCP_Server_Info *server, num_rqst, remaining_data_length); rqst_idx =0 ; +smbd_send_batch_init (&batch, false,0 ); do { rqst = &rqst_array[rqst_idx];
@@ -2333,20 +2446,28 @@ int smbd_send(struct TCP_Server_Info *server, klen += rqst->rq_iov[i].iov_len;iov_iter_kvec (&iter,ITER_SOURCE , rqst->rq_iov, rqst->rq_nvec, klen); - rc =smbd_post_send_full_iter (sc, &iter, &remaining_data_length); -if (rc <0 ) + rc =smbd_post_send_full_iter (sc, &batch, &iter, &remaining_data_length); +if (rc <0 ) { + error = rc;break ; + }if (iov_iter_count (&rqst->rq_iter) >0 ) {/* And then the data pages if there are any */ - rc =smbd_post_send_full_iter (sc, &rqst->rq_iter, + rc =smbd_post_send_full_iter (sc, &batch, &rqst->rq_iter, &remaining_data_length); -if (rc <0 ) +if (rc <0 ) { + error = rc;break ; + } } }while (++rqst_idx < num_rqst); + rc =smbd_send_batch_flush (sc, &batch, true); +if (unlikely (!rc && error)) + rc = error; +/* * As an optimization, we don't wait for individual I/O to finish * before sending the next one.
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 17/20 smb: client: make use of smbdirect_socket.send_io.bcredits
In reply to Stefan MetzmacherIt turns out that our code will corrupt the stream of reassabled data transfer messages when we trigger an immendiate (empty) send.
In order to fix this we'll have a single 'batch' credit per connection. And code getting that credit is free to use as much messages until remaining_length reaches 0, then the batch credit it given back and the next logical send can happen.
--- fs/smb/client/smbdirect.c | 58 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-)
fs/smb/client/smbdirect.c +55 -3
@@ -657,6 +657,7 @@ static bool process_negotiation_response( sp->max_frmr_depth * PAGE_SIZE); sp->max_frmr_depth = sp->max_read_write_size /PAGE_SIZE ; +atomic_set (&sc->send_io.bcredits.count,1 ); sc->recv_io.expected =SMBDIRECT_EXPECT_DATA_TRANSFER ;return true; }
@@ -1214,6 +1215,7 @@ static void smbd_send_batch_init(struct smbdirect_send_batch *batch, batch->wr_cnt =0 ; batch->need_invalidate_rkey = need_invalidate_rkey; batch->remote_key = remote_key; + batch->credit =0 ; } static int smbd_send_batch_flush(struct smbdirect_socket *sc,
@@ -1224,7 +1226,7 @@ static int smbd_send_batch_flush(struct smbdirect_socket *sc,int ret =0 ;if (list_empty (&batch->msg_list)) -return 0 ; + goto release_credit; first = list_first_entry(&batch->msg_list, struct smbdirect_send_io,
@@ -1266,6 +1268,13 @@ static int smbd_send_batch_flush(struct smbdirect_socket *sc,smbd_free_send_io (last); } +release_credit: +if (is_last && !ret && batch->credit) { +atomic_add (batch->credit, &sc->send_io.bcredits.count); + batch->credit =0 ; +wake_up (&sc->send_io.bcredits.wait_queue); + } +return ret; }
@@ -1291,6 +1300,25 @@ static int wait_for_credits(struct smbdirect_socket *sc, }while (true); } +static int wait_for_send_bcredit (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch) +{ +int ret; + +if (batch->credit) +return 0 ; + + ret =wait_for_credits (sc, + &sc->send_io.bcredits.wait_queue, + &sc->send_io.bcredits.count, +1 ); +if (ret) +return ret; + + batch->credit =1 ; +return 0 ; +} +static int wait_for_send_lcredit (struct smbdirect_socket *sc,struct smbdirect_send_batch *batch) {
@@ -1338,6 +1366,19 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,struct smbdirect_send_io *request;struct smbdirect_data_transfer *packet;int new_credits =0 ; +struct smbdirect_send_batch _batch; + +if (!batch) { +smbd_send_batch_init (&_batch, false,0 ); + batch = &_batch; + } + + rc =wait_for_send_bcredit (sc, batch); +if (rc) { +log_outgoing (ERR ,"disconnected not sending on wait_bcredit\n" ); + rc = -EAGAIN ; + goto err_wait_bcredit; + } rc =wait_for_send_lcredit (sc, batch);if (rc) {
@@ -1432,8 +1473,14 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, le32_to_cpu(packet->remaining_data_length)); rc =smbd_post_send (sc, batch, request); -if (!rc) -return 0 ; +if (!rc) { +if (batch != &_batch) +return 0 ; + + rc =smbd_send_batch_flush (sc, batch, true); +if (!rc) +return 0 ; + } err_dma:smbd_free_send_io (request);
@@ -1447,6 +1494,11 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,wake_up (&sc->send_io.lcredits.wait_queue); err_wait_lcredit: +atomic_add (batch->credit, &sc->send_io.bcredits.count); + batch->credit =0 ; +wake_up (&sc->send_io.bcredits.wait_queue); + +err_wait_bcredit:return rc; }
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 18/20 smb: client: fix last send credit problem causing disconnects
In reply to Stefan MetzmacherWhen we are about to use the last send credit that was granted to us by the peer, we need to wait until we are ourself able to grant at least one credit to the peer. Otherwise it might not be possible for the peer to grant more credits.
The following sections in MS-SMBD are related to this:
3.1.5.1 Sending Upper Layer Messages
...
If Connection.SendCredits is 1 and the CreditsGranted field of the message is 0, stop processing.
...
3.1.5.9 Managing Credits Prior to Sending
...
If Connection.ReceiveCredits is zero, or if Connection.SendCredits is one and the Connection.SendQueue is not empty, the sender MUST allocate and post at least one receive of size Connection.MaxReceiveSize and MUST increment Connection.ReceiveCredits by the number allocated and posted. If no receives are posted, the processing MUST return a value of zero to indicate to the caller that no Send message can be currently performed.
...
This is a similar logic as we have in the server.
--- fs/smb/client/smbdirect.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
fs/smb/client/smbdirect.c +29 -2
@@ -697,6 +697,15 @@ static void smbd_post_send_credits(struct work_struct *work)atomic_add (posted, &sc->recv_io.credits.available); +/* +* If the last send credit is waiting for credits +* it can grant we need to wake it up +*/ +if (posted && +atomic_read (&sc->send_io.bcredits.count) ==0 && +atomic_read (&sc->send_io.credits.count) ==0 ) +wake_up (&sc->send_io.credits.wait_queue); +/* Promptly send an immediate packet as defined in [MS-SMBD] 3.1.1.1 */ if (atomic_read (&sc->recv_io.credits.count) < sc->recv_io.credits.target -1 ) {
@@ -1394,6 +1403,26 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc, goto err_wait_credit; } + new_credits =manage_credits_prior_sending (sc); +if (new_credits ==0 && +atomic_read (&sc->send_io.credits.count) ==0 && +atomic_read (&sc->recv_io.credits.count) ==0 ) { +queue_work (sc->workqueue, &sc->recv_io.posted.refill_work); + rc =wait_event_interruptible (sc->send_io.credits.wait_queue, +atomic_read (&sc->send_io.credits.count) >=1 || +atomic_read (&sc->recv_io.credits.available) >=1 || + sc->status !=SMBDIRECT_SOCKET_CONNECTED ); +if (sc->status !=SMBDIRECT_SOCKET_CONNECTED ) + rc = -ENOTCONN ; +if (rc <0 ) { +log_outgoing (ERR ,"disconnected not sending on last credit\n" ); + rc = -EAGAIN ; + goto err_wait_credit; + } + + new_credits =manage_credits_prior_sending (sc); + } + request =smbd_alloc_send_io (sc);if (IS_ERR (request)) { rc =PTR_ERR (request);
@@ -1448,8 +1477,6 @@ static int smbd_post_send_iter(struct smbdirect_socket *sc,/* Fill in the packet header */ packet->credits_requested =cpu_to_le16 (sp->send_credit_target); - - new_credits =manage_credits_prior_sending (sc); packet->credits_granted =cpu_to_le16 (new_credits); packet->flags =0 ;
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 19/20 smb: client: let smbd_post_send_negotiate_req() use smbd_post_send()
In reply to Stefan MetzmacherThe server has similar logic and it makes sure that request->wr is used instead of a stack struct ib_send_wr send_wr.
This makes sure send_done can see request->wr.send_flags as the next commit will check for IB_SEND_SIGNALED
--- fs/smb/client/smbdirect.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-)
fs/smb/client/smbdirect.c +7 -25
@@ -35,6 +35,10 @@ static void enqueue_reassembly(static struct smbdirect_recv_io *_get_first_reassembly (struct smbdirect_socket *sc); +static int smbd_post_send (struct smbdirect_socket *sc, +struct smbdirect_send_batch *batch, +struct smbdirect_send_io *request); +static int smbd_post_recv (struct smbdirect_socket *sc,struct smbdirect_recv_io *response);
@@ -1021,7 +1025,6 @@ static int smbd_ia_open(static int smbd_post_send_negotiate_req (struct smbdirect_socket *sc) {struct smbdirect_socket_parameters *sp = &sc->parameters; -struct ib_send_wr send_wr;int rc;struct smbdirect_send_io *request;struct smbdirect_negotiate_req *packet;
@@ -1052,33 +1055,12 @@ static int smbd_post_send_negotiate_req(struct smbdirect_socket *sc) request->sge[0 ].length =sizeof (*packet); request->sge[0 ].lkey = sc->ib.pd->local_dma_lkey; -ib_dma_sync_single_for_device ( - sc->ib.dev, request->sge[0 ].addr, - request->sge[0 ].length,DMA_TO_DEVICE ); - - request->cqe.done = send_done; - - send_wr.next =NULL ; - send_wr.wr_cqe = &request->cqe; - send_wr.sg_list = request->sge; - send_wr.num_sge = request->num_sge; - send_wr.opcode =IB_WR_SEND ; - send_wr.send_flags =IB_SEND_SIGNALED ; - -log_rdma_send (INFO ,"sge addr=0x%llx length=%u lkey=0x%x\n" , - request->sge[0 ].addr, - request->sge[0 ].length, request->sge[0 ].lkey); - -atomic_inc (&sc->send_io.pending.count); - rc =ib_post_send (sc->ib.qp, &send_wr,NULL ); + rc =smbd_post_send (sc,NULL , request);if (!rc)return 0 ; -/* if we reach here, post send failed */ -log_rdma_send (ERR ,"ib_post_send failed rc=%d\n" , rc); -atomic_dec (&sc->send_io.pending.count); - -smbd_disconnect_rdma_connection (sc); +if (rc == -EAGAIN ) + rc = -EIO ; dma_mapping_failed:smbd_free_send_io (request);
Signature · 1 line
2.43.0
Stefan Metzmacher
reviewerPATCH v2 20/20 smb: client: let send_done handle a completion without IB_SEND_SIGNALED
In reply to Stefan MetzmacherWith smbdirect_send_batch processing we likely have requests without IB_SEND_SIGNALED, which will be destroyed in the final request that has IB_SEND_SIGNALED set.
If the connection is broken all requests are signaled even without explicit IB_SEND_SIGNALED.
--- fs/smb/client/smbdirect.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
fs/smb/client/smbdirect.c +27 -0
@@ -554,6 +554,32 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)log_rdma_send (INFO ,"smbdirect_send_io 0x%p completed wc->status=%s\n" , request,ib_wc_status_msg (wc->status)); +if (unlikely (!(request->wr.send_flags &IB_SEND_SIGNALED ))) { +/* +* This happens when smbdirect_send_io is a sibling +* before the final message, it is signaled on +* error anyway, so we need to skip +* smbdirect_connection_free_send_io here, +* otherwise is will destroy the memory +* of the siblings too, which will cause +* use after free problems for the others +* triggered from ib_drain_qp(). +*/ +if (wc->status !=IB_WC_SUCCESS ) + goto skip_free; + +/* +* This should not happen! +* But we better just close the +* connection... +*/ +log_rdma_send (ERR , +"unexpected send completion wc->status=%s (%d) wc->opcode=%d\n" , +ib_wc_status_msg (wc->status), wc->status, wc->opcode); +smbd_disconnect_rdma_connection (sc); +return ; + } +/* * Free possible siblings and then the main send_io */
@@ -567,6 +593,7 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc) lcredits +=1 ;if (wc->status !=IB_WC_SUCCESS || wc->opcode !=IB_WC_SEND ) { +skip_free:if (wc->status !=IB_WC_WR_FLUSH_ERR )log_rdma_send (ERR ,"wc->status=%s wc->opcode=%d\n" ,ib_wc_status_msg (wc->status), wc->opcode);
Signature · 1 line
2.43.0