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

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Add __must_hold() annotations where these are missing. Because
> null_lock_zone() and null_unlock_zone() use conditional locking, instead
> of annotating these functions, introduce guard class null_zone. Replace
> null_lock_zone() and null_unlock_zone() calls with scoped_guard(null_zone,
> ...). Enable lock context analysis in the Makefile.
> 
> Signed-off-by: Bart Van Assche<[email protected]>
> ---
>   drivers/block/null_blk/Makefile |   2 +
>   drivers/block/null_blk/main.c   |   4 ++
>   drivers/block/null_blk/zoned.c  | 120 ++++++++++++++------------------
>   3 files changed, 57 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/block/null_blk/Makefile b/drivers/block/null_blk/Makefile
> index 84c36e512ab8..282b0d51a477 100644
> --- a/drivers/block/null_blk/Makefile
> +++ b/drivers/block/null_blk/Makefile
> @@ -1,5 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   
> +CONTEXT_ANALYSIS := y
> +
>   # needed for trace events
>   ccflags-y			+= -I$(src)
>   
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index f8c0fd57e041..baffc6c61178 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1038,6 +1038,7 @@ static struct nullb_page *null_insert_page(struct nullb *nullb,
>   }
>   
>   static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
> +	__must_hold(&nullb->lock)
>   {
>   	int i;
>   	unsigned int offset;
> @@ -1087,6 +1088,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
>   }
>   
>   static int null_make_cache_space(struct nullb *nullb, unsigned long n)
> +	__must_hold(&nullb->lock)
>   {
>   	int i, err, nr_pages;
>   	struct nullb_page *c_pages[FREE_BATCH];
> @@ -1141,6 +1143,7 @@ static int null_make_cache_space(struct nullb *nullb, unsigned long n)
>   
>   static blk_status_t copy_to_nullb(struct nullb *nullb, void *source,
>   				  loff_t pos, size_t n, bool is_fua)
> +	__must_hold(&nullb->lock)
>   {
>   	size_t temp, count = 0;
>   	struct nullb_page *t_page;
> @@ -1242,6 +1245,7 @@ static blk_status_t null_handle_flush(struct nullb *nullb)
>   static blk_status_t null_transfer(struct nullb *nullb, struct page *page,
>   	unsigned int len, unsigned int off, bool is_write, loff_t pos,
>   	bool is_fua)
> +	__must_hold(&nullb->lock)
>   {
>   	struct nullb_device *dev = nullb->dev;
>   	blk_status_t err = BLK_STS_OK;
> diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c
> index 384bdce6a9b7..dbae748c90ba 100644
> --- a/drivers/block/null_blk/zoned.c
> +++ b/drivers/block/null_blk/zoned.c
> @@ -30,23 +30,26 @@ static inline void null_init_zone_lock(struct nullb_device *dev,
>   		mutex_init(&zone->mutex);
>   }
>   
> -static inline void null_lock_zone(struct nullb_device *dev,
> -				  struct nullb_zone *zone)
> -{
> -	if (!dev->memory_backed)
> -		spin_lock_irq(&zone->spinlock);
> -	else
> -		mutex_lock(&zone->mutex);
> -}
> -
> -static inline void null_unlock_zone(struct nullb_device *dev,
> -				    struct nullb_zone *zone)
> -{
> -	if (!dev->memory_backed)
> -		spin_unlock_irq(&zone->spinlock);
> -	else
> -		mutex_unlock(&zone->mutex);
> -}
> +struct nullb_dev_and_zone {
> +	struct nullb_device *dev;
> +	struct nullb_zone *zone;
> +};
> +
> +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? Since both the constructor
and destructor are marked __context_unsafe, the lock context analysis still
won't reason about the conditional locking.

In the previous series, we annotated null_lock_zone() and null_unlock_zone()
with __context_unsafe, and from that perspective the semantics seem unchanged
by introducing the guard class.

I'm not opposed to using a guard class here, but it does make the code a bit
more complex to follow. Unless there is a specific benefit (beyond using
scoped_guard()), I think the previous approach was simpler.

Thanks,
--Nilay