Restore ARC MFU/MRU pressure

[slw_zxy.spb.ru]

Summary

Before r305323 commit (MFV r302991: 6950 ARC should cache compressed data) in arc_read() do next sequence (access to ghost buffer):

1. arc_adapt() (from arc_get_data_buf())
2. arc_access(hdr, hash_lock)

i.e. first check access to MFU ghost/MRU ghost buffer and addapt MFU/MRU sizes (in arc_adapt()) and next move buffer from ghost state to regular.

After r305323 sequence is differ:

1. arc_access(hdr, hash_lock);
2. arc_hdr_alloc_pabd(hdr);

i.e. first move buffer from ghost state in arc_access() and next check access to buffer in ghost state (in arc_hdr_alloc_pabd()->arc_get_data_abd()->arc_get_data_impl()->arc_adapt()).
This is incorrect: arc_adapt() never see access to ghost buffer because arc_access() already migrate buffer from ghost state to regular.

Becaues cheange order can cause assert (see comment before this code) implement conditional call to arc_adapt() from arc_hdr_alloc_pabd() and call arc_adapt() before arc_access()

arc_adapt(arc_hdr_size(hdr), hdr->b_l1hdr.b_state);
arc_access(hdr, hash_lock);
arc_hdr_alloc_pabd(hdr, B_FALSE);

[mav]

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.

Have you tried to ask for feedback from George Wilson, the author of the change?

[lev]

In D19094#410062, @mav wrote:

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.

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.

[mav]

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.

Right, but I am trying to understand motivation of the order change by George Wilson as part of compressed ARC commit, hoping it could be somehow reverted rather then workarounded.

[slw_zxy.spb.ru]

In D19094#410062, @mav wrote:

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.

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.

Have you tried to ask for feedback from George Wilson, the author of the change?

I am wrote to George Wilson about this issuse Jan 10, no answer.

[mav]

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.

I am not sure what accounting problem you are talking about, but if you take a look into FreeBSD 9 before compressed ARC, arc_read() there calls arc_get_data_buf() before arc_access(), and I see the same !GHOST_STATE(state) case in ^arc_get_data_buf(), so I suppose having buffers in ghost state was not denied then. This is not my favorite part of ZFS, so I don't have all of it in my head, while have no week of time to spend there, sorry.

[slw_zxy.spb.ru]

In D19094#410239, @mav wrote:

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.

I am not sure what accounting problem you are talking about,

Tracking size of ARC MFU, MRU, MFU ghost and etc.

but if you take a look into FreeBSD 9 before compressed ARC, arc_read() there calls arc_get_data_buf() before arc_access(), and I see the same !GHOST_STATE(state) case in ^arc_get_data_buf(), so I suppose having buffers in ghost state was not denied then. This is not my favorite part of ZFS, so I don't have all of it in my head, while have no week of time to spend there, sorry.

And with different arc_change_state().

[avg]

Let me try to contact George again.

[slw_zxy.spb.ru]

In D19094#410508, @avg wrote:

Let me try to contact George again.

Do you succesefull contact George?

[avg]

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

Do you succesefull contact George?

I've just got a reply from George.
He agrees with your analysis, but needs some more time to think about how to address the issue.
Let's wait a bit more.
Thanks!

[slw_zxy.spb.ru]

In D19094#412736, @avg wrote:

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

Do you succesefull contact George?

I've just got a reply from George.
He agrees with your analysis, but needs some more time to think about how to address the issue.
Let's wait a bit more.
Thanks!

No more replays?

[avg]

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

No more replays?

Unfortunately, no.
I think that we can commit your proposed change. If George comes up with a different and better solution later on, there should be no problem switching to it.

[slw_zxy.spb.ru]

In D19094#418092, @avg wrote:

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

No more replays?

Unfortunately, no.
I think that we can commit your proposed change. If George comes up with a different and better solution later on, there should be no problem switching to it.

I am agree.
Can you commit?

[avg]

Sorry, I myself went missing for a long while.
Yes, I can commit this change.

Do you want anything specific to appear in a commit message?
Like any additional attributions, etc?

[slw_zxy.spb.ru]

In D19094#443654, @avg wrote:

Sorry, I myself went missing for a long while.
Yes, I can commit this change.

Do you want anything specific to appear in a commit message?
Like any additional attributions, etc?

Nice!
Sponsored by: Integros [integros.com]