On Tue, Aug 18, 2020 at 04:59:59PM -0700, Darrick J. Wong wrote:
...
> > + bucket_index = 0;
> > + /* During recovery, the old multiple bucket index can be applied */
> > + if (!log || log->l_flags & XLOG_RECOVERY_NEEDED) {
>
> Does the flag test need parentheses?
Yeah, that would be better.
>
> It feels a little funny that we pass in old_agino (having gotten it from
> agi_unlinked) and then compare it with agi_unlinked, but as the commit
> log points out, I guess this is a wart of having to support the old
> unlinked list behavior. It makes sense to me that if we're going to
> change the unlinked list behavior we could be a little more careful
> about double-checking things.
>
> Question: if a newer kernel crashes with a super-long unlinked list and
> the fs gets recovered on an old kernel, will this lead to insanely high
> recovery times? I think the answer is no, because recovery is single
> threaded and the hash only existed to reduce AGI contention during
> normal unlinking operations?
btw, if my understanding is correct, as I mentioned starting from my v1,
this new feature isn't forward compatible since old kernel hardcode
agino % XFS_AGI_UNLINKED_BUCKETS but not tracing original bucket_index
from its logging recovery code. So yeah, a bit awkward from its original
design...
Thanks,
Gao Xiang
