Re: [PATCH 2/2] smb: client: let smbd_post_send_iter() respect the peers max_send_size and transmit all data

[Stefan Metzmacher <metze@samba.org>]

Hi Tom,

> On 6/18/2025 12:51 PM, Stefan Metzmacher wrote:
>> We should not send smbdirect_data_transfer messages larger than
>> the negotiated max_send_size, typically 1364 bytes, which means
>> 24 bytes of the smbdirect_data_transfer header + 1340 payload bytes.
>>
>> This happened when doing an SMB2 write with more than 1340 bytes
>> (which is done inline as it's below rdma_readwrite_threshold).
>>
>> It means the peer resets the connection.
> 
> Obviously needs fixing but I'm unclear on the proposed change.
> See below.
> 
>> Note for stable sp->max_send_size needs to be info->max_send_size:
> 
> So this is important and maybe needs more than this comment, which
> is not really something that should go upstream since future stable
> kernels won't apply. Recommend deleting this and sending a separate
> patch.

I can skip it, but I think it might be very useful for
the one who needs to do the conflict resolution for the backport.

Currently master is the only branch that has 'sp->',
so all current backports will need the change.

@Steve what would you prefer? Should I remove the hint and
conflict resolution diff? In the past I saw something similar
in merge requests send to Linus in order to make it easier for
him to resolve the git conflicts.

As it's preferred to backport fixes from master, I don't
think it's a good idea to send a separate patch for the backports.

>>    @@ -895,7 +895,7 @@ static int smbd_post_send_iter(struct smbd_connection *info,
>>                            .direction      = DMA_TO_DEVICE,
>>                    };
>>                    size_t payload_len = min_t(size_t, *_remaining_data_length,
>>    -                                          sp->max_send_size - sizeof(*packet));
>>    +                                          info->max_send_size - sizeof(*packet));
>>
>>                    rc = smb_extract_iter_to_rdma(iter, payload_len,
>>                                                  &extract);
>>
>> cc: Steve French <sfrench@samba.org>
>> cc: David Howells <dhowells@redhat.com>
>> cc: Tom Talpey <tom@talpey.com>
>> cc: linux-cifs@vger.kernel.org
>> Fixes: 3d78fe73fa12 ("cifs: Build the RDMA SGE list directly from an iterator")
>> Signed-off-by: Stefan Metzmacher <metze@samba.org>
>> ---
>>   fs/smb/client/smbdirect.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
>> index cbc85bca006f..3a41dcbbff81 100644
>> --- a/fs/smb/client/smbdirect.c
>> +++ b/fs/smb/client/smbdirect.c
>> @@ -842,7 +842,7 @@ static int smbd_post_send(struct smbd_connection *info,
>>   static int smbd_post_send_iter(struct smbd_connection *info,
>>                      struct iov_iter *iter,
>> -                   int *_remaining_data_length)
>> +                   unsigned int *_remaining_data_length)
>>   {
>>       struct smbdirect_socket *sc = &info->socket;
>>       struct smbdirect_socket_parameters *sp = &sc->parameters;
>> @@ -907,8 +907,10 @@ static int smbd_post_send_iter(struct smbd_connection *info,
>>               .local_dma_lkey    = sc->ib.pd->local_dma_lkey,
>>               .direction    = DMA_TO_DEVICE,
>>           };
>> +        size_t payload_len = min_t(size_t, *_remaining_data_length,
>> +                       sp->max_send_size - sizeof(*packet));
>> -        rc = smb_extract_iter_to_rdma(iter, *_remaining_data_length,
>> +        rc = smb_extract_iter_to_rdma(iter, payload_len,
>>                             &extract);
>>           if (rc < 0)
>>               goto err_dma;
>> @@ -970,8 +972,16 @@ static int smbd_post_send_iter(struct smbd_connection *info,
>>       request->sge[0].lkey = sc->ib.pd->local_dma_lkey;
>>       rc = smbd_post_send(info, request);
>> -    if (!rc)
>> +    if (!rc) {
>> +        if (iter && iov_iter_count(iter) > 0) {
>> +            /*
>> +             * There is more data to send
>> +             */
>> +            goto wait_credit;
> 
> But, shouldn't the caller have done this overflow check, and looped on
> the fragments and credits? It seems wrong to push the credit check down
> to this level.

At least for the caller I guess we want a function that sends
the whole iter and smbd_post_send_iter() only gets the iter as argument
with an implicit length.

To avoid this 'goto wait_credit', we could something like this in
the caller:

  fs/smb/client/smbdirect.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 3a41dcbbff81..e0ba9395ff42 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -2042,17 +2042,24 @@ 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_iter(info, &iter, &remaining_data_length);
+		while (iov_iter_count(&iter) > 0) {
+			rc = smbd_post_send_iter(info, &iter,
+						 &remaining_data_length);
+			if (rc < 0)
+				break;
+		}
  		if (rc < 0)
  			break;

-		if (iov_iter_count(&rqst->rq_iter) > 0) {
+		while (iov_iter_count(&rqst->rq_iter) > 0) {
  			/* And then the data pages if there are any */
  			rc = smbd_post_send_iter(info, &rqst->rq_iter,
  						 &remaining_data_length);
  			if (rc < 0)
  				break;
  		}
+		if (rc < 0)
+			break;

  	} while (++rqst_idx < num_rqst);


But to me that also doesn't look pretty.

Or we rename the current smbd_post_send_iter() to smbd_post_send_iter_chunk()
and implement smbd_post_send_iter() as a loop over smbd_post_send_iter_chunk().

I think currently we want a small patch to actually fix the regression.

>> +        }
>> +
>>           return 0;
>> +    }
>>   err_dma:
>>       for (i = 0; i < request->num_sge; i++)
>> @@ -1007,7 +1017,7 @@ static int smbd_post_send_iter(struct smbd_connection *info,
>>    */
>>   static int smbd_post_send_empty(struct smbd_connection *info)
>>   {
>> -    int remaining_data_length = 0;
>> +    unsigned int remaining_data_length = 0;
> 
> Does this fix something??

I guess if I use umin() (as proposed by David) we don't strictly need that
change.

So I'd prefer to go with skipping the int vs unsigned change and use
umin() and keep the of the patch as is.

Thanks!
metze