On Fri, Sep 04, 2020 at 11:07:30PM +0800, Gao Xiang wrote:
> On Fri, Sep 04, 2020 at 09:37:21AM -0400, Brian Foster wrote:
> > On Fri, Sep 04, 2020 at 08:59:29PM +0800, Gao Xiang wrote:
> ...
> > > Could you kindly give me some code flow on your preferred way about
> > > this so I could update this patch proper (since we have a complex
> > > case in xlog_do_recovery_pass(), I'm not sure how the unique helper
> > > will be like because there are 3 cases below...)
> > >
> > > - for the first 2 cases, we already have rhead read in-memory,
> > > so the logic is like:
> > > ....
> > > log_bread (somewhere in advance)
> > > ....
> > >
> > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> > > ...
> > > } else {
> > > ...
> > > }
> > > (so I folded this two cases in xlog_logrec_hblks())
> > >
> > > - for xlog_do_recovery_pass, it behaves like
> > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> > > log_bread (another extra bread to get h_size for
> > > allocated buffer and hblks).
> > >
> > > ...
> > > } else {
> > > ...
> > > }
> > > so in this case we don't have rhead until
> > > xfs_sb_version_haslogv2(&log->l_mp->m_sb) is true...
> > >
> >
> > I'm not sure I'm following the problem...
> >
> > The current patch makes the following change in xlog_do_recovery_pass():
> >
> > @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass(
> > if (error)
> > goto bread_err1;
> >
> > - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) &&
> > - (h_size > XLOG_HEADER_CYCLE_SIZE)) {
> > - hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
> > - if (h_size % XLOG_HEADER_CYCLE_SIZE)
> > - hblks++;
> > + hblks = xlog_logrecv2_hblks(rhead);
> > + if (hblks != 1) {
> > kmem_free(hbp);
> > hbp = xlog_alloc_buffer(log, hblks);
> > - } else {
> > - hblks = 1;
> > }
> > } else {
> > ASSERT(log->l_sectBBsize == 1);
> >
> > My question is: why can't we replace the xlog_logrecv2_hblks() call here
> > with xlog_logrec_hblks()? We already have rhead as the existing code is
> > already looking at h_version. We're inside a _haslogv2() branch, so the
> > check inside the helper is effectively a duplicate/no-op.. Hm?
>
> Yeah, I get your point. That would introduce a duplicated check of
> _haslogv2() if we use xlog_logrec_hblks() here (IMHO compliers wouldn't
> treat the 2nd _haslogv2() check as no-op).
>
Yeah, I meant it as more as a logical no-op. IOW, it wouldn't affect
functionality. The check instructions might be duplicated, but I doubt
that would measurably impact log recovery.
> I will go forward like this if no other concerns. Thank you!
>
Thanks.
Brian
> Thanks,
> Gao Xiang
>
> >
> > Brian
> >
> > > Thanks in advance!
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > >
> > > >
> > > > Brian
> > >
> >
>
