| Message ID | 97622c5c2e2dbb2316901c6ebd9792cbf58385fd.1667776994.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "btrfs: scrub: use larger block size for data extent scrub" | expand |
On 2022/11/7 07:23, Qu Wenruo wrote: > This reverts commit 786672e9e1a39a231806313e3c445c236588ceef. > > [BUG] > Since commit 786672e9e1a3 ("btrfs: scrub: use larger block size for data > extent scrub"), btrfs scrub no longer reports errors if the corruption > is not in the first sector of a STRIPE_LEN. > > The following script can expose the problem: > > mkfs.btrfs -f $dev > mount $dev $mnt > xfs_io -f -c "pwrite -S 0xff 0 8k" $mnt/foobar > umount $mnt > > # 13631488 is the logical bytenr of above 8K extent > btrfs-map-logical -l 13631488 -b 4096 $dev > mirror 1 logical 13631488 physical 13631488 device /dev/test/scratch1 > > # Corrupt the 2nd sector of that extent > xfs_io -f -c "pwrite -S 0x00 13635584 4k" $dev > > mount $dev $mnt > btrfs scrub start -B $mnt > scrub done for 54e63f9f-0c30-4c84-a33b-5c56014629b7 > Scrub started: Mon Nov 7 07:18:27 2022 > Status: finished > Duration: 0:00:00 > Total to scrub: 536.00MiB > Rate: 0.00B/s > Error summary: no errors found <<< > > [CAUSE] > That offending commit enlarge the data extent scrub size from sector > size to BTRFS_STRIPE_LEN, to avoid extra scrub_block to be allocated. > > But unfortunately the data extent scrub is still heavily relying on the > fact that there is only one scrub_sector per scrub_block. > > Thus it will only check the first sector, and ignoring the remaining > sectors. > > Furthermore the error reporting is not able to handle multiple sectors > either. > > [FIX] > For now just revert the offending commit. > > The consequence is just extra memory usage during scrub. > We will need a proper change to make the remaining data scrub path to > handle multiple sectors before we enlarging the data scrub size. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reported-by: Li Zhang <zhanglikernel@gmail.com> > --- > fs/btrfs/scrub.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index 06c6626eae3d..e5dbf875f6d9 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -2691,17 +2691,11 @@ static int scrub_extent(struct scrub_ctx *sctx, struct map_lookup *map, > u8 csum[BTRFS_CSUM_SIZE]; > u32 blocksize; > > - /* > - * Block size determines how many scrub_block will be allocated. Here > - * we use BTRFS_STRIPE_LEN (64KiB) as default limit, so we won't > - * allocate too many scrub_block, while still won't cause too large > - * bios for large extents. > - */ > if (flags & BTRFS_EXTENT_FLAG_DATA) { > if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) > blocksize = map->stripe_len; > else > - blocksize = BTRFS_STRIPE_LEN; > + blocksize = sctx->fs_info->sectorsize; > spin_lock(&sctx->stat_lock); > sctx->stat.data_extents_scrubbed++; > sctx->stat.data_bytes_scrubbed += len;
On Mon, Nov 07, 2022 at 07:23:26AM +0800, Qu Wenruo wrote: > This reverts commit 786672e9e1a39a231806313e3c445c236588ceef. > > [BUG] > Since commit 786672e9e1a3 ("btrfs: scrub: use larger block size for data > extent scrub"), btrfs scrub no longer reports errors if the corruption > is not in the first sector of a STRIPE_LEN. > > The following script can expose the problem: > > mkfs.btrfs -f $dev > mount $dev $mnt > xfs_io -f -c "pwrite -S 0xff 0 8k" $mnt/foobar > umount $mnt > > # 13631488 is the logical bytenr of above 8K extent > btrfs-map-logical -l 13631488 -b 4096 $dev > mirror 1 logical 13631488 physical 13631488 device /dev/test/scratch1 > > # Corrupt the 2nd sector of that extent > xfs_io -f -c "pwrite -S 0x00 13635584 4k" $dev > > mount $dev $mnt > btrfs scrub start -B $mnt > scrub done for 54e63f9f-0c30-4c84-a33b-5c56014629b7 > Scrub started: Mon Nov 7 07:18:27 2022 > Status: finished > Duration: 0:00:00 > Total to scrub: 536.00MiB > Rate: 0.00B/s > Error summary: no errors found <<< > > [CAUSE] > That offending commit enlarge the data extent scrub size from sector > size to BTRFS_STRIPE_LEN, to avoid extra scrub_block to be allocated. > > But unfortunately the data extent scrub is still heavily relying on the > fact that there is only one scrub_sector per scrub_block. > > Thus it will only check the first sector, and ignoring the remaining > sectors. > > Furthermore the error reporting is not able to handle multiple sectors > either. > > [FIX] > For now just revert the offending commit. > > The consequence is just extra memory usage during scrub. > We will need a proper change to make the remaining data scrub path to > handle multiple sectors before we enlarging the data scrub size. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 06c6626eae3d..e5dbf875f6d9 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -2691,17 +2691,11 @@ static int scrub_extent(struct scrub_ctx *sctx, struct map_lookup *map, u8 csum[BTRFS_CSUM_SIZE]; u32 blocksize; - /* - * Block size determines how many scrub_block will be allocated. Here - * we use BTRFS_STRIPE_LEN (64KiB) as default limit, so we won't - * allocate too many scrub_block, while still won't cause too large - * bios for large extents. - */ if (flags & BTRFS_EXTENT_FLAG_DATA) { if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) blocksize = map->stripe_len; else - blocksize = BTRFS_STRIPE_LEN; + blocksize = sctx->fs_info->sectorsize; spin_lock(&sctx->stat_lock); sctx->stat.data_extents_scrubbed++; sctx->stat.data_bytes_scrubbed += len;
This reverts commit 786672e9e1a39a231806313e3c445c236588ceef. [BUG] Since commit 786672e9e1a3 ("btrfs: scrub: use larger block size for data extent scrub"), btrfs scrub no longer reports errors if the corruption is not in the first sector of a STRIPE_LEN. The following script can expose the problem: mkfs.btrfs -f $dev mount $dev $mnt xfs_io -f -c "pwrite -S 0xff 0 8k" $mnt/foobar umount $mnt # 13631488 is the logical bytenr of above 8K extent btrfs-map-logical -l 13631488 -b 4096 $dev mirror 1 logical 13631488 physical 13631488 device /dev/test/scratch1 # Corrupt the 2nd sector of that extent xfs_io -f -c "pwrite -S 0x00 13635584 4k" $dev mount $dev $mnt btrfs scrub start -B $mnt scrub done for 54e63f9f-0c30-4c84-a33b-5c56014629b7 Scrub started: Mon Nov 7 07:18:27 2022 Status: finished Duration: 0:00:00 Total to scrub: 536.00MiB Rate: 0.00B/s Error summary: no errors found <<< [CAUSE] That offending commit enlarge the data extent scrub size from sector size to BTRFS_STRIPE_LEN, to avoid extra scrub_block to be allocated. But unfortunately the data extent scrub is still heavily relying on the fact that there is only one scrub_sector per scrub_block. Thus it will only check the first sector, and ignoring the remaining sectors. Furthermore the error reporting is not able to handle multiple sectors either. [FIX] For now just revert the offending commit. The consequence is just extra memory usage during scrub. We will need a proper change to make the remaining data scrub path to handle multiple sectors before we enlarging the data scrub size. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/scrub.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)