Page MenuHomeFreeBSD

Restore ARC MFU/MRU pressure
ClosedPublic

Authored by slw_zxy.spb.ru on Feb 6 2019, 2:48 PM.
Tags
Referenced Files
F106179180: D19094.diff
Thu, Dec 26, 4:59 PM
F106170455: D19094.id53623.diff
Thu, Dec 26, 1:22 PM
Unknown Object (File)
Thu, Dec 12, 2:24 AM
Unknown Object (File)
Mon, Dec 9, 3:43 AM
Unknown Object (File)
Thu, Dec 5, 8:13 PM
Unknown Object (File)
Wed, Dec 4, 6:03 AM
Unknown Object (File)
Nov 22 2024, 4:27 AM
Unknown Object (File)
Oct 19 2024, 9:29 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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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?

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.

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.

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().

Let me try to contact George again.

In D19094#410508, @avg wrote:

Let me try to contact George again.

Do you succesefull contact George?

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?

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?

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]

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.