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

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

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?

>>> diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/ zoned.c
>>> index 384bdce6a9b7..a7f94e76034f 100644
>>> --- a/drivers/block/null_blk/zoned.c
>>> +++ b/drivers/block/null_blk/zoned.c
>>> @@ -32,6 +32,7 @@ static inline void null_init_zone_lock(struct nullb_device *dev,
>>>   static inline void null_lock_zone(struct nullb_device *dev,
>>>                     struct nullb_zone *zone)
>>> +    __no_context_analysis /* conditional locking */
>>
>> I see we already have __context_unsafe(comment) macro defined
>> which may be used instead for inline comment.
> 
> context_unsafe() suppresses warnings triggered by __guarded_by() and
> __guarded_by_ptr() but does not suppress analysis of functions in the
> function body annotated with __releases() and __acquired(). If I remove
> the __no_context_analysis annotation and surround the function body with
> context_unsafe(), the following appears:
> 
> drivers/block/null_blk/zoned.c:37:2: error: spinlock 'zone->.spinlock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
>     37 |         context_unsafe(
>        |         ^
> ./include/linux/compiler-context-analysis.h:201:30: note: expanded from macro 'context_unsafe'
>    201 | #define context_unsafe(...)             \
>        |                                         ^
> drivers/block/null_blk/zoned.c:39:3: note: spinlock acquired here
>     39 |                 spin_lock_irq(&zone->spinlock);
>        |                 ^
> 
> drivers/block/null_blk/zoned.c:37:2: error: mutex 'zone->.mutex' is not held on every path through here [-Werror,-Wthread-safety-analysis]
>     37 |         context_unsafe(
>        |         ^
> ./include/linux/compiler-context-analysis.h:201:30: note: expanded from macro 'context_unsafe'
>    201 | #define context_unsafe(...)             \
>        |                                         ^
> drivers/block/null_blk/zoned.c:41:3: note: mutex acquired here
>     41 |                 mutex_lock(&zone->mutex);
>        |                 ^
> 2 errors generated.
> 
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

Thanks,
--Nilay