Re: [f2fs-dev] [PATCH v6 8/8] fsck: Check write pointer consistency of non-open zones

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Nov 05, 2019 / 19:32, Chao Yu wrote:
> On 2019/10/28 14:55, Shin'ichiro Kawasaki wrote:
> > To catch f2fs bug in write pointer handling code for zoned block devices,
> > have fsck check consistency of write pointers of non-open zones, that
> > current segments do not point to. Check two items comparing write pointer
> > positions with valid block maps in SIT.
> > 
> > The first item is check for zones with no valid blocks. When there is no
> > valid blocks in a zone, the write pointer should be at the start of the
> > zone. If not, next write operation to the zone will cause unaligned write
> > error. If write pointer is not at the zone start, reset the zone to move
> > the write pointer to the zone start.
> > 
> > The second item is check between write pointer position and the last
> > valid block in the zone. It is unexpected that the last valid block
> > position is beyond the write pointer. In such a case, report as the bug.
> > Fix is not required for such zone, because the zone is not selected for
> > next write operation until the zone get discarded.
> > 
> > In the same manner as the consistency check for current segments, do the
> > check and fix twice: at the beginning of do_fsck() to avoid unaligned
> > write error during fsck, and at fsck_verify() to reflect meta data
> > updates by fsck.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  fsck/fsck.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 119 insertions(+)
> > 
> > diff --git a/fsck/fsck.c b/fsck/fsck.c
> > index e0eda4e..8400929 100644
> > --- a/fsck/fsck.c
> > +++ b/fsck/fsck.c
> > @@ -2751,6 +2751,122 @@ out:
> >  	return cnt;
> >  }
> >  
> > +#ifdef HAVE_LINUX_BLKZONED_H
> > +
> > +struct write_pointer_check_data {
> > +	struct f2fs_sb_info *sbi;
> > +	int dev_index;
> > +};
> > +
> > +static int chk_and_fix_wp_with_sit(int i, void *blkzone, void *opaque)
> > +{
> > +	struct blk_zone *blkz = (struct blk_zone *)blkzone;
> > +	struct write_pointer_check_data *wpd = opaque;
> > +	struct f2fs_sb_info *sbi = wpd->sbi;
> > +	struct device_info *dev = c.devices + wpd->dev_index;
> > +	struct f2fs_fsck *fsck = F2FS_FSCK(sbi);
> > +	block_t zone_block, wp_block, wp_blkoff;
> > +	unsigned int zone_segno, wp_segno;
> > +	struct curseg_info *cs;
> > +	int cs_index, ret, last_valid_blkoff;
> > +	int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT;
> > +	unsigned int segs_per_zone = sbi->segs_per_sec * sbi->secs_per_zone;
> > +
> > +	if (blk_zone_conv(blkz))
> > +		return 0;
> > +
> > +	zone_block = dev->start_blkaddr
> > +		+ (blk_zone_sector(blkz) >> log_sectors_per_block);
> > +	zone_segno = GET_SEGNO(sbi, zone_block);
> > +	if (zone_segno >= MAIN_SEGS(sbi))
> > +		return 0;
> > +
> > +	wp_block = dev->start_blkaddr
> > +		+ (blk_zone_wp_sector(blkz) >> log_sectors_per_block);
> > +	wp_segno = GET_SEGNO(sbi, wp_block);
> > +	wp_blkoff = wp_block - START_BLOCK(sbi, wp_segno);
> > +
> > +	/* if a curseg points to the zone, skip the check */
> > +	for (cs_index = 0; cs_index < NO_CHECK_TYPE; cs_index++) {
> > +		cs = &SM_I(sbi)->curseg_array[cs_index];
> > +		if (zone_segno <= cs->segno &&
> > +		    cs->segno < zone_segno + segs_per_zone)
> > +			return 0;
> > +	}
> > +
> > +	last_valid_blkoff = last_vblk_off_in_zone(sbi, zone_segno);
> > +
> > +	/*
> > +	 * When there is no valid block in the zone, check write pointer is
> > +	 * at zone start. If not, reset the write pointer.
> > +	 */
> > +	if (last_valid_blkoff < 0 &&
> > +	    blk_zone_wp_sector(blkz) != blk_zone_sector(blkz)) {
> > +		if (!c.fix_on) {
> > +			MSG(0, "Inconsistent write pointer: wp[0x%x,0x%x]\n",
> > +			    wp_segno, wp_blkoff);
> > +			fsck->chk.wp_inconsistent_zones++;
> > +			return 0;
> > +		}
> > +
> > +		FIX_MSG("Reset write pointer of zone at segment 0x%x",
> > +			zone_segno);
> > +		ret = f2fs_reset_zone(wpd->dev_index, blkz);
> > +		if (ret) {
> > +			printf("[FSCK] Write pointer reset failed: %s\n",
> > +			       dev->path);
> > +			return ret;
> > +		}
> > +		fsck->chk.wp_fixed = 1;
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * If valid blocks exist in the zone beyond the write pointer, it
> > +	 * is a f2fs bug. No need to fix because the zone is not selected
> 
> Minor thing, mostly probably it is a f2fs bug, however there should be
> software/hardware bug in other layer can cause such inconsistent.. so let's get
> rid of such first impression. :)
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,

Ah, that is a stereotype. I think it's the better to remove the word "f2fs", as
follows. Will do that edit.

/*
 * If valid blocks exist in the zone beyond the write pointer, it
 * is a bug. No need to ...

--
Best Regards,
Shin'ichiro Kawasaki

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel