Spent some time in this code path. Seems like this is more complicated
than I initially thought.
@Aurélien Aptel A few things to consider:
1. What if the credits that will be needed by the request is more than
3 (or any constant). We may still end up returning -EDEADLK to the
user when we don't find enough credits, and there are not enough
in_flight to satisfy the request. Ideally, we should still try other
channels.
2. Echo and oplock use 1 reserved credit each, which the regular
operations can steal, in case of shortage.
3. Reading server->credits without a lock can result in wildly
different values, since some CPU architectures may not update it
atomically. At worse, we may select a channel which may not have
enough credits when we get to it.
What if we do this...
wait_for_compound_request() can return -EDEADLK when it doesn't get
enough credits, and there are no requests in_flight.
In compound_send_recv(), after we call wait_for_compound_request(), we
can check it's return value. If it's -EDEADLK, we keep calling
cifs_pick_another_channel(ses, bitmask) (bitmask has bits set for
channels already selected and rejected) and
wait_for_compound_request() in a loop till we find a channel which has
enough credits, or we run out of channels; in which case we can return
-EDEADLK.
Makes sense? Do you see a problem with that approach?
Regards,
Shyam
On Fri, Mar 5, 2021 at 9:40 PM Steve French <smfrench@xxxxxxxxx> wrote:
>
> 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
--
Regards,
Shyam
