Skip to content

Commit

Permalink
Improve zfs_blkptr_verify()
Browse files Browse the repository at this point in the history 
 - Skip config lock enter/exit for embedded blocks.  They have no
DVAs, so there is nothing to check under the lock.
 - Skip CHECKSUM check and properly check PSIZE for embedded blocks.
 - Add static branch predictions for unlikely conditions.
 - Do not verify DVAs for blocks already in ARC.  ARC hit already
"verified" the first (often the only) DVA, and it does not worth to
enter/exit config lock for nothing.

Some profiles show me up to 3% of CPU saving from this change.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
  • Loading branch information
@amotin
amotin committed Jul 24, 2024
1 parent a1be921 commit 2126386
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 33 deletions.
39 changes: 26 additions & 13 deletions module/zfs/arc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5508,19 +5508,6 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
*/
fstrans_cookie_t cookie = spl_fstrans_mark();
top:
/*
* Verify the block pointer contents are reasonable. This should
* always be the case since the blkptr is protected by a checksum.
* However, if there is damage it's desirable to detect this early
* and treat it as a checksum error. This allows an alternate blkptr
* to be tried when one is available (e.g. ditto blocks).
*/
if (!zfs_blkptr_verify(spa, bp, (zio_flags & ZIO_FLAG_CONFIG_WRITER) ?
BLK_CONFIG_HELD : BLK_CONFIG_NEEDED, BLK_VERIFY_LOG)) {
rc = SET_ERROR(ECKSUM);
goto done;
}

if (!embedded_bp) {
/*
* Embedded BP's have no DVA and require no I/O to "read".
Expand All @@ -5540,6 +5527,18 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
(hdr->b_l1hdr.b_pabd != NULL && !encrypted_read))) {
boolean_t is_data = !HDR_ISTYPE_METADATA(hdr);

/*
* Verify the block pointer contents are reasonable. This
* should always be the case since the blkptr is protected by
* a checksum.
*/
if (!zfs_blkptr_verify(spa, bp, BLK_CONFIG_SKIP,
BLK_VERIFY_LOG)) {
mutex_exit(hash_lock);
rc = SET_ERROR(ECKSUM);
goto done;
}

if (HDR_IO_IN_PROGRESS(hdr)) {
if (*arc_flags & ARC_FLAG_CACHED_ONLY) {
mutex_exit(hash_lock);
Expand Down Expand Up @@ -5693,6 +5692,20 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
goto done;
}

/*
* Verify the block pointer contents are reasonable. This
* should always be the case since the blkptr is protected by
* a checksum.
*/
if (!zfs_blkptr_verify(spa, bp,
(zio_flags & ZIO_FLAG_CONFIG_WRITER) ?
BLK_CONFIG_HELD : BLK_CONFIG_NEEDED, BLK_VERIFY_LOG)) {
if (hash_lock != NULL)
mutex_exit(hash_lock);
rc = SET_ERROR(ECKSUM);
goto done;
}

if (hdr == NULL) {
/*
* This block is not in the cache or it has
Expand Down
45 changes: 25 additions & 20 deletions module/zfs/zio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1101,45 +1101,50 @@ zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp,
{
int errors = 0;

if (!DMU_OT_IS_VALID(BP_GET_TYPE(bp))) {
if (unlikely(!DMU_OT_IS_VALID(BP_GET_TYPE(bp)))) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid TYPE %llu",
bp, (longlong_t)BP_GET_TYPE(bp));
}
if (BP_GET_CHECKSUM(bp) >= ZIO_CHECKSUM_FUNCTIONS) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid CHECKSUM %llu",
bp, (longlong_t)BP_GET_CHECKSUM(bp));
}
if (BP_GET_COMPRESS(bp) >= ZIO_COMPRESS_FUNCTIONS) {
if (unlikely(BP_GET_COMPRESS(bp) >= ZIO_COMPRESS_FUNCTIONS)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid COMPRESS %llu",
bp, (longlong_t)BP_GET_COMPRESS(bp));
}
if (BP_GET_LSIZE(bp) > SPA_MAXBLOCKSIZE) {
if (unlikely(BP_GET_LSIZE(bp) > SPA_MAXBLOCKSIZE)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid LSIZE %llu",
bp, (longlong_t)BP_GET_LSIZE(bp));
}
if (BP_GET_PSIZE(bp) > SPA_MAXBLOCKSIZE) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid PSIZE %llu",
bp, (longlong_t)BP_GET_PSIZE(bp));
}

if (BP_IS_EMBEDDED(bp)) {
if (BPE_GET_ETYPE(bp) >= NUM_BP_EMBEDDED_TYPES) {
if (unlikely(BPE_GET_ETYPE(bp) >= NUM_BP_EMBEDDED_TYPES)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid ETYPE %llu",
bp, (longlong_t)BPE_GET_ETYPE(bp));
}
if (unlikely(BPE_GET_PSIZE(bp) > BPE_PAYLOAD_SIZE)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid PSIZE %llu",
bp, (longlong_t)BPE_GET_PSIZE(bp));
}
return (errors == 0);
}
if (unlikely(BP_GET_CHECKSUM(bp) >= ZIO_CHECKSUM_FUNCTIONS)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid CHECKSUM %llu",
bp, (longlong_t)BP_GET_CHECKSUM(bp));
}
if (unlikely(BP_GET_PSIZE(bp) > SPA_MAXBLOCKSIZE)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px has invalid PSIZE %llu",
bp, (longlong_t)BP_GET_PSIZE(bp));
}

/*
* Do not verify individual DVAs if the config is not trusted. This
* will be done once the zio is executed in vdev_mirror_map_alloc.
*/
if (!spa->spa_trust_config)
if (unlikely(!spa->spa_trust_config))
return (errors == 0);

switch (blk_config) {
Expand Down Expand Up @@ -1168,20 +1173,20 @@ zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp,
const dva_t *dva = &bp->blk_dva[i];
uint64_t vdevid = DVA_GET_VDEV(dva);

if (vdevid >= spa->spa_root_vdev->vdev_children) {
if (unlikely(vdevid >= spa->spa_root_vdev->vdev_children)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px DVA %u has invalid VDEV %llu",
bp, i, (longlong_t)vdevid);
continue;
}
vdev_t *vd = spa->spa_root_vdev->vdev_child[vdevid];
if (vd == NULL) {
if (unlikely(vd == NULL)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px DVA %u has invalid VDEV %llu",
bp, i, (longlong_t)vdevid);
continue;
}
if (vd->vdev_ops == &vdev_hole_ops) {
if (unlikely(vd->vdev_ops == &vdev_hole_ops)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px DVA %u has hole VDEV %llu",
bp, i, (longlong_t)vdevid);
Expand All @@ -1199,7 +1204,7 @@ zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp,
uint64_t asize = DVA_GET_ASIZE(dva);
if (DVA_GET_GANG(dva))
asize = vdev_gang_header_asize(vd);
if (offset + asize > vd->vdev_asize) {
if (unlikely(offset + asize > vd->vdev_asize)) {
errors += zfs_blkptr_verify_log(spa, bp, blk_verify,
"blkptr at %px DVA %u has invalid OFFSET %llu",
bp, i, (longlong_t)offset);
Expand Down

0 comments on commit 2126386

Please sign in to comment.