Re: [PATCH v2 2/2] xfs: clean up calculation of LR header blocks

[Brian Foster <bfoster@redhat.com>]

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
> > > 
> > 
>