Re: [PATCH] cifs: try to pick channel with a minimum of credits

["Aurélien Aptel" <aaptel@suse.com>]

Hi Shyam,

Thanks for the review. I also realized we cannot make this failproof so
I went in with the idea to just avoid picking easy, non-confusing cases
of unusable channels. If that's not good we can drop the patch all
together.

Shyam Prasad N <nspmangalore@gmail.com> writes:
> 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.

Yes you're right, it won't prevent failing if more credits are
needed. The patch wasn't meant to be failproof, just to avoid most
occurences of the problem. It's a simple sanity check with some
false-positives and false-negatives.

> 2. Echo and oplock use 1 reserved credit each, which the regular
> operations can steal, in case of shortage.

There's a dedicated server->echo_credit I think.

> 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

If we are just doing a read on an aligned int, at least on x86 you will
get either a stale value or the new value, you cannot get a garbage mix
of both. Which CPU architecture doesn't provide cache coherency at that
level? This is a complex question I couldn't find an easy answer to.

In any case cifs.ko is already assuming it's valid in various places. We
are doing it for some usage of the server->tcpStatus, ses->status,
tcon->tidStatus at least.

The problems of atomic read in pick_channel() aside, the credits might
change from another process between the moment the channel is picked and
the moment the credits needed are spent (server->credit -= XYZ). In
which case it will also EDEADLK later on.

> 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?

Some code relies on server->* values so if you swap the server pointer
it at the last moment I'm not sure those values will match the new
server ptr.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)