The comment caught my attention - is that accurate? When a channel
has fewer than 3 credits (assuming we had two reserved for oplock and
echo), that doesn't mean that there are fewer than 3 credits in flight
- just that current credits available is lower right? So the channel
could still recover as long as current credits + credits in flight is
at least 3.
+/*
+ * Min number of credits for a channel to be picked.
+ *
+ * Note that once a channel reaches this threshold it will never be
+ * picked again as no credits can be requested from it.
+ */
+#define CIFS_CHANNEL_MIN_CREDITS 3
On Fri, Mar 5, 2021 at 8:24 AM Aurélien Aptel <aaptel@xxxxxxxx> wrote:
>
> From: Aurelien Aptel <aaptel@xxxxxxxx>
>
> Check channel credits to prevent the client from using a starved
> channel that cannot send anything.
>
> Special care must be taken in selecting the minimum value: when
> channels are created they start off with a small amount that slowly
> ramps up as the channel gets used. Thus a new channel might never be
> picked if the min value is too small.
>
> Signed-off-by: Aurelien Aptel <aaptel@xxxxxxxx>
> ---
> fs/cifs/transport.c | 57 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index e90a1d1380b0..7bb1584b3724 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -44,6 +44,14 @@
> /* Max number of iovectors we can use off the stack when sending requests. */
> #define CIFS_MAX_IOV_SIZE 8
>
> +/*
> + * Min number of credits for a channel to be picked.
> + *
> + * Note that once a channel reaches this threshold it will never be
> + * picked again as no credits can be requested from it.
> + */
> +#define CIFS_CHANNEL_MIN_CREDITS 3
> +
> void
> cifs_wake_up_task(struct mid_q_entry *mid)
> {
> @@ -1051,20 +1059,53 @@ cifs_cancelled_callback(struct mid_q_entry *mid)
> struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses)
> {
> uint index = 0;
> + struct TCP_Server_Info *s;
>
> if (!ses)
> return NULL;
>
> - if (!ses->binding) {
> - /* round robin */
> - if (ses->chan_count > 1) {
> - index = (uint)atomic_inc_return(&ses->chan_seq);
> - index %= ses->chan_count;
> - }
> - return ses->chans[index].server;
> - } else {
> + if (ses->binding)
> return cifs_ses_server(ses);
> +
> + /*
> + * Channels are created right after the session is made. The
> + * count cannot change after that so it is not racy to check.
> + */
> + if (ses->chan_count == 1)
> + return ses->chans[index].server;
> +
> + /* round robin */
> + index = (uint)atomic_inc_return(&ses->chan_seq);
> + index %= ses->chan_count;
> + s = ses->chans[index].server;
> +
> + /*
> + * Checking server credits is racy, but getting a slightly
> + * stale value should not be an issue here
> + */
> + if (s->credits <= CIFS_CHANNEL_MIN_CREDITS) {
> + uint i;
> +
> + cifs_dbg(VFS, "cannot pick conn_id=0x%llx not enough credits (%u)",
> + s->conn_id,
> + s->credits);
> +
> + /*
> + * Look at all other channels starting from the next
> + * one and pick first possible channel.
> + */
> + for (i = 1; i < ses->chan_count; i++) {
> + s = ses->chans[(index+i) % ses->chan_count].server;
> + if (s->credits > CIFS_CHANNEL_MIN_CREDITS)
> + return s;
> + }
> }
> +
> + /*
> + * If none are possible, keep the initially picked one, but
> + * later on it will block to wait for credits or fail.
> + */
> + return ses->chans[index].server;
> }
>
> int
> --
> 2.30.0
>
--
Thanks,
Steve
