In D19369#416253, @trasz wrote:

Regarding the message - why? I quite like the current one, because it tells you exactly what you need to do, instead of having to figure it out.

I agree, zero should be the right answer here. That is what kernel does also.

I have no objections, but I have no documentation to know what that coefficient being set means.

I think generally it may be not a bad idea, but I'd prefer less exact message, like "Consider loading ispfw kernel module for known good firmware".

I have no more objections.

Any more comments?

Fix few things noticed by cem@.

In D19231#412056, @cem wrote:

How much testing have you been able to perform with this patch?

In D19094#410192, @slw_zxy.spb.ru wrote:

Don't sure about calling remove_reference() from arc_hdr_alloc_pabd() (or from parallel tasks), but see at ARC MFU/MRU size calculation in arc_change_state() called from arc_access() and !GHOST_STATE(state) case in arc_get_data_impl() called from arc_hdr_alloc_pabd().
I mean interchange this lines can cause problems for this accountings.

In D19094#410076, @lev wrote:

Huh? As far as I can see, problem not remove_reference(), but arc_adapt() called too late, with header in wrong state (promoted from something-ghost to live LRU), which brraks main idea of ARC adaptation.

While I see the problem you are fixing, the fix looks ugly to me, that is why I would look for something nicer. I agree that according to logic of remove_reference() dropping last reference for header in ghost state is a failure, but how can remove_reference() be called before the arc_access() just on following line? I would guess from description telling about the case of prefetch read it should happen no sooner then we actually initiate the I/O, which is done much later then those two lines. So while I agree it is somewhat odd to have buffer for header in ghost state, is that a criminal.

In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is stil a valid value (?).

Generally I agree with it. I spotted it too when worked on my change. It just needs 8 times more RAM then what we have now to overflow, so I preferred my patch to be minimal. Just going that way, why not make uh_hashmask also unsigned? Signed bit mask makes even less sense. You've changed hval from int in one place, but not other(s).

Yes, please make a patch to split allocation and programming.

I think I would remove this code completely, together with above NULL assignments. One way or another, this check is at least incomplete, since just in if block before execution can go to out bypassing this check.

It looks like a workaround on workaround, but considering it is deep legacy, so be it.

Looks good to me.