Re: [PATCH v2 20/26] null_blk: Enable lock context analysis

[Bart Van Assche <bvanassche@acm.org>]

On 3/30/26 10:37 PM, Nilay Shroff wrote:
> On 3/30/26 9:55 PM, Bart Van Assche wrote:
>> (+Marco)
>>
>> On 3/30/26 3:23 AM, Nilay Shroff wrote:
>>> On 3/26/26 3:15 AM, Bart Van Assche wrote:
>>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/ 
>>>> main.c
>>>> index f8c0fd57e041..677ac829ef80 100644
>>>> --- a/drivers/block/null_blk/main.c
>>>> +++ b/drivers/block/null_blk/main.c
>>>> @@ -1004,8 +1004,7 @@ static struct nullb_page 
>>>> *null_lookup_page(struct nullb *nullb,
>>>>   static struct nullb_page *null_insert_page(struct nullb *nullb,
>>>>                          sector_t sector, bool ignore_cache)
>>>> -    __releases(&nullb->lock)
>>>> -    __acquires(&nullb->lock)
>>>> +    __must_hold(&nullb->lock)
>>>
>>> This function temporarily drops the &nullb->lock and requires it.
>>> So why do we need to replace __releases/__acquires with __must_hold?
>>> This is not clear.
>>
>> __must_hold() has the same meaning as __releases() + __acquires(). The
>> only difference between these two annotations is that __must_hold() is
>> more compact. I can leave out this change if this change is considered
>> confusing.
>>
> Hmm, but per the documentation of __must_hold() it seems that this 
> attribute signifies the caller of the function must hold the 
> exclusive context lock. On contrary, __releases() attribute is meant 
> to be used when function releases a context lock exclusively and 
> __acquires() should be used when function acquires context lock 
> exclusively. From the above rational, it seems that
> null_insert_page() should have all the above three attributes
> applicable, isn't it?

__must_hold() is defined as __attribute__((requires_capability(...)))
In the Clang documentation this capability is called REQUIRES(). From
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html: "REQUIRES is an 
attribute on functions, methods or function parameters of reference to
SCOPED_CAPABILITY-annotated type, which declares that the calling thread
must have exclusive access to the given capabilities. More than one
capability may be specified. The capabilities must be held on entry to
the function, and must still be held on exit."

To me the above text means that the __must_hold() annotation is sufficient.

> I am not sure why did compiler generate the above errors? If I look at the
> macro definition then it seems __context_unsafe(/* comment */) should 
> simply
> expand to __no_context_analysis.
> 
> // From include/linux/compiler-context-analysis.h:
> 
> #define __context_unsafe(comment) __no_context_analysis

I misread your previous comment. The output I shared is produced when 
the function body is surrounded with context_unsafe() (no leading
underscores). Your request was to annotate the function with
__context_unsafe() (with two leading underscores).

I will consider to use __context_unsafe() instead of 
__no_context_analysis and a comment.

Thanks,

Bart.