Re: [PATCH 6/6] Btrfs: add sanity check of extent item in scrub

[Liu Bo <bo.li.liu@oracle.com>]

On Fri, May 26, 2017 at 08:33:16PM +0200, David Sterba wrote:
> On Thu, May 25, 2017 at 06:26:31PM -0600, Liu Bo wrote:
> > Currently scrub only verify checksum of both metadata and data and
> > couldn't detect an invalid extent_item.
> 
> This is a different kind of check that scrub was never designed to do.
> Scrub just verifies the checksums, not the sructural integrity. This has
> been referred to as an "on-line fsck". AFAIK xfs does not use 'scrub' in
> the same sense as btrfs, so things are going to be confusing for the
> users.
> 
> The on-line fsck is still desired, but I'd like to see a discussion how
> exactly it should work, whether to extend scrub or add a new ioctl etc.
>

I was hesitating about scrub vs online fsck when posting, just thought
it'd be good when users got this error while doing a regular scrub
ahead of really getting into trouble.  If we have a plan for online
fsck, I'm happy to let fsck do the job.

Or we can do this kind of sanity check at the time when reading the
eb.

Thanks,
-liubo

> > This adds sanity check for extent item, now it can check if
> > extent_inline_ref_type is valid.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/scrub.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > index b0251eb..e87b752 100644
> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -3058,6 +3058,39 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
> >  	return ret < 0 ? ret : 0;
> >  }
> >  
> > +static int check_extent_item(struct extent_buffer *l, int slot,
> 
> Please use 'eb' for the extent_buffer.
> 
> > +			     struct btrfs_extent_item *ei, int key_type)
> 
> key_type should be u8
> 
> > +{
> > +	unsigned long ptr;
> > +	unsigned long end;
> > +	struct btrfs_extent_inline_ref *iref;
> > +	u64 flags = btrfs_extent_flags(l, ei);
> > +	int is_data = !!(flags & BTRFS_EXTENT_FLAG_DATA);
> > +	int type;
> > +
> > +	ptr = (unsigned long)(ei + 1);
> > +	if (!is_data &&
> > +	    key_type != BTRFS_METADATA_ITEM_KEY)
> > +		ptr += sizeof(struct btrfs_tree_block_info);
> > +	end = (unsigned long)ei +
> > +		btrfs_item_size_nr(l, slot);
> 
> This will fit to one line
> 
> > +
> > +	while (1) {
> > +		if (ptr >= end) {
> > +			WARN_ON(ptr > end);
> 
> Please add some verbose error message or get rid of the WARN_ON
> completely if possible.
> 
> > +			break;
> > +		}
> > +
> > +		iref = (struct btrfs_extent_inline_ref *)ptr;
> > +		type = btrfs_get_extent_inline_ref_type(l, iref, is_data);
> > +		if (type < 0)
> > +			return type;
> > +
> > +		ptr += btrfs_extent_inline_ref_size(type);
> > +	}
> > +	return 0;
> > +}
> > +
> >  static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
> >  					   struct map_lookup *map,
> >  					   struct btrfs_device *scrub_dev,
> > @@ -3318,6 +3351,16 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
> >  				goto next;
> >  			}
> >  
> > +			/* sanity check for extent inline ref type */
> > +			if (check_extent_item(l, slot, extent, key.type)) {
> > +				btrfs_err(fs_info,
> > +					  "scrub: extent %llu(0x%llx) has an invalid extent inline ref type, ignored.",
> 
> Strings can be un-indented to the left.
> 
> > +					  key.objectid, key.objectid);
> > +				spin_lock(&sctx->stat_lock);
> > +				sctx->stat.uncorrectable_errors++;
> 
> Yeah, this is an example where the uncorrectable_error would gain a new
> meaning. We'd need to extend the scrub error reporting so the various
> metadata errors get reported properly and not mangled with the rest.
> 
> > +				spin_unlock(&sctx->stat_lock);
> > +				goto next;
> > +			}
> >  again:
> >  			extent_logical = key.objectid;
> >  			extent_len = bytes;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html