Re: [PATCH 3/5] btrfs: store details about first setget bounds check failure

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Feb 03, 2022 at 06:26:29PM +0100, David Sterba wrote:
> The way the setget helpers are used makes it hard to return an error
> code at the call site, as this would require to clutter a lot of code
> with potential failures that are expected to be rare.
> 
> Instead, do a delayed reporting, tracked by a filesystem-wide bit that
> synchronizes potential races in the reporting function that records only
> the first event. To give a bit more insight into the scale, count the
> total number of events.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h        | 16 ++++++++++++++--
>  fs/btrfs/struct-funcs.c | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 9f7a950b8a69..5d12a80d09f5 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -127,6 +127,13 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
>  enum {
>  	/* Global indicator of serious filesystem errors */
>  	BTRFS_FS_STATE_ERROR,
> +	/* Track if a transaction abort has been reported on this filesystem */
> +	BTRFS_FS_STATE_TRANS_ABORTED,
> +	/*
> +	 * There was a failed bounds check in check_setget_bounds, set this on
> +	 * first event.
> +	 */
> +	BTRFS_FS_SETGET_COMPLAINS,
>  	/*
>  	 * Filesystem is being remounted, allow to skip some operations, like
>  	 * defrag
> @@ -134,8 +141,6 @@ enum {
>  	BTRFS_FS_STATE_REMOUNTING,
>  	/* Filesystem in RO mode */
>  	BTRFS_FS_STATE_RO,
> -	/* Track if a transaction abort has been reported on this filesystem */
> -	BTRFS_FS_STATE_TRANS_ABORTED,
>  	/*
>  	 * Bio operations should be blocked on this filesystem because a source
>  	 * or target device is being destroyed as part of a device replace
> @@ -1060,6 +1065,13 @@ struct btrfs_fs_info {
>  	spinlock_t zone_active_bgs_lock;
>  	struct list_head zone_active_bgs;
>  
> +	/* Store details about the first bounds check failure in report_setget_bounds */
> +	u64 setget_eb_start;
> +	const void *setget_ptr;
> +	unsigned setget_off;
> +	int setget_size;
> +	atomic_t setget_failures;
> +
>  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>  	spinlock_t ref_verify_lock;
>  	struct rb_root block_tree;
> diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
> index c97c69e29d64..28b9e62cdc86 100644
> --- a/fs/btrfs/struct-funcs.c
> +++ b/fs/btrfs/struct-funcs.c
> @@ -11,11 +11,23 @@ static void report_setget_bounds(const struct extent_buffer *eb,
>  				 const void *ptr, unsigned off, int size)
>  {
>  	const unsigned long member_offset = (unsigned long)ptr + off;
> +	struct btrfs_fs_info *fs_info;
>  
>  	btrfs_err_rl(eb->fs_info,
>  		"bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
>  		(member_offset > eb->len ? "start" : "end"),
>  		(unsigned long)ptr, eb->start, member_offset, size);
> +
> +	/* Count events and record more details about the first one */
> +	fs_info = eb->fs_info;
> +	atomic_inc(&fs_info->setget_failures);
> +	if (test_and_set_bit(BTRFS_FS_SETGET_COMPLAINS, &eb->fs_info->flags))
> +		return;
> +
> +	fs_info->setget_eb_start = eb->start;
> +	fs_info->setget_ptr = ptr;
> +	fs_info->setget_off = member_offset;
> +	fs_info->setget_size = size;

These new fields at fs_info are set here, but then never read, neither in this
patch nor in the remaining of this patchset.

Do you plan to use them somewhere else? If not, it seems they could go away.

Thanks.

>  }
>  
>  static inline bool check_setget_bounds(const struct extent_buffer *eb,
> -- 
> 2.34.1
>