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

From: Brian Foster <bfoster@redhat.com>
To: Gao Xiang <hsiangkao@redhat.com>
Cc: linux-xfs@vger.kernel.org, "Darrick J . Wong" <darrick.wong@oracle.com>
Subject: Re: [PATCH v2 2/2] xfs: clean up calculation of LR header blocks
Date: Fri, 4 Sep 2020 09:37:21 -0400	[thread overview]
Message-ID: <20200904133721.GE529978@bfoster> (raw)
In-Reply-To: <20200904125929.GB28752@xiangao.remote.csb>

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
> 

