Yes, if we change atime to be a timestamp, we also need to change the code for the case when the object is not in any hit sets. -----Original Message----- From: 池信泽 [mailto:xmdxcxz@xxxxxxxxx] Sent: Tuesday, October 28, 2014 12:46 PM To: Sage Weil; Wang, Zhiqiang; ceph-devel@xxxxxxxxxxxxxxx Subject: Re: Cache pool bug? If it changed to be *atime = p->first we should change ReplicatedPG::agent_maybe_evict if (atime < 0 && obc->obs.oi.mtime != utime_t()) { if (obc->obs.oi.local_mtime != utime_t()) { atime = obc->obs.oi.local_mtime; } else { atime = obc->obs.oi.mtime; } } if (atime < 0) atime = 0; // or other void ReplicatedPG::agent_estimate_atime_temp(const hobject_t& oid, int *atime, int *temp) { assert(hit_set); time_t now = ceph_clock_now(NULL).sec(); *atime = -1; if (temp) *temp = 0; if (hit_set->contains(oid)) { *atime = now; if (temp) ++(*temp); else return; } for (map<time_t,HitSetRef>::iterator p = agent_state->hit_set_map.rbegin(); p != agent_state->hit_set_map.rend(); ++p) { if (p->second->contains(oid)) { if (*atime < 0) *atime = now - p->first; if (temp) ++(*temp); else return; } } } 2014-10-28 12:36 GMT+08:00 池信泽 <xmdxcxz@xxxxxxxxx>: > I am sorry. We should only just modify the logic > ReplicatedPG::agent_estimate_atime_temp, atime should be now. > > if (temp) > *temp = 0; > if (hit_set->contains(oid)) { > *atime = 0; > if (temp) > ++(*temp); > else > return; > } > > 2014-10-28 12:28 GMT+08:00 池信泽 <xmdxcxz@xxxxxxxxx>: >> I think if it changed to be *atime = p->first, Below logic should >> also be modified. >> >> ReplicatedPG::agent_maybe_evict >> >> if (atime < 0 && obc->obs.oi.mtime != utime_t()) { >> if (obc->obs.oi.local_mtime != utime_t()) { >> atime = ceph_clock_now(NULL).sec() - obc->obs.oi.local_mtime; >> } else { >> atime = ceph_clock_now(NULL).sec() - obc->obs.oi.mtime; >> } >> } >> >> >> including >> ReplicatedPG::agent_estimate_atime_temp >> if (temp) >> *temp = 0; >> if (hit_set->contains(oid)) { >> *atime = 0; >> if (temp) >> ++(*temp); >> else >> return; >> } >> >> 2014-10-28 12:02 GMT+08:00 Sage Weil <sage@xxxxxxxxxxxx>: >>> Yep. The pull request looks is correct too, but needs to be >>> squashed into a single commit with a Signed-off-by: line and description. >>> >>> Zhiqiang, I think the reversed logic you just fixed in >>> agent_maybe_evict() was partly because this function is filling in >>> atime (which sounds like a >>> timestamp) with an *age* (now - p->first). I think we should change >>> >>>> if (*atime < 0) >>>> *atime = now - p->first; >>> >>> to be >>> >>> *atime = p->first >>> >>> and/or adjust the caller accordingly...? >>> >>> sage ��.n��������+%������w��{.n����z��u���ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
