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).
I will go forward like this if no other concerns. Thank you!
Thanks,
Gao Xiang
>
> Brian
>
> > Thanks in advance!
> >
> > Thanks,
> > Gao Xiang
> >
> >
> > >
> > > Brian
> >
>
