Re: [PATCHv2 07/17] nvme: add Clang context annotations for nvme_subsystem::lock

[Nilay Shroff]

On 6/26/26 12:13 PM, Christoph Hellwig wrote:
> > +	scoped_guard(mutex_init, &subsys->lock)
> > +		INIT_LIST_HEAD(&subsys->nsheads);
>
> Same init mess as a few patches earlier.
yes correct, but this one is tricky as the list has to be inited
with non-zero value. Another way to fix this is by adding
context_unsafe wrapper around INIT_LIST_HEAD(). Again overuse of
those unsafe wrappers hinders readability.

Would it make sense to introduce a helper, e.g. INIT_LIST_HEAD_UNSAFE(),
that simply wraps INIT_LIST_HEAD() with context_unsafe()? It would document
that this should only be used in cases where the caller knows the object
has not yet been published and therefore no concurrent access is possible,
such as during object initialization.

> > --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -719,7 +719,14 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
> >   	INIT_WORK(&head->requeue_work, nvme_requeue_work);
> >   	INIT_WORK(&head->partition_scan_work, nvme_partition_scan_work);
> >   	INIT_DELAYED_WORK(&head->remove_work, nvme_remove_head_work);
> > -	head->delayed_removal_secs = 0;
> > +	/*
> > +	 * The namespace head is not yet visible to other threads, so
> > +	 * initializing delayed_removal_secs does not require holding
> > +	 * subsys->lock. So suppress Clang's context analyzer warning by
> > +	 * annotating initialization of delayed_removal_secs using
> > +	 * context_unsafe.
> > +	 */
> > +	context_unsafe(head->delayed_removal_secs = 0);
>
> head is a zeroed allocation, and we call nvme_mpath_alloc_disk
> exactly once on it.  So we can just remove the initialization
> of delayed_removal_secs entirely here.
Yes that makes sense. Will handle this in next patchset.

> > +	struct list_head	nsheads __guarded_by(&lock);
> >   	char			subnqn[NVMF_NQN_SIZE];
> >   	char			serial[20];
> >   	char			model[40];
> > @@ -562,7 +562,7 @@ struct nvme_ns_head {
> >   	struct mutex		lock;
> >   	unsigned long		flags;
> >   	struct delayed_work	remove_work;
> > -	unsigned int		delayed_removal_secs;
> > +	unsigned int		delayed_removal_secs __guarded_by(&subsys->lock);
>
> Btw, I find throwing the __guarded_by at the end of the line really
> hard to read, especially with all these long lines.
>
> What about moving them to the next line with an extra tab indent
> instead?
Yes sure, will address this in next patchset.

Thanks,
--Nilay