Re: parallel nvme format

[Keith Busch <kbusch@kernel.org>]

On Thu, Dec 14, 2023 at 11:27:57PM +0000, Kamaljit Singh wrote:
> Hi Keith,
> 
> Good suggestions but we may need another change to check for NVME_CTRL_NEEDS_SCAN and kick-off nvme_scan_work later, to follow-up for the call that may have been skipped.

Another check where? These are the only two paths that hold scan_lock,
so if the initial scan was aborted because the other path was holding
the lock, that path has to see that the needs rescan flag was set and
requeue it.

I guess there could be some user initiated scan or an AER triggered scan
in a multipath+multihost setup that was aborted because of an admin
command without ever explicitly setting the flag bit, so we could set it
on trylock failure to make doubly sure it gets rekicked after the
scan_lock is released:

	if (!mutex_trylock(&ctrl->scan_lock)) {
		set_bit(NVME_CTRL_NEEDS_RESCAN, &ctrl->flags);
		return;
	}
 
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1120,6 +1120,8 @@ u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
> >               nvme_start_freeze(ctrl);
> >               nvme_wait_freeze(ctrl);
> >       }
> > +     if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
> > +             set_bit(NVME_CTRL_NEEDS_RESCAN, &ctrl->flags);
> >       return effects;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(nvme_passthru_start, NVME_TARGET_PASSTHRU);
> > @@ -1140,7 +1142,7 @@ void nvme_passthru_end(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u32 effects,
> >  "controller capabilities changed, reset may be required to take effect.\n");
> >               }
> >       }
> > -     if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC)) {
> > +     if (test_bit(NVME_CTRL_NEEDS_RESCAN, &ctrl->flags)) {
> >               nvme_queue_scan(ctrl);
> >               flush_work(&ctrl->scan_work);
> >       }
> > @@ -3945,7 +3947,14 @@ static void nvme_scan_work(struct work_struct *work)
> >               nvme_clear_changed_ns_log(ctrl);
> >       }
> >
> > -     mutex_lock(&ctrl->scan_lock);
> > +     /*
> > +      * Failure to acquire scan_lock means another thread will requeue this
> > +      * work shortly
> > +      */
> > +     if (!mutex_trylock(&ctrl->scan_lock))
> > +             return;
> > +
> > +     clear_bit(NVME_CTRL_NEEDS_RESCAN, &ctrl->flags);
> >       if (nvme_ctrl_limited_cns(ctrl)) {
> >               nvme_scan_ns_sequential(ctrl);
> >       } else {
> > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> > index 39a90b7cb1254..79a71b6ae64fe 100644
> > --- a/drivers/nvme/host/nvme.h
> > +++ b/drivers/nvme/host/nvme.h
> > @@ -251,6 +251,7 @@ enum nvme_ctrl_flags {
> >       NVME_CTRL_STOPPED               = 3,
> >       NVME_CTRL_SKIP_ID_CNS_CS        = 4,
> >       NVME_CTRL_DIRTY_CAPABILITY      = 5,
> > +     NVME_CTRL_NEEDS_RESCAN          = 6,
> >  };
> >
> >  struct nvme_ctrl {
> > --
> >