Page MenuHomeFreeBSD

Restore ARC MFU/MRU pressure
Needs ReviewPublic

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

Details

Reviewers
mav
avg
mahrens
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
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

slw_zxy.spb.ru created this revision.Wed, Feb 6, 2:48 PM
mav added a comment.Tue, Feb 12, 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.Tue, Feb 12, 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.Tue, Feb 12, 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.Wed, Feb 13, 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.Thu, Feb 14, 12:27 PM

Let me try to contact George again.

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

Let me try to contact George again.

Do you succesefull contact George?

avg added a comment.Thu, Feb 21, 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!