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

Kent Overstreet
2023-12-08T20:34:59Z

On Fri, Dec 08, 2023 at 03:25:02PM -0500, Brian Foster wrote:
> 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?

Yeah, this was my early rough draft, it's fixed now :)