| Message ID | 20210714041912.2625692-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework feature flags | expand |
On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> .. in the long run we really need to kill of b_bn to avoid this kind of confusion.
On Wed, Jul 14, 2021 at 07:43:19AM +0100, Christoph Hellwig wrote: > On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > 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. > > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > .. in the long run we really need to kill of b_bn to avoid this > kind of confusion. b_bn is supposed to only be an internal cache index these days. We need that index in the first cacheline of the struct xfs_buf for performance reasons (so traversals fetch only a single cacheline per level), so perhaps a rename is in order just to catch all these remaining users that shouldn't be using it... Cheers, Dave.
On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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@redhat.com> > --- > 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; With that changed, this looks reasonable to me. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > xfs_sb_version_hascrc(&mp->m_sb)) { > error = -EFSBADCRC; > goto out_error; > -- > 2.31.1 >
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@redhat.com> > > > > 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@redhat.com> > > --- > > 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... 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... 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.... Cheers, Dave.
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@redhat.com> > > > > > > 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@redhat.com> > > > --- > > > 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@fromorbit.com
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 || xfs_sb_version_hascrc(&mp->m_sb)) { error = -EFSBADCRC; goto out_error;