Page MenuHomeFreeBSD

Restore ARC MFU/MRU pressure
ClosedPublic

Authored by slw_zxy.spb.ru on Feb 6 2019, 2:48 PM.

Details

Reviewers
mav
avg
mahrens
Group Reviewers
ZFS
Commits
rS348772: Restore ARC MFU/MRU pressure
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);

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

slw_zxy.spb.ru created this revision.Feb 6 2019, 2:48 PM
mav added a comment.Feb 12 2019, 9:54 PM

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 added a subscriber: lev.Feb 12 2019, 10:36 PM
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 added a comment.Feb 12 2019, 11:01 PM
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.

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 added a comment.Feb 13 2019, 5:53 PM

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.

In D19094#410239, @mav 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 added a comment.Feb 14 2019, 12:27 PM

Let me try to contact George again.

ae added a subscriber: ae.Feb 18 2019, 5:12 PM
In D19094#410508, @avg wrote:

Let me try to contact George again.

Do you succesefull contact George?

avg added a comment.Feb 21 2019, 5:36 PM

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!

In D19094#412736, @avg 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 added a comment.Mar 11 2019, 9:39 AM

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.

In D19094#418092, @avg 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?

ae added a reviewer: ZFS.Apr 9 2019, 4:16 PM
avg added a comment.Jun 6 2019, 11:08 AM

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?

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]

avg accepted this revision.Jun 7 2019, 6:08 AM
This revision is now accepted and ready to land.Jun 7 2019, 6:08 AM
This revision was automatically updated to reflect the committed changes.