Re: [PATCH 4/6] bcachefs: bch2_run_online_recovery_passes()

[Brian Foster <bfoster@redhat.com>]

On Wed, Dec 06, 2023 at 03:33:08PM -0500, Kent Overstreet wrote:
> Add a new helper for running online recovery passes - i.e. online fsck.
> This is a subset of our normal recovery passes, and does not - for now -
> use or follow c->curr_recovery_pass.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/bcachefs/bcachefs.h |  7 ++++++
>  fs/bcachefs/recovery.c | 51 ++++++++++++++++++++++++++++--------------
>  fs/bcachefs/recovery.h |  1 +
>  3 files changed, 42 insertions(+), 17 deletions(-)
> 
...
> diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c
> index 585b4928964f..262c923b2f1a 100644
> --- a/fs/bcachefs/recovery.c
> +++ b/fs/bcachefs/recovery.c
...
> @@ -651,39 +651,56 @@ static bool should_run_recovery_pass(struct bch_fs *c, enum bch_recovery_pass pa
>  
>  static int bch2_run_recovery_pass(struct bch_fs *c, enum bch_recovery_pass pass)
>  {
> +	struct recovery_pass_fn *p = recovery_pass_fns + pass;
>  	int ret;
>  
> -	c->curr_recovery_pass = pass;
> +	if (!(p->when & PASS_SILENT))
> +		printk(KERN_INFO bch2_log_msg(c, "%s..."),
> +		       bch2_recovery_passes[pass]);
> +	ret = p->fn(c);
> +	if (ret)
> +		return ret;
> +	if (!(p->when & PASS_SILENT))
> +		printk(KERN_CONT " done\n");
>  
> -	if (should_run_recovery_pass(c, pass)) {
> -		struct recovery_pass_fn *p = recovery_pass_fns + pass;
> +	return 0;
> +}
>  
> -		if (!(p->when & PASS_SILENT))
> -			printk(KERN_INFO bch2_log_msg(c, "%s..."),
> -			       bch2_recovery_passes[pass]);
> -		ret = p->fn(c);
> -		if (ret)
> -			return ret;
> -		if (!(p->when & PASS_SILENT))
> -			printk(KERN_CONT " done\n");
> +static int bch2_run_recovery_passes(struct bch_fs *c)
> +{
> +	int ret = 0;
>  
> -		c->recovery_passes_complete |= BIT_ULL(pass);
> +	while (c->curr_recovery_pass < ARRAY_SIZE(recovery_pass_fns)) {
> +		if (should_run_recovery_pass(c, c->curr_recovery_pass)) {
> +			ret = bch2_run_recovery_pass(c, c->curr_recovery_pass);
> +			if (bch2_err_matches(ret, BCH_ERR_restart_recovery))
> +				continue;
> +			if (ret)
> +				break;
> +
> +			c->recovery_passes_complete |= BIT_ULL(c->curr_recovery_pass);
> +		}
> +		c->curr_recovery_pass++;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
> -static int bch2_run_recovery_passes(struct bch_fs *c)
> +int bch2_run_online_recovery_passes(struct bch_fs *c)
>  {
>  	int ret = 0;
>  
> -	while (c->curr_recovery_pass < ARRAY_SIZE(recovery_pass_fns)) {
> +	for (unsigned i = 0; i < ARRAY_SIZE(recovery_pass_fns); i++) {
> +		struct recovery_pass_fn *p = recovery_pass_fns + c->curr_recovery_pass;

Maybe I'm confused, but should this be using i?

Brian

> +
> +		if (!(p->when & PASS_ONLINE))
> +			continue;
> +
>  		ret = bch2_run_recovery_pass(c, c->curr_recovery_pass);
>  		if (bch2_err_matches(ret, BCH_ERR_restart_recovery))
>  			continue;
>  		if (ret)
>  			break;
> -		c->curr_recovery_pass++;
>  	}
>  
>  	return ret;
> diff --git a/fs/bcachefs/recovery.h b/fs/bcachefs/recovery.h
> index 852d30567da9..447590f60609 100644
> --- a/fs/bcachefs/recovery.h
> +++ b/fs/bcachefs/recovery.h
> @@ -25,6 +25,7 @@ static inline int bch2_run_explicit_recovery_pass(struct bch_fs *c,
>  	}
>  }
>  
> +int bch2_run_online_recovery_passes(struct bch_fs *);
>  u64 bch2_fsck_recovery_passes(void);
>  
>  int bch2_fs_recovery(struct bch_fs *);
> -- 
> 2.42.0
> 
>