On Fri, Sep 04, 2020 at 08:59:29PM +0800, Gao Xiang wrote:
> Hi Brian,
>
> On Fri, Sep 04, 2020 at 07:25:48AM -0400, Brian Foster wrote:
> > On Fri, Sep 04, 2020 at 04:25:16PM +0800, Gao Xiang wrote:
>
> ...
>
> > >
> > > +static inline int xlog_logrecv2_hblks(struct xlog_rec_header *rh)
> > > +{
> > > + int h_size = be32_to_cpu(rh->h_size);
> > > +
> > > + if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) &&
> > > + h_size > XLOG_HEADER_CYCLE_SIZE)
> > > + return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
> > > + return 1;
> > > +}
> > > +
> > > +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh)
> > > +{
> > > + if (!xfs_sb_version_haslogv2(&log->l_mp->m_sb))
> > > + return 1;
> > > + return xlog_logrecv2_hblks(rh);
> > > +}
> > > +
> >
> > h_version is assigned based on xfs_sb_version_haslogv2() in the first
> > place so I'm not sure I see the need for multiple helpers like this, at
> > least with the current code. I can't really speak to why some code
> > checks the feature bit and/or the record header version and not the
> > other way around, but perhaps there's some historical reason I'm not
> > aware of. Regardless, is there ever a case where
> > xfs_sb_version_haslogv2() == true and h_version != 2? That strikes me as
> > more of a corruption scenario than anything..
>
> Thanks for this.
>
> Honestly, I'm not quite sure if xfs_sb_version_haslogv2() == true and
> h_version != 2 is useful (or existed historially)... anyway, that is
> another seperate topic though...
>
Indeed.
> 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?
Brian
> Thanks in advance!
>
> Thanks,
> Gao Xiang
>
>
> >
> > Brian
>
