On Thu, Jul 15, 2021 at 09:00:57AM +1000, Dave Chinner wrote:
> On Wed, Jul 14, 2021 at 03:44:50PM -0700, Darrick J. Wong wrote:
> > On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > >
> > > The verifier checks explicitly for bp->b_bn == XFS_SB_DADDR to match
> > > the primary superblock buffer, but the primary superblock is an
> > > uncached buffer and so bp->b_bn is always -1ULL. Hence this never
> > > matches and the CRC error reporting is wholly dependent on the
> > > mount superblock already being populated so CRC feature checks pass
> > > and allow CRC errors to be reported.
> > >
> > > Fix this so that the primary superblock CRC error reporting is not
> > > dependent on already having read the superblock into memory.
> > >
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_sb.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index 04f5386446db..4a4586bd2ba2 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -636,7 +636,7 @@ xfs_sb_read_verify(
> > >
> > > if (!xfs_buf_verify_cksum(bp, XFS_SB_CRC_OFF)) {
> > > /* Only fail bad secondaries on a known V5 filesystem */
> > > - if (bp->b_bn == XFS_SB_DADDR ||
> > > + if (bp->b_maps[0].bm_bn == XFS_SB_DADDR ||
> >
> > I did not know that b_bn only applies to cached buffers.
> >
> > Would you mind ... I dunno, updating the comment in the struct xfs_buf
> > declaration to make this clearer?
> >
> > /*
> > * Block number of buffer, when this buffer is cached. For
> > * uncached buffers, only the buffer map (i.e. b_maps[0].bm_bn)
> > * contains the block number.
> > */
> > xfs_daddr_t b_bn;
>
> Even that isn't stating the whole truth. b_maps[0].bm_bn contains
> the current daddr for all buffers, regardless of whether they are
> cached or not. This is what the IO path uses to provide the physical
> address for the bio we submit...
<nod>
> Looking at the kernel code, there is still a lot of users of
> bp->b_bn, and we've still got inconsistent use of XFS_BUF_ADDR, too.
> I think fixing this all up needs a separate patchset - there's relatively
> little outside libxfs/ in userspace that uses bp->b_bn directly
> (largely repair, which is a 50/50 mix of b_bn and XFS_BUF_ADDR(bp))
> so we should be able to clean this up entirely.
>
> I suspect that converting all the external users to a
> xfs_buf_daddr(bp) helper is the way to go here, and then renaming
> b_bn to something else and use it only in xfs_buf.c for cache
> indexing...
Agreed.
> I'll put together another patchset to clean this up - it's separate
> to the mount features rework, and this patch is only here because
> when I change the feature check in the || case of this check to use
> mount flags, the primary superblock buffer verification on first
> read no longer flags CRC errors....
<nod> Definitely something to tack on the end.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
