Re: [PATCH v2 06/12] null_blk: Enable lock context analysis

On 8/3/26 11:39 PM, Bart Van Assche wrote:
> 
> On 8/3/26 6:35 AM, Nilay Shroff wrote:
>> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>>> +DEFINE_CLASS(null_zone, struct nullb_dev_and_zone, ({
>>> +             if (!_T.dev->memory_backed)
>>> +                 spin_unlock_irq(&_T.zone->spinlock);
>>> +             else
>>> +                 mutex_unlock(&_T.zone->mutex);
>>> +         }), ({
>>> +             if (!dev->memory_backed)
>>> +                 spin_lock_irq(&zone->spinlock);
>>> +             else
>>> +                 mutex_lock(&zone->mutex);
>>> +             (struct nullb_dev_and_zone){ dev, zone };
>>> +         }),
>>> +         struct nullb_device *dev, struct nullb_zone *zone)
>>> +
>>> +DEFINE_CLASS_IS_UNCONDITIONAL(null_zone)
>>
>> What do we gain by introducing a guard class here?
> 
> What we gain is that __context_unsafe() annotations are avoided
> entirely. __context_unsafe() is a big hammer that shouldn't be used
> unless absolutely necessary. Additionally, introducing this class makes
> it possible to convert multiple goto statements into return statements.
> I think that's a significant improvement.
> 
That's a fair statement.

>> Since both the constructor and destructor are marked
>> __context_unsafe, the lock context analysis still
>> won't reason about the conditional locking.
> 
> Such reasoning is not needed when using guard() or scoped_guard() since
> these macro's make it impossible to leave a scope without releasing the
> acquired synchronization object.
> 
Okay got it.

>> I'm not opposed to using a guard class here, but it does make the
>> code a bit more complex to follow.
> 
> Hmm ... I think this means that you are not familiar yet with
> DEFINE_CLASS(). The code that uses the null_zone class becomes simpler.
> 
Yes, I may just need to get more familiar with DEFINE_CLASS() :-)

Regarding the proposed null_zone class implementation, could we at least
move the constructor and destructor logic into separate helper functions
instead of embedding it directly in the DEFINE_CLASS() arguments?

I understand that DEFINE_CLASS() supports statement expressions for these
arguments, but with the conditional spinlock/mutex handling here, I find
separate helpers easier to read and follow. For example, the class definition
itself would then remain fairly compact while the locking logic is kept in
regular functions. What do you think?

Thanks,
--Nilay