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

[Nilay Shroff <nilay@linux.ibm.com>]

On 4/1/26 1:32 AM, Bart Van Assche wrote:
> 
> 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.
> 

Okay, that makes sense — using only __must_hold() is reasonable.
That said, since this function temporarily releases and then re-acquires
the lock, it may not be entirely redundant to also annotate it with
__releases() and __acquires(). Those annotations more explicitly describe
the lock transitions happening within the function body.

Still, I’m fine with keeping just __must_hold() if we prefer a more concise
annotation.

>> 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,
--Nilay