[PATCH v2 00/12] Enable lock context analysis in drivers/block/

[PATCH v2 00/12] Enable lock context analysis in drivers/block/

[Bart Van Assche]

Hi Jens,

This patch series enables lock context analysis in all block drivers in the
drivers/block/ directory except DRBD. Please consider these patches for the
next merge window.

Thank you,

Bart.

Changes compared to v1:
 - Moved the DRBD changes into a separate patch series.
 - Combined two patches for the loop driver into a single patch.
 - Also for the loop driver, added a __must_hold() annotation to
   __loop_change_fd() and __loop_configure(). Added a __guarded_by()
   annotation to loop_index_idr.
 - Added a __guarded_by() annotation to nbd_index_idr.
 - Introduced the null_zone locking "class" in the null_blk driver.
 - Fixed the rbd patch description.
 - Added a __cond_acquires() annotation to a new function in the ublk driver.

Bart Van Assche (12):
  aoe: Enable lock context analysis
  loop: Remove the "bool global" function argument
  loop: Add lock context annotations
  mtip32: Enable lock context analysis
  nbd: Enable lock context analysis
  null_blk: Enable lock context analysis
  rbd: Enable lock context analysis
  ublk: Enable lock context analysis
  xen-blkback: Enable lock context analysis
  zram: Enable lock context analysis
  rnbd: Enable lock context analysis
  block: Enable lock context analysis for all block drivers

 drivers/block/Makefile             |   2 +
 drivers/block/aoe/Makefile         |   2 +
 drivers/block/aoe/aoecmd.c         |   1 +
 drivers/block/loop.c               | 256 ++++++++++++++++-------------
 drivers/block/mtip32xx/Makefile    |   2 +
 drivers/block/nbd.c                |  11 +-
 drivers/block/null_blk/Makefile    |   2 +
 drivers/block/null_blk/main.c      |   4 +
 drivers/block/null_blk/zoned.c     | 120 ++++++--------
 drivers/block/rbd.c                |   8 +
 drivers/block/rnbd/Makefile        |   2 +
 drivers/block/ublk_drv.c           |   7 +
 drivers/block/xen-blkback/Makefile |   3 +
 drivers/block/zram/Makefile        |   2 +
 drivers/block/zram/zcomp.c         |   3 +-
 drivers/block/zram/zcomp.h         |   6 +-
 16 files changed, 245 insertions(+), 186 deletions(-)

[PATCH v2 01/12] aoe: Enable lock context analysis

[Bart Van Assche]

Add a missing __must_hold() annotation. Enable lock context analysis in the
Makefile.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/aoe/Makefile | 2 ++
 drivers/block/aoe/aoecmd.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/block/aoe/Makefile b/drivers/block/aoe/Makefile
index b7545ce2f1b0..27bff6359a56 100644
--- a/drivers/block/aoe/Makefile
+++ b/drivers/block/aoe/Makefile
@@ -3,5 +3,7 @@
 # Makefile for ATA over Ethernet
 #
 
+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_ATA_OVER_ETH)	+= aoe.o
 aoe-y := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index a4744a30a8af..54c57b9f8894 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -1193,6 +1193,7 @@ noskb:		if (buf)
  */
 static int
 ktio(int id)
+	__must_hold(&iocq[id].lock)
 {
 	struct frame *f;
 	struct list_head *pos;

Re: [PATCH v2 01/12] aoe: Enable lock context analysis

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Add a missing __must_hold() annotation. Enable lock context analysis in the
> Makefile.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/aoe/Makefile | 2 ++
>   drivers/block/aoe/aoecmd.c | 1 +
>   2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/block/aoe/Makefile b/drivers/block/aoe/Makefile
> index b7545ce2f1b0..27bff6359a56 100644
> --- a/drivers/block/aoe/Makefile
> +++ b/drivers/block/aoe/Makefile
> @@ -3,5 +3,7 @@
>   # Makefile for ATA over Ethernet
>   #
>   
> +CONTEXT_ANALYSIS := y
> +
>   obj-$(CONFIG_ATA_OVER_ETH)	+= aoe.o
>   aoe-y := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o
> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
> index a4744a30a8af..54c57b9f8894 100644
> --- a/drivers/block/aoe/aoecmd.c
> +++ b/drivers/block/aoe/aoecmd.c
> @@ -1193,6 +1193,7 @@ noskb:		if (buf)
>    */
>   static int
>   ktio(int id)
> +	__must_hold(&iocq[id].lock)
>   {
>   	struct frame *f;
>   	struct list_head *pos;

As I see, iocq->lock guards iocq->head. So shouldn't we annotate
iocq->head with __guarded_by(iocq->lock)? For instance,

struct iocq_ktio {
	struct list_head head;
                 __guarded_by(&lock);
	spinlock_t lock;
};

Thanks,
--Nilay

Re: [PATCH v2 01/12] aoe: Enable lock context analysis

[Bart Van Assche]

On 8/3/26 5:53 AM, Nilay Shroff wrote:
> As I see, iocq->lock guards iocq->head. So shouldn't we annotate
> iocq->head with __guarded_by(iocq->lock)? For instance,
> 
> struct iocq_ktio {
>      struct list_head head;
>                  __guarded_by(&lock);
>      spinlock_t lock;
> };

How about integrating the change below into this patch?

diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index 54c57b9f8894..7ceda4ee4e55 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -51,8 +51,8 @@ static struct ktstate *kts;

  /* io completion queue */
  struct iocq_ktio {
-	struct list_head head;
  	spinlock_t lock;
+	struct list_head head __guarded_by(&lock);
  };
  static struct iocq_ktio *iocq;

@@ -1717,8 +1717,8 @@ aoecmd_init(void)
  	}

  	for (i = 0; i < ncpus; i++) {
-		INIT_LIST_HEAD(&iocq[i].head);
-		spin_lock_init(&iocq[i].lock);
+		scoped_guard(spinlock_init, &iocq[i].lock)
+			INIT_LIST_HEAD(&iocq[i].head);
  		init_waitqueue_head(&ktiowq[i]);
  		snprintf(kts[i].name, sizeof(kts[i].name), "aoe_ktio%d", i);
  		kts[i].fn = ktio;

Thanks,

Bart.

Re: [PATCH v2 01/12] aoe: Enable lock context analysis

[Nilay Shroff]

On 8/3/26 10:54 PM, Bart Van Assche wrote:
> On 8/3/26 5:53 AM, Nilay Shroff wrote:
>> As I see, iocq->lock guards iocq->head. So shouldn't we annotate
>> iocq->head with __guarded_by(iocq->lock)? For instance,
>>
>> struct iocq_ktio {
>>      struct list_head head;
>>                  __guarded_by(&lock);
>>      spinlock_t lock;
>> };
> 
> How about integrating the change below into this patch?
> 
> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
> index 54c57b9f8894..7ceda4ee4e55 100644
> --- a/drivers/block/aoe/aoecmd.c
> +++ b/drivers/block/aoe/aoecmd.c
> @@ -51,8 +51,8 @@ static struct ktstate *kts;
> 
>   /* io completion queue */
>   struct iocq_ktio {
> -    struct list_head head;
>       spinlock_t lock;
> +    struct list_head head __guarded_by(&lock);
>   };
>   static struct iocq_ktio *iocq;
> 
> @@ -1717,8 +1717,8 @@ aoecmd_init(void)
>       }
> 
>       for (i = 0; i < ncpus; i++) {
> -        INIT_LIST_HEAD(&iocq[i].head);
> -        spin_lock_init(&iocq[i].lock);
> +        scoped_guard(spinlock_init, &iocq[i].lock)
> +            INIT_LIST_HEAD(&iocq[i].head);

This looks good to me, except for the use of scoped_guard() here.
Christoph has previously expressed a preference against using
scoped_guard():
https://lore.kernel.org/all/20260626063738.GD10731@lst.de/

So lets just see what's his opinion on this change.

Thanks,
--Nilay

[PATCH v2 02/12] loop: Remove the "bool global" function argument

[Bart Van Assche]

Move the code that is protected by a mutex from loop_change_fd() into
__loop_change_fd(). Move the code that is protected by a mutex from
loop_configure() into __loop_configure(). Keep the behavior in
loop_global_lock_killable() for the global == true case. Expand
loop_global_lock_killable(lo, false) calls into a mutex_lock_killable()
and a mutex_unlock() call. This patch prepares for adding lock context
annotations.

Cc: Haris Iqbal <haris.iqbal@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/loop.c | 249 ++++++++++++++++++++++++-------------------
 1 file changed, 138 insertions(+), 111 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1faecef33009..a71fe763c933 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -98,25 +98,22 @@ static DEFINE_MUTEX(loop_validate_mutex);
  * loop_global_lock_killable() - take locks for safe loop_validate_file() test
  *
  * @lo: struct loop_device
- * @global: true if @lo is about to bind another "struct loop_device", false otherwise
  *
  * Returns 0 on success, -EINTR otherwise.
  *
- * Since loop_validate_file() traverses on other "struct loop_device" if
- * is_loop_device() is true, we need a global lock for serializing concurrent
+ * Since loop_validate_file() traverses on other "struct loop_device", we need a
+ * global lock for serializing concurrent
  * loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
  */
-static int loop_global_lock_killable(struct loop_device *lo, bool global)
+static int loop_global_lock_killable(struct loop_device *lo)
 {
 	int err;
 
-	if (global) {
-		err = mutex_lock_killable(&loop_validate_mutex);
-		if (err)
-			return err;
-	}
+	err = mutex_lock_killable(&loop_validate_mutex);
+	if (err)
+		return err;
 	err = mutex_lock_killable(&lo->lo_mutex);
-	if (err && global)
+	if (err)
 		mutex_unlock(&loop_validate_mutex);
 	return err;
 }
@@ -125,13 +122,11 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
  * loop_global_unlock() - release locks taken by loop_global_lock_killable()
  *
  * @lo: struct loop_device
- * @global: true if @lo was about to bind another "struct loop_device", false otherwise
  */
-static void loop_global_unlock(struct loop_device *lo, bool global)
+static void loop_global_unlock(struct loop_device *lo)
 {
 	mutex_unlock(&lo->lo_mutex);
-	if (global)
-		mutex_unlock(&loop_validate_mutex);
+	mutex_unlock(&loop_validate_mutex);
 }
 
 static int max_part;
@@ -523,6 +518,50 @@ static int loop_check_backing_file(struct file *file)
 	return 0;
 }
 
+static int __loop_change_fd(struct loop_device *lo, struct block_device *bdev,
+			    struct file *file, struct file **old_file,
+			    bool *partscan)
+	__must_hold(&lo->lo_mutex)
+{
+	unsigned int memflags;
+	int error;
+
+	if (lo->lo_state != Lo_bound)
+		return -ENXIO;
+
+	/* the loop device has to be read-only */
+	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
+		return -EINVAL;
+
+	error = loop_validate_file(file, bdev);
+	if (error)
+		return error;
+
+	*old_file = lo->lo_backing_file;
+
+	/* size of the new backing store needs to be the same */
+	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, *old_file))
+		return -EINVAL;
+
+	/*
+	 * We might switch to direct I/O mode for the loop device, write back
+	 * all dirty data the page cache now that so that the individual I/O
+	 * operations don't have to do that.
+	 */
+	vfs_fsync(file, 0);
+
+	/* and ... switch */
+	disk_force_media_change(lo->lo_disk);
+	memflags = blk_mq_freeze_queue(lo->lo_queue);
+	mapping_set_gfp_mask((*old_file)->f_mapping, lo->old_gfp_mask);
+	loop_assign_backing_file(lo, file);
+	loop_update_dio(lo);
+	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
+	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -536,7 +575,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 {
 	struct file *file = fget(arg);
 	struct file *old_file;
-	unsigned int memflags;
 	int error;
 	bool partscan;
 	bool is_loop;
@@ -554,46 +592,21 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
 	is_loop = is_loop_device(file);
-	error = loop_global_lock_killable(lo, is_loop);
+	if (is_loop) {
+		error = loop_global_lock_killable(lo);
+		if (error)
+			goto out_putf;
+		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+		loop_global_unlock(lo);
+	} else {
+		error = mutex_lock_killable(&lo->lo_mutex);
+		if (error)
+			goto out_putf;
+		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+		mutex_unlock(&lo->lo_mutex);
+	}
 	if (error)
 		goto out_putf;
-	error = -ENXIO;
-	if (lo->lo_state != Lo_bound)
-		goto out_err;
-
-	/* the loop device has to be read-only */
-	error = -EINVAL;
-	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
-		goto out_err;
-
-	error = loop_validate_file(file, bdev);
-	if (error)
-		goto out_err;
-
-	old_file = lo->lo_backing_file;
-
-	error = -EINVAL;
-
-	/* size of the new backing store needs to be the same */
-	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, old_file))
-		goto out_err;
-
-	/*
-	 * We might switch to direct I/O mode for the loop device, write back
-	 * all dirty data the page cache now that so that the individual I/O
-	 * operations don't have to do that.
-	 */
-	vfs_fsync(file, 0);
-
-	/* and ... switch */
-	disk_force_media_change(lo->lo_disk);
-	memflags = blk_mq_freeze_queue(lo->lo_queue);
-	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
-	loop_assign_backing_file(lo, file);
-	loop_update_dio(lo);
-	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
-	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	loop_global_unlock(lo, is_loop);
 
 	/*
 	 * Flush loop_validate_file() before fput(), for l->lo_backing_file
@@ -618,8 +631,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 	return error;
 
-out_err:
-	loop_global_unlock(lo, is_loop);
 out_putf:
 	fput(file);
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
@@ -974,61 +985,29 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
 		lim->discard_granularity = 0;
 }
 
-static int loop_configure(struct loop_device *lo, blk_mode_t mode,
-			  struct block_device *bdev,
-			  const struct loop_config *config)
+static int __loop_configure(struct loop_device *lo, blk_mode_t mode,
+			    struct block_device *bdev,
+			    const struct loop_config *config, struct file *file,
+			    bool *partscan)
+	__must_hold(&lo->lo_mutex)
 {
-	struct file *file = fget(config->fd);
 	struct queue_limits lim;
-	int error;
 	loff_t size;
-	bool partscan;
-	bool is_loop;
-
-	if (!file)
-		return -EBADF;
-
-	error = loop_check_backing_file(file);
-	if (error) {
-		fput(file);
-		return error;
-	}
-
-	is_loop = is_loop_device(file);
-
-	/* This is safe, since we have a reference from open(). */
-	__module_get(THIS_MODULE);
-
-	/*
-	 * If we don't hold exclusive handle for the device, upgrade to it
-	 * here to avoid changing device under exclusive owner.
-	 */
-	if (!(mode & BLK_OPEN_EXCL)) {
-		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
-		if (error)
-			goto out_putf;
-	}
-
-	error = loop_global_lock_killable(lo, is_loop);
-	if (error)
-		goto out_bdev;
+	int error;
 
-	error = -EBUSY;
 	if (lo->lo_state != Lo_unbound)
-		goto out_unlock;
+		return -EBUSY;
 
 	error = loop_validate_file(file, bdev);
 	if (error)
-		goto out_unlock;
+		return error;
 
-	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
-		error = -EINVAL;
-		goto out_unlock;
-	}
+	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0)
+		return -EINVAL;
 
 	error = loop_set_status_from_info(lo, &config->info);
 	if (error)
-		goto out_unlock;
+		return error;
 	lo->lo_flags = config->info.lo_flags;
 
 	if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||
@@ -1039,10 +1018,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 		lo->workqueue = alloc_workqueue("loop%d",
 						WQ_UNBOUND | WQ_FREEZABLE,
 						0, lo->lo_number);
-		if (!lo->workqueue) {
-			error = -ENOMEM;
-			goto out_unlock;
-		}
+		if (!lo->workqueue)
+			return -ENOMEM;
 	}
 
 	/* suppress uevents while reconfiguring the device */
@@ -1059,7 +1036,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	/* No need to freeze the queue as the device isn't bound yet. */
 	error = queue_limits_commit_update(lo->lo_queue, &lim);
 	if (error)
-		goto out_unlock;
+		return error;
 
 	/*
 	 * We might switch to direct I/O mode for the loop device, write back
@@ -1080,14 +1057,66 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	WRITE_ONCE(lo->lo_state, Lo_bound);
 	if (part_shift)
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
-	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	if (partscan)
+	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+	if (*partscan)
 		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 
-	loop_global_unlock(lo, is_loop);
+	return 0;
+}
+
+static int loop_configure(struct loop_device *lo, blk_mode_t mode,
+			  struct block_device *bdev,
+			  const struct loop_config *config)
+{
+	struct file *file = fget(config->fd);
+	int error;
+	bool partscan;
+	bool is_loop;
+
+	if (!file)
+		return -EBADF;
+
+	error = loop_check_backing_file(file);
+	if (error) {
+		fput(file);
+		return error;
+	}
+
+	is_loop = is_loop_device(file);
+
+	/* This is safe, since we have a reference from open(). */
+	__module_get(THIS_MODULE);
+
+	/*
+	 * If we don't hold exclusive handle for the device, upgrade to it
+	 * here to avoid changing device under exclusive owner.
+	 */
+	if (!(mode & BLK_OPEN_EXCL)) {
+		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
+		if (error)
+			goto out_putf;
+	}
+
+	if (is_loop) {
+		error = loop_global_lock_killable(lo);
+		if (error)
+			goto out_bdev;
+		error = __loop_configure(lo, mode, bdev, config, file,
+					 &partscan);
+		loop_global_unlock(lo);
+	} else {
+		error = mutex_lock_killable(&lo->lo_mutex);
+		if (error)
+			goto out_bdev;
+		error = __loop_configure(lo, mode, bdev, config, file,
+					 &partscan);
+		mutex_unlock(&lo->lo_mutex);
+	}
+	if (error)
+		goto out_bdev;
 	if (partscan)
 		loop_reread_partitions(lo);
 
@@ -1096,8 +1125,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	return 0;
 
-out_unlock:
-	loop_global_unlock(lo, is_loop);
 out_bdev:
 	if (!(mode & BLK_OPEN_EXCL))
 		bd_abort_claiming(bdev, loop_configure);
@@ -1194,11 +1221,11 @@ static int loop_clr_fd(struct loop_device *lo)
 	 * which loop_configure()/loop_change_fd() found via fget() was this
 	 * loop device.
 	 */
-	err = loop_global_lock_killable(lo, true);
+	err = loop_global_lock_killable(lo);
 	if (err)
 		return err;
 	if (lo->lo_state != Lo_bound) {
-		loop_global_unlock(lo, true);
+		loop_global_unlock(lo);
 		return -ENXIO;
 	}
 	/*
@@ -1210,7 +1237,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
 	if (disk_openers(lo->lo_disk) == 1)
 		WRITE_ONCE(lo->lo_state, Lo_rundown);
-	loop_global_unlock(lo, true);
+	loop_global_unlock(lo);
 
 	return 0;
 }

Re: [PATCH v2 02/12] loop: Remove the "bool global" function argument

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Move the code that is protected by a mutex from loop_change_fd() into
> __loop_change_fd(). Move the code that is protected by a mutex from
> loop_configure() into __loop_configure(). Keep the behavior in
> loop_global_lock_killable() for the global == true case. Expand
> loop_global_lock_killable(lo, false) calls into a mutex_lock_killable()
> and a mutex_unlock() call. This patch prepares for adding lock context
> annotations.
> 
> Cc: Haris Iqbal <haris.iqbal@linux.dev>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/loop.c | 249 ++++++++++++++++++++++++-------------------
>   1 file changed, 138 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1faecef33009..a71fe763c933 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -98,25 +98,22 @@ static DEFINE_MUTEX(loop_validate_mutex);
>    * loop_global_lock_killable() - take locks for safe loop_validate_file() test
>    *
>    * @lo: struct loop_device
> - * @global: true if @lo is about to bind another "struct loop_device", false otherwise
>    *
>    * Returns 0 on success, -EINTR otherwise.
>    *
> - * Since loop_validate_file() traverses on other "struct loop_device" if
> - * is_loop_device() is true, we need a global lock for serializing concurrent
> + * Since loop_validate_file() traverses on other "struct loop_device", we need a
> + * global lock for serializing concurrent
>    * loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
>    */
> -static int loop_global_lock_killable(struct loop_device *lo, bool global)
> +static int loop_global_lock_killable(struct loop_device *lo)
>   {
>   	int err;
>   
> -	if (global) {
> -		err = mutex_lock_killable(&loop_validate_mutex);
> -		if (err)
> -			return err;
> -	}
> +	err = mutex_lock_killable(&loop_validate_mutex);
> +	if (err)
> +		return err;
>   	err = mutex_lock_killable(&lo->lo_mutex);
> -	if (err && global)
> +	if (err)
>   		mutex_unlock(&loop_validate_mutex);
>   	return err;
>   }
> @@ -125,13 +122,11 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
>    * loop_global_unlock() - release locks taken by loop_global_lock_killable()
>    *
>    * @lo: struct loop_device
> - * @global: true if @lo was about to bind another "struct loop_device", false otherwise
>    */
> -static void loop_global_unlock(struct loop_device *lo, bool global)
> +static void loop_global_unlock(struct loop_device *lo)
>   {
>   	mutex_unlock(&lo->lo_mutex);
> -	if (global)
> -		mutex_unlock(&loop_validate_mutex);
> +	mutex_unlock(&loop_validate_mutex);
>   }
>   
>   static int max_part;
> @@ -523,6 +518,50 @@ static int loop_check_backing_file(struct file *file)
>   	return 0;
>   }
>   
> +static int __loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> +			    struct file *file, struct file **old_file,
> +			    bool *partscan)
> +	__must_hold(&lo->lo_mutex)
> +{
> +	unsigned int memflags;
> +	int error;
> +
> +	if (lo->lo_state != Lo_bound)
> +		return -ENXIO;
> +
> +	/* the loop device has to be read-only */
> +	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
> +		return -EINVAL;
> +
> +	error = loop_validate_file(file, bdev);
> +	if (error)
> +		return error;
> +
> +	*old_file = lo->lo_backing_file;
> +
> +	/* size of the new backing store needs to be the same */
> +	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, *old_file))
> +		return -EINVAL;
> +
> +	/*
> +	 * We might switch to direct I/O mode for the loop device, write back
> +	 * all dirty data the page cache now that so that the individual I/O
> +	 * operations don't have to do that.
> +	 */
> +	vfs_fsync(file, 0);
> +
> +	/* and ... switch */
> +	disk_force_media_change(lo->lo_disk);
> +	memflags = blk_mq_freeze_queue(lo->lo_queue);
> +	mapping_set_gfp_mask((*old_file)->f_mapping, lo->old_gfp_mask);
> +	loop_assign_backing_file(lo, file);
> +	loop_update_dio(lo);
> +	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
> +	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> +
> +	return 0;
> +}
> +
>   /*
>    * loop_change_fd switched the backing store of a loopback device to
>    * a new file. This is useful for operating system installers to free up
> @@ -536,7 +575,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   {
>   	struct file *file = fget(arg);
>   	struct file *old_file;
> -	unsigned int memflags;
>   	int error;
>   	bool partscan;
>   	bool is_loop;
> @@ -554,46 +592,21 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
>   
>   	is_loop = is_loop_device(file);
> -	error = loop_global_lock_killable(lo, is_loop);
> +	if (is_loop) {
> +		error = loop_global_lock_killable(lo);
> +		if (error)
> +			goto out_putf;
> +		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
> +		loop_global_unlock(lo);
> +	} else {
> +		error = mutex_lock_killable(&lo->lo_mutex);
> +		if (error)
> +			goto out_putf;
> +		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
> +		mutex_unlock(&lo->lo_mutex);
> +	}
>   	if (error)
>   		goto out_putf;
> -	error = -ENXIO;
> -	if (lo->lo_state != Lo_bound)
> -		goto out_err;
> -
> -	/* the loop device has to be read-only */
> -	error = -EINVAL;
> -	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
> -		goto out_err;
> -
> -	error = loop_validate_file(file, bdev);
> -	if (error)
> -		goto out_err;
> -
> -	old_file = lo->lo_backing_file;
> -
> -	error = -EINVAL;
> -
> -	/* size of the new backing store needs to be the same */
> -	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, old_file))
> -		goto out_err;
> -
> -	/*
> -	 * We might switch to direct I/O mode for the loop device, write back
> -	 * all dirty data the page cache now that so that the individual I/O
> -	 * operations don't have to do that.
> -	 */
> -	vfs_fsync(file, 0);
> -
> -	/* and ... switch */
> -	disk_force_media_change(lo->lo_disk);
> -	memflags = blk_mq_freeze_queue(lo->lo_queue);
> -	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
> -	loop_assign_backing_file(lo, file);
> -	loop_update_dio(lo);
> -	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
> -	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> -	loop_global_unlock(lo, is_loop);
>   
>   	/*
>   	 * Flush loop_validate_file() before fput(), for l->lo_backing_file
> @@ -618,8 +631,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   	return error;
>   
> -out_err:
> -	loop_global_unlock(lo, is_loop);
>   out_putf:
>   	fput(file);
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> @@ -974,61 +985,29 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
>   		lim->discard_granularity = 0;
>   }
>   
> -static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> -			  struct block_device *bdev,
> -			  const struct loop_config *config)
> +static int __loop_configure(struct loop_device *lo, blk_mode_t mode,
> +			    struct block_device *bdev,
> +			    const struct loop_config *config, struct file *file,
> +			    bool *partscan)
> +	__must_hold(&lo->lo_mutex)
>   {
> -	struct file *file = fget(config->fd);
>   	struct queue_limits lim;
> -	int error;
>   	loff_t size;
> -	bool partscan;
> -	bool is_loop;
> -
> -	if (!file)
> -		return -EBADF;
> -
> -	error = loop_check_backing_file(file);
> -	if (error) {
> -		fput(file);
> -		return error;
> -	}
> -
> -	is_loop = is_loop_device(file);
> -
> -	/* This is safe, since we have a reference from open(). */
> -	__module_get(THIS_MODULE);
> -
> -	/*
> -	 * If we don't hold exclusive handle for the device, upgrade to it
> -	 * here to avoid changing device under exclusive owner.
> -	 */
> -	if (!(mode & BLK_OPEN_EXCL)) {
> -		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
> -		if (error)
> -			goto out_putf;
> -	}
> -
> -	error = loop_global_lock_killable(lo, is_loop);
> -	if (error)
> -		goto out_bdev;
> +	int error;
>   
> -	error = -EBUSY;
>   	if (lo->lo_state != Lo_unbound)
> -		goto out_unlock;
> +		return -EBUSY;
>   
>   	error = loop_validate_file(file, bdev);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   
> -	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
> -		error = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0)
> +		return -EINVAL;
>   
>   	error = loop_set_status_from_info(lo, &config->info);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   	lo->lo_flags = config->info.lo_flags;
>   
>   	if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||
> @@ -1039,10 +1018,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   		lo->workqueue = alloc_workqueue("loop%d",
>   						WQ_UNBOUND | WQ_FREEZABLE,
>   						0, lo->lo_number);
> -		if (!lo->workqueue) {
> -			error = -ENOMEM;
> -			goto out_unlock;
> -		}
> +		if (!lo->workqueue)
> +			return -ENOMEM;
>   	}
>   
>   	/* suppress uevents while reconfiguring the device */
> @@ -1059,7 +1036,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   	/* No need to freeze the queue as the device isn't bound yet. */
>   	error = queue_limits_commit_update(lo->lo_queue, &lim);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   
>   	/*
>   	 * We might switch to direct I/O mode for the loop device, write back
> @@ -1080,14 +1057,66 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   	WRITE_ONCE(lo->lo_state, Lo_bound);
>   	if (part_shift)
>   		lo->lo_flags |= LO_FLAGS_PARTSCAN;
> -	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> -	if (partscan)
> +	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> +	if (*partscan)
>   		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>   
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   
> -	loop_global_unlock(lo, is_loop);
> +	return 0;
> +}
> +
> +static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> +			  struct block_device *bdev,
> +			  const struct loop_config *config)
> +{
> +	struct file *file = fget(config->fd);
> +	int error;
> +	bool partscan;
> +	bool is_loop;
> +
> +	if (!file)
> +		return -EBADF;
> +
> +	error = loop_check_backing_file(file);
> +	if (error) {
> +		fput(file);
> +		return error;
> +	}
> +
> +	is_loop = is_loop_device(file);
> +
> +	/* This is safe, since we have a reference from open(). */
> +	__module_get(THIS_MODULE);
> +
> +	/*
> +	 * If we don't hold exclusive handle for the device, upgrade to it
> +	 * here to avoid changing device under exclusive owner.
> +	 */
> +	if (!(mode & BLK_OPEN_EXCL)) {
> +		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
> +		if (error)
> +			goto out_putf;
> +	}
> +
> +	if (is_loop) {
> +		error = loop_global_lock_killable(lo);
> +		if (error)
> +			goto out_bdev;
> +		error = __loop_configure(lo, mode, bdev, config, file,
> +					 &partscan);
> +		loop_global_unlock(lo);
> +	} else {
> +		error = mutex_lock_killable(&lo->lo_mutex);
> +		if (error)
> +			goto out_bdev;
> +		error = __loop_configure(lo, mode, bdev, config, file,
> +					 &partscan);
> +		mutex_unlock(&lo->lo_mutex);
> +	}
> +	if (error)
> +		goto out_bdev;
>   	if (partscan)
>   		loop_reread_partitions(lo);
>   
> @@ -1096,8 +1125,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   
>   	return 0;
>   
> -out_unlock:
> -	loop_global_unlock(lo, is_loop);
>   out_bdev:
>   	if (!(mode & BLK_OPEN_EXCL))
>   		bd_abort_claiming(bdev, loop_configure);
> @@ -1194,11 +1221,11 @@ static int loop_clr_fd(struct loop_device *lo)
>   	 * which loop_configure()/loop_change_fd() found via fget() was this
>   	 * loop device.
>   	 */
> -	err = loop_global_lock_killable(lo, true);
> +	err = loop_global_lock_killable(lo);
>   	if (err)
>   		return err;
>   	if (lo->lo_state != Lo_bound) {
> -		loop_global_unlock(lo, true);
> +		loop_global_unlock(lo);
>   		return -ENXIO;
>   	}
>   	/*
> @@ -1210,7 +1237,7 @@ static int loop_clr_fd(struct loop_device *lo)
>   	lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
>   	if (disk_openers(lo->lo_disk) == 1)
>   		WRITE_ONCE(lo->lo_state, Lo_rundown);
> -	loop_global_unlock(lo, true);
> +	loop_global_unlock(lo);
>   
>   	return 0;
>   }

Overall this change looks good to me. But I see, lo->lo_lock
is used to protect lo->lo_backing_file. so shall we annotate
the lo->lo_backing_file using __guarded_by(&lo_lock)?

Thanks,
--Nilay

Re: [PATCH v2 02/12] loop: Remove the "bool global" function argument

[Bart Van Assche]

On 8/3/26 6:05 AM, Nilay Shroff wrote:
> Overall this change looks good to me. But I see, lo->lo_lock
> is used to protect lo->lo_backing_file. so shall we annotate
> the lo->lo_backing_file using __guarded_by(&lo_lock)?

I don't think so. It seems to me that the strategy in the loop driver
for serializing accesses to lo->lo_backing_file is too complicated for
lock context annotations. My understanding is as follows:
* lo->lo_mutex serializes configuration and state changes on an
   individual loop device.
* loop_validate_mutex serializes concurrent loop_configure(),
   loop_change_fd(), and loop_clr_fd() calls across all loop devices to
   safely execute loop_validate_file() when loop devices are stacked
   or nested.
* The blk_mq_freeze_queue() call in __loop_change_fd() serializes I/O
   request processing and the code in __loop_change_fd() that is executed
   while the queue is frozen.

Thanks,

Bart.

Re: [PATCH v2 02/12] loop: Remove the "bool global" function argument

[Nilay Shroff]

On 8/3/26 11:11 PM, Bart Van Assche wrote:
> On 8/3/26 6:05 AM, Nilay Shroff wrote:
>> Overall this change looks good to me. But I see, lo->lo_lock
>> is used to protect lo->lo_backing_file. so shall we annotate
>> the lo->lo_backing_file using __guarded_by(&lo_lock)?
> 
> I don't think so. It seems to me that the strategy in the loop driver
> for serializing accesses to lo->lo_backing_file is too complicated for
> lock context annotations. My understanding is as follows:
> * lo->lo_mutex serializes configuration and state changes on an
>    individual loop device.
> * loop_validate_mutex serializes concurrent loop_configure(),
>    loop_change_fd(), and loop_clr_fd() calls across all loop devices to
>    safely execute loop_validate_file() when loop devices are stacked
>    or nested.
> * The blk_mq_freeze_queue() call in __loop_change_fd() serializes I/O
>    request processing and the code in __loop_change_fd() that is executed
>    while the queue is frozen.
> 

Yes, you're right about the locking being more complicated here. However,
I think there may be another issue: a race between the sysfs read in
loop_attr_backing_file_show() and replacement of the backing file in
__loop_change_fd().

__loop_change_fd() replaces lo->lo_backing_file through loop_assign_backing_file(),
but loop_assign_backing_file() does not take lo->lo_lock. Hence, the sysfs path
could read the old lo_backing_file pointer while __loop_change_fd() replaces it
and subsequently drops the old file reference with fput().

Since loop_attr_backing_file_show() does not take its own reference to the file,
this looks like it could result in a use-after-free.

Initially, I thought we could fix this by consistently protecting publication/removal
of lo->lo_backing_file with lo->lo_lock and taking a temporary file reference from
the sysfs path, e.g.:

static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
{
	struct file *file;
	...

	spin_lock_irq(&lo->lo_lock);
	file = lo->lo_backing_file;
	if (file)
		get_file(file);
	spin_unlock_irq(&lo->lo_lock);

	if (!file)
		return -ENOENT;

	...
	fput(file);
}
```

and similarly protect the assignment in loop_assign_backing_file().

static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
{
         spin_lock_irq(&lo->lo_lock);
         lo->lo_backing_file = file;
         spin_unlock_irq(&lo->lo_lock);
         ...
}

However, looking further through the code, I wonder whether we can instead get rid of
lo->lo_lock entirely and use lo->lo_mutex to serialize the sysfs access to lo->lo_backing_file.

__loop_change_fd() is already called with lo->lo_mutex held, so the update path would not
need any additional locking. We would only need to acquire lo->lo_mutex in
loop_attr_backing_file_show() while accessing lo->lo_backing_file.

Similarly, for the clear path in __loop_clr_fd(), it looks like the existing device teardown
serialization may already be sufficient, in which case the lo->lo_lock protection around
clearing lo->lo_backing_file could potentially be removed as well.

If that's correct, we could simplify the locking and remove lo->lo_lock altogether rather than
adding more users of it.

Thanks,
--Nilay

[PATCH v2 03/12] loop: Add lock context annotations

[Bart Van Assche]

Add lock context annotations that are compatible with Clang. Swap the
loop_ctl_mutex and loop_index_idr declarations such that __guarded_by()
can be used.

Cc: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/loop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index a71fe763c933..e2ac19de11f4 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -90,8 +90,8 @@ struct loop_cmd {
 #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
 #define LOOP_DEFAULT_HW_Q_DEPTH 128
 
-static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
+static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_validate_mutex);
 
 /**
@@ -106,6 +106,8 @@ static DEFINE_MUTEX(loop_validate_mutex);
  * loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
  */
 static int loop_global_lock_killable(struct loop_device *lo)
+	__cond_acquires(0, &loop_validate_mutex)
+	__cond_acquires(0, &lo->lo_mutex)
 {
 	int err;
 
@@ -124,6 +126,8 @@ static int loop_global_lock_killable(struct loop_device *lo)
  * @lo: struct loop_device
  */
 static void loop_global_unlock(struct loop_device *lo)
+	__releases(&lo->lo_mutex)
+	__releases(&loop_validate_mutex)
 {
 	mutex_unlock(&lo->lo_mutex);
 	mutex_unlock(&loop_validate_mutex);
@@ -2329,6 +2333,7 @@ static void __exit loop_exit(void)
 	 * module unloading is requested). If this is not a clean unloading,
 	 * we have no means to avoid kernel crash.
 	 */
+	__assume_ctx_lock(&loop_ctl_mutex);
 	idr_for_each_entry(&loop_index_idr, lo, id)
 		loop_remove(lo);
 

Re: [PATCH v2 03/12] loop: Add lock context annotations

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Add lock context annotations that are compatible with Clang. Swap the
> loop_ctl_mutex and loop_index_idr declarations such that __guarded_by()
> can be used.
> 
> Cc: Nilay Shroff <nilay@linux.ibm.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/loop.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index a71fe763c933..e2ac19de11f4 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -90,8 +90,8 @@ struct loop_cmd {
>   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
>   #define LOOP_DEFAULT_HW_Q_DEPTH 128
>   
> -static DEFINE_IDR(loop_index_idr);
>   static DEFINE_MUTEX(loop_ctl_mutex);
> +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);

Can we define helper macro here, which is more readable?

# define DEFINE_IDR_GUARDED(name, lock)\
         __guarded_by(&(lock)) DEFINE_IDR(name);

This is similar to what we have defined for guarded LIST_HEAD here:
https://lore.kernel.org/all/20260713115444.465704-2-nilay@linux.ibm.com/

Thanks,
--Nilay

Re: [PATCH v2 03/12] loop: Add lock context annotations

[Bart Van Assche]

On 8/3/26 6:11 AM, Nilay Shroff wrote:
> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>> Add lock context annotations that are compatible with Clang. Swap the
>> loop_ctl_mutex and loop_index_idr declarations such that __guarded_by()
>> can be used.
>>
>> Cc: Nilay Shroff <nilay@linux.ibm.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/block/loop.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index a71fe763c933..e2ac19de11f4 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -90,8 +90,8 @@ struct loop_cmd {
>>   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
>>   #define LOOP_DEFAULT_HW_Q_DEPTH 128
>> -static DEFINE_IDR(loop_index_idr);
>>   static DEFINE_MUTEX(loop_ctl_mutex);
>> +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);
> 
> Can we define helper macro here, which is more readable?
> 
> # define DEFINE_IDR_GUARDED(name, lock)\
>          __guarded_by(&(lock)) DEFINE_IDR(name);
> 
> This is similar to what we have defined for guarded LIST_HEAD here:
> https://lore.kernel.org/all/20260713115444.465704-2-nilay@linux.ibm.com/

Maybe I missed something but it's not clear to me how such a short macro
helps? It doesn't seem to improve code readability to me?

Thanks,

Bart.

Re: [PATCH v2 03/12] loop: Add lock context annotations

[Nilay Shroff]

On 8/3/26 11:13 PM, Bart Van Assche wrote:
> On 8/3/26 6:11 AM, Nilay Shroff wrote:
>> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>>> Add lock context annotations that are compatible with Clang. Swap the
>>> loop_ctl_mutex and loop_index_idr declarations such that __guarded_by()
>>> can be used.
>>>
>>> Cc: Nilay Shroff <nilay@linux.ibm.com>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/block/loop.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>>> index a71fe763c933..e2ac19de11f4 100644
>>> --- a/drivers/block/loop.c
>>> +++ b/drivers/block/loop.c
>>> @@ -90,8 +90,8 @@ struct loop_cmd {
>>>   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
>>>   #define LOOP_DEFAULT_HW_Q_DEPTH 128
>>> -static DEFINE_IDR(loop_index_idr);
>>>   static DEFINE_MUTEX(loop_ctl_mutex);
>>> +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);
>>
>> Can we define helper macro here, which is more readable?
>>
>> # define DEFINE_IDR_GUARDED(name, lock)\
>>          __guarded_by(&(lock)) DEFINE_IDR(name);
>>
>> This is similar to what we have defined for guarded LIST_HEAD here:
>> https://lore.kernel.org/all/20260713115444.465704-2-nilay@linux.ibm.com/
> 
> Maybe I missed something but it's not clear to me how such a short macro
> helps? It doesn't seem to improve code readability to me?
> 
I think the helper makes the relationship between the IDR and its protecting
lock a little more explicit, so this is mostly a matter of readability/style.

I'm fine either way. Let's see if others have a preference.

Thanks,
--Nilay

[PATCH v2 04/12] mtip32: Enable lock context analysis

[Bart Van Assche]

Let the compiler verify lock and unlock calls. Enable lock context analysis
in the Makefile.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/mtip32xx/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/mtip32xx/Makefile b/drivers/block/mtip32xx/Makefile
index bff32b5d3c19..233961fdb41b 100644
--- a/drivers/block/mtip32xx/Makefile
+++ b/drivers/block/mtip32xx/Makefile
@@ -3,4 +3,6 @@
 # Makefile for  Block device driver for Micron PCIe SSD
 #
 
+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_BLK_DEV_PCIESSD_MTIP32XX) += mtip32xx.o

Re: [PATCH v2 04/12] mtip32: Enable lock context analysis

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Let the compiler verify lock and unlock calls. Enable lock context analysis
> in the Makefile.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/mtip32xx/Makefile | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/block/mtip32xx/Makefile b/drivers/block/mtip32xx/Makefile
> index bff32b5d3c19..233961fdb41b 100644
> --- a/drivers/block/mtip32xx/Makefile
> +++ b/drivers/block/mtip32xx/Makefile
> @@ -3,4 +3,6 @@
>   # Makefile for  Block device driver for Micron PCIe SSD
>   #
>   
> +CONTEXT_ANALYSIS := y
> +
>   obj-$(CONFIG_BLK_DEV_PCIESSD_MTIP32XX) += mtip32xx.o

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

[PATCH v2 05/12] nbd: Enable lock context analysis

[Bart Van Assche]

Add __must_hold() annotations where these are missing. Document which mutex
protects nbd_index_idr.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/nbd.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 8f10762e90ef..751449d362b0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -49,8 +49,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/nbd.h>
 
-static DEFINE_IDR(nbd_index_idr);
 static DEFINE_MUTEX(nbd_index_mutex);
+static __guarded_by(&nbd_index_mutex) DEFINE_IDR(nbd_index_idr);
 static struct workqueue_struct *nbd_del_wq;
 static int nbd_total_devices = 0;
 
@@ -1506,6 +1506,7 @@ static void nbd_config_put(struct nbd_device *nbd)
 }
 
 static int nbd_start_device(struct nbd_device *nbd)
+	__must_hold(&nbd->config_lock)
 {
 	struct nbd_config *config = nbd->config;
 	int num_connections = config->num_connections;
@@ -1578,6 +1579,7 @@ static int nbd_start_device(struct nbd_device *nbd)
 }
 
 static int nbd_start_device_ioctl(struct nbd_device *nbd)
+	__must_hold(&nbd->config_lock)
 {
 	struct nbd_config *config = nbd->config;
 	int ret;
@@ -1629,6 +1631,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
 /* Must be called with config_lock held */
 static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		       unsigned int cmd, unsigned long arg)
+	__must_hold(&nbd->config_lock)
 {
 	struct nbd_config *config = nbd->config;
 	loff_t bytesize;
@@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
 	/* Also wait for nbd_dev_remove_work() completes */
 	destroy_workqueue(nbd_del_wq);
 
-	idr_destroy(&nbd_index_idr);
+	{
+		__assume_ctx_lock(&nbd_index_mutex);
+		idr_destroy(&nbd_index_idr);
+	}
+
 	unregister_blkdev(NBD_MAJOR, "nbd");
 }
 

Re: [PATCH v2 05/12] nbd: Enable lock context analysis

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Add __must_hold() annotations where these are missing. Document which mutex
> protects nbd_index_idr.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/nbd.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 8f10762e90ef..751449d362b0 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -49,8 +49,8 @@
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/nbd.h>
>   
> -static DEFINE_IDR(nbd_index_idr);
>   static DEFINE_MUTEX(nbd_index_mutex);
> +static __guarded_by(&nbd_index_mutex) DEFINE_IDR(nbd_index_idr);

Maybe we should replace this with DEFINE_IDR_GUARDED() as I mentioned
in previous email.

>   static struct workqueue_struct *nbd_del_wq;
>   static int nbd_total_devices = 0;
>   
> @@ -1506,6 +1506,7 @@ static void nbd_config_put(struct nbd_device *nbd)
>   }
>   
>   static int nbd_start_device(struct nbd_device *nbd)
> +	__must_hold(&nbd->config_lock)
>   {
>   	struct nbd_config *config = nbd->config;
>   	int num_connections = config->num_connections;
> @@ -1578,6 +1579,7 @@ static int nbd_start_device(struct nbd_device *nbd)
>   }
>   
>   static int nbd_start_device_ioctl(struct nbd_device *nbd)
> +	__must_hold(&nbd->config_lock)
>   {
>   	struct nbd_config *config = nbd->config;
>   	int ret;
> @@ -1629,6 +1631,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
>   /* Must be called with config_lock held */
>   static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>   		       unsigned int cmd, unsigned long arg)
> +	__must_hold(&nbd->config_lock)
>   {
>   	struct nbd_config *config = nbd->config;
>   	loff_t bytesize;
> @@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
>   	/* Also wait for nbd_dev_remove_work() completes */
>   	destroy_workqueue(nbd_del_wq);
>   
> -	idr_destroy(&nbd_index_idr);
> +	{
> +		__assume_ctx_lock(&nbd_index_mutex);
> +		idr_destroy(&nbd_index_idr);
> +	}
> +

Could we instead introduce a scoped __assume_ctx_lock() helper so we don't need the
explicit scope here? Alternatively, would annotating nbd_cleanup() with __context_unsafe
be a better fit?

Thanks,
--Nilay

Re: [PATCH v2 05/12] nbd: Enable lock context analysis

[Bart Van Assche]

On 8/3/26 6:26 AM, Nilay Shroff wrote:
> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>> @@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
>>       /* Also wait for nbd_dev_remove_work() completes */
>>       destroy_workqueue(nbd_del_wq);
>> -    idr_destroy(&nbd_index_idr);
>> +    {
>> +        __assume_ctx_lock(&nbd_index_mutex);
>> +        idr_destroy(&nbd_index_idr);
>> +    }
>> +
> 
> Could we instead introduce a scoped __assume_ctx_lock() helper so we
> don't need the explicit scope here? Alternatively, would annotating
> nbd_cleanup() with __context_unsafe be a better fit?
There is code in nbd_cleanup() that must be protected by a mutex so I
think that we shouldn't annotate nbd_cleanup() with __context_unsafe().

Introducing a scoped __assume_ctx_lock() might be overkill.

Thanks,

Bart.

Re: [PATCH v2 05/12] nbd: Enable lock context analysis

[Nilay Shroff]

On 8/3/26 11:33 PM, Bart Van Assche wrote:
> On 8/3/26 6:26 AM, Nilay Shroff wrote:
>> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>>> @@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
>>>       /* Also wait for nbd_dev_remove_work() completes */
>>>       destroy_workqueue(nbd_del_wq);
>>> -    idr_destroy(&nbd_index_idr);
>>> +    {
>>> +        __assume_ctx_lock(&nbd_index_mutex);
>>> +        idr_destroy(&nbd_index_idr);
>>> +    }
>>> +
>>
>> Could we instead introduce a scoped __assume_ctx_lock() helper so we
>> don't need the explicit scope here? Alternatively, would annotating
>> nbd_cleanup() with __context_unsafe be a better fit?
> There is code in nbd_cleanup() that must be protected by a mutex so I
> think that we shouldn't annotate nbd_cleanup() with __context_unsafe().
> 
That's fair enough.

> Introducing a scoped __assume_ctx_lock() might be overkill.
> 
Adding an explicit scope around __assume_ctx_lock() just to suppress a
false-positive context warning still looks a bit awkward to me. At this
point we know that the IDR is being destroyed and that there can no longer
be any concurrent users, so holding nbd_index_mutex is unnecessary.
If a scoped __assume_ctx_lock() helper is considered overkill, perhaps we
should have an IDR teardown helper for this case, e.g. idr_destroy_unlocked()
or similar, with the implementation annotated with __context_unsafe. That
would make the teardown semantics explicit without having individual callers
pretend that the protecting lock is held.

Thanks,
--Nilay

Re: [PATCH v2 05/12] nbd: Enable lock context analysis

[Marco Elver]

On Mon, 3 Aug 2026 at 20:03, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 8/3/26 6:26 AM, Nilay Shroff wrote:
> > On 7/31/26 1:28 AM, Bart Van Assche wrote:
> >> @@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
> >>       /* Also wait for nbd_dev_remove_work() completes */
> >>       destroy_workqueue(nbd_del_wq);
> >> -    idr_destroy(&nbd_index_idr);
> >> +    {
> >> +        __assume_ctx_lock(&nbd_index_mutex);
> >> +        idr_destroy(&nbd_index_idr);
> >> +    }
> >> +
> >
> > Could we instead introduce a scoped __assume_ctx_lock() helper so we
> > don't need the explicit scope here? Alternatively, would annotating
> > nbd_cleanup() with __context_unsafe be a better fit?
> There is code in nbd_cleanup() that must be protected by a mutex so I
> think that we shouldn't annotate nbd_cleanup() with __context_unsafe().

Just 'context_unsafe(idr_destroy(&nbd_index_idr)); /* comment */'
would be fine here I think. I've used that elsewhere for destructors.
It makes sure that the rest of the code is still checked and is as
simple as it gets.

> Introducing a scoped __assume_ctx_lock() might be overkill.

Agree. Note that __assume_ctx_lock() leaks outside the scope, so a
real scoped __assume_ctx_lock() would just do no-op __acquire() and
__release() using the cleanup.h infrastructure (similar to the init
guards I suppose).

Re: [PATCH v2 05/12] nbd: Enable lock context analysis

[Nilay Shroff]

On 8/4/26 2:55 PM, Marco Elver wrote:
> On Mon, 3 Aug 2026 at 20:03, Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> On 8/3/26 6:26 AM, Nilay Shroff wrote:
>>> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>>>> @@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
>>>>        /* Also wait for nbd_dev_remove_work() completes */
>>>>        destroy_workqueue(nbd_del_wq);
>>>> -    idr_destroy(&nbd_index_idr);
>>>> +    {
>>>> +        __assume_ctx_lock(&nbd_index_mutex);
>>>> +        idr_destroy(&nbd_index_idr);
>>>> +    }
>>>> +
>>>
>>> Could we instead introduce a scoped __assume_ctx_lock() helper so we
>>> don't need the explicit scope here? Alternatively, would annotating
>>> nbd_cleanup() with __context_unsafe be a better fit?
>> There is code in nbd_cleanup() that must be protected by a mutex so I
>> think that we shouldn't annotate nbd_cleanup() with __context_unsafe().
> 
> Just 'context_unsafe(idr_destroy(&nbd_index_idr)); /* comment */'
> would be fine here I think. I've used that elsewhere for destructors.
> It makes sure that the rest of the code is still checked and is as
> simple as it gets.
> 
>> Introducing a scoped __assume_ctx_lock() might be overkill.
> 
> Agree. Note that __assume_ctx_lock() leaks outside the scope, so a
> real scoped __assume_ctx_lock() would just do no-op __acquire() and
> __release() using the cleanup.h infrastructure (similar to the init
> guards I suppose).

Yes correct, I think if we ever implement scoped __assume_ctx_lock() then
it should just do no-op acquire capability and then release capability
like init guard.

Thanks,
--Nilay

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

[Bart Van Assche]

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 <bvanassche@acm.org>
---
 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)
 
 int null_init_zoned_dev(struct nullb_device *dev,
 			struct queue_limits *lim)
@@ -216,14 +219,14 @@ int null_report_zones(struct gendisk *disk, sector_t sector,
 		 * So use a local copy to avoid corruption of the device zone
 		 * array.
 		 */
-		null_lock_zone(dev, zone);
-		blkz.start = zone->start;
-		blkz.len = zone->len;
-		blkz.wp = zone->wp;
-		blkz.type = zone->type;
-		blkz.cond = zone->cond;
-		blkz.capacity = zone->capacity;
-		null_unlock_zone(dev, zone);
+		scoped_guard(null_zone, dev, zone) {
+			blkz.start = zone->start;
+			blkz.len = zone->len;
+			blkz.wp = zone->wp;
+			blkz.type = zone->type;
+			blkz.cond = zone->cond;
+			blkz.capacity = zone->capacity;
+		}
 
 		error = disk_report_zone(disk, &blkz, i, args);
 		if (error)
@@ -364,7 +367,7 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 		return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
 	}
 
-	null_lock_zone(dev, zone);
+	guard(null_zone)(dev, zone);
 
 	/*
 	 * Regular writes must be at the write pointer position. Zone append
@@ -376,19 +379,15 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 	 */
 	if (append) {
 		if (WARN_ON_ONCE(!dev->zone_append_max_sectors) ||
-		    zone->wp == NULL_ZONE_INVALID_WP) {
-			ret = BLK_STS_IOERR;
-			goto unlock_zone;
-		}
+		    zone->wp == NULL_ZONE_INVALID_WP)
+			return BLK_STS_IOERR;
 		sector = zone->wp;
 		blk_mq_rq_from_pdu(cmd)->__sector = sector;
 	}
 
 	if (sector != zone->wp ||
-	    zone->wp + nr_sectors > zone->start + zone->capacity) {
-		ret = BLK_STS_IOERR;
-		goto unlock_zone;
-	}
+	    zone->wp + nr_sectors > zone->start + zone->capacity)
+		return BLK_STS_IOERR;
 
 	if (zone->cond == BLK_ZONE_COND_CLOSED ||
 	    zone->cond == BLK_ZONE_COND_EMPTY) {
@@ -398,7 +397,7 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 			ret = null_check_zone_resources(dev, zone);
 			if (ret != BLK_STS_OK) {
 				spin_unlock(&dev->zone_res_lock);
-				goto unlock_zone;
+				return ret;
 			}
 			if (zone->cond == BLK_ZONE_COND_CLOSED) {
 				dev->nr_zones_closed--;
@@ -415,17 +414,15 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 
 	if (dev->badblocks.shift != -1) {
 		badblocks_ret = null_handle_badblocks(cmd, sector, &nr_sectors);
-		if (badblocks_ret != BLK_STS_OK && !nr_sectors) {
-			ret = badblocks_ret;
-			goto unlock_zone;
-		}
+		if (badblocks_ret != BLK_STS_OK && !nr_sectors)
+			return badblocks_ret;
 	}
 
 	if (dev->memory_backed) {
 		ret = null_handle_memory_backed(cmd, REQ_OP_WRITE, sector,
 						nr_sectors);
 		if (ret != BLK_STS_OK)
-			goto unlock_zone;
+			return ret;
 	}
 
 	zone->wp += nr_sectors;
@@ -441,12 +438,7 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 		zone->cond = BLK_ZONE_COND_FULL;
 	}
 
-	ret = badblocks_ret;
-
-unlock_zone:
-	null_unlock_zone(dev, zone);
-
-	return ret;
+	return badblocks_ret;
 }
 
 static blk_status_t null_open_zone(struct nullb_device *dev,
@@ -655,14 +647,14 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,
 	if (op == REQ_OP_ZONE_RESET_ALL) {
 		for (i = dev->zone_nr_conv; i < dev->nr_zones; i++) {
 			zone = &dev->zones[i];
-			null_lock_zone(dev, zone);
-			if (zone->cond != BLK_ZONE_COND_EMPTY &&
-			    zone->cond != BLK_ZONE_COND_READONLY &&
-			    zone->cond != BLK_ZONE_COND_OFFLINE) {
-				null_reset_zone(dev, zone);
-				trace_nullb_zone_op(cmd, i, zone->cond);
+			scoped_guard(null_zone, dev, zone) {
+				if (zone->cond != BLK_ZONE_COND_EMPTY &&
+				    zone->cond != BLK_ZONE_COND_READONLY &&
+				    zone->cond != BLK_ZONE_COND_OFFLINE) {
+					null_reset_zone(dev, zone);
+					trace_nullb_zone_op(cmd, i, zone->cond);
+				}
 			}
-			null_unlock_zone(dev, zone);
 		}
 		return BLK_STS_OK;
 	}
@@ -670,13 +662,11 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,
 	zone_no = null_zone_no(dev, sector);
 	zone = &dev->zones[zone_no];
 
-	null_lock_zone(dev, zone);
+	guard(null_zone)(dev, zone);
 
 	if (zone->cond == BLK_ZONE_COND_READONLY ||
-	    zone->cond == BLK_ZONE_COND_OFFLINE) {
-		ret = BLK_STS_IOERR;
-		goto unlock;
-	}
+	    zone->cond == BLK_ZONE_COND_OFFLINE)
+		return BLK_STS_IOERR;
 
 	switch (op) {
 	case REQ_OP_ZONE_RESET:
@@ -699,9 +689,6 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,
 	if (ret == BLK_STS_OK)
 		trace_nullb_zone_op(cmd, zone_no, zone->cond);
 
-unlock:
-	null_unlock_zone(dev, zone);
-
 	return ret;
 }
 
@@ -710,7 +697,6 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op,
 {
 	struct nullb_device *dev;
 	struct nullb_zone *zone;
-	blk_status_t sts;
 
 	switch (op) {
 	case REQ_OP_WRITE:
@@ -729,10 +715,8 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op,
 		if (zone->cond == BLK_ZONE_COND_OFFLINE)
 			return BLK_STS_IOERR;
 
-		null_lock_zone(dev, zone);
-		sts = null_process_cmd(cmd, op, sector, nr_sectors);
-		null_unlock_zone(dev, zone);
-		return sts;
+		scoped_guard(null_zone, dev, zone)
+			return null_process_cmd(cmd, op, sector, nr_sectors);
 	}
 }
 
@@ -746,7 +730,7 @@ static void null_set_zone_cond(struct nullb_device *dev,
 			 cond != BLK_ZONE_COND_OFFLINE))
 		return;
 
-	null_lock_zone(dev, zone);
+	guard(null_zone)(dev, zone);
 
 	/*
 	 * If the read-only condition is requested again to zones already in
@@ -767,8 +751,6 @@ static void null_set_zone_cond(struct nullb_device *dev,
 		zone->cond = cond;
 		zone->wp = NULL_ZONE_INVALID_WP;
 	}
-
-	null_unlock_zone(dev, zone);
 }
 
 /*

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

[Nilay Shroff]

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<bvanassche@acm.org>
> ---
>   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

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

[Bart Van Assche]

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.

> 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.

> 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.

Thanks,

Bart.

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

[Nilay Shroff]

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

[PATCH v2 07/12] rbd: Enable lock context analysis

[Bart Van Assche]

Add lock context annotations.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/rbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1f1c2810f6ee..f0f7a94e3e3e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4183,6 +4183,7 @@ static void rbd_acquire_lock(struct work_struct *work)
 }
 
 static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
 {
 	dout("%s rbd_dev %p\n", __func__, rbd_dev);
 	lockdep_assert_held_write(&rbd_dev->lock_rwsem);
@@ -4227,6 +4228,7 @@ static void __rbd_release_lock(struct rbd_device *rbd_dev)
  * lock_rwsem must be held for write
  */
 static void rbd_release_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
 {
 	if (!rbd_quiesce_lock(rbd_dev))
 		return;
@@ -4583,6 +4585,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)
  * lock_rwsem must be held for write
  */
 static void rbd_reacquire_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
 	char cookie[32];
@@ -6780,6 +6783,7 @@ static void rbd_dev_device_release(struct rbd_device *rbd_dev)
  * upon return.
  */
 static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
+	__releases(&rbd_dev->header_rwsem)
 {
 	int ret;
 
@@ -6881,6 +6885,7 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)
  * with @depth == 0.
  */
 static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
+	__context_unsafe(conditional locking on @depth)
 {
 	bool need_watch = !rbd_is_ro(rbd_dev);
 	int ret;
@@ -7134,6 +7139,9 @@ static ssize_t do_rbd_add(const char *buf, size_t count)
 	if (rc < 0)
 		goto err_out_rbd_dev;
 
+	/* Acquired by rbd_dev_image_probe(rbd_dev, 0) */
+	__acquire(&rbd_dev->header_rwsem);
+
 	if (rbd_dev->opts->alloc_size > rbd_dev->layout.object_size) {
 		rbd_warn(rbd_dev, "alloc_size adjusted to %u",
 			 rbd_dev->layout.object_size);

Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Nilay Shroff]

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Add lock context annotations.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/rbd.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 1f1c2810f6ee..f0f7a94e3e3e 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4183,6 +4183,7 @@ static void rbd_acquire_lock(struct work_struct *work)
>   }
>   
>   static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
> +	__must_hold(&rbd_dev->lock_rwsem)
>   {
>   	dout("%s rbd_dev %p\n", __func__, rbd_dev);
>   	lockdep_assert_held_write(&rbd_dev->lock_rwsem);
> @@ -4227,6 +4228,7 @@ static void __rbd_release_lock(struct rbd_device *rbd_dev)
>    * lock_rwsem must be held for write
>    */
>   static void rbd_release_lock(struct rbd_device *rbd_dev)
> +	__must_hold(&rbd_dev->lock_rwsem)
>   {
>   	if (!rbd_quiesce_lock(rbd_dev))
>   		return;
> @@ -4583,6 +4585,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)
>    * lock_rwsem must be held for write
>    */
>   static void rbd_reacquire_lock(struct rbd_device *rbd_dev)
> +	__must_hold(&rbd_dev->lock_rwsem)
>   {
>   	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>   	char cookie[32];
> @@ -6780,6 +6783,7 @@ static void rbd_dev_device_release(struct rbd_device *rbd_dev)
>    * upon return.
>    */
>   static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
> +	__releases(&rbd_dev->header_rwsem)
>   {
>   	int ret;
>   
> @@ -6881,6 +6885,7 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)
>    * with @depth == 0.
>    */
>   static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
> +	__context_unsafe(conditional locking on @depth)
>   {
>   	bool need_watch = !rbd_is_ro(rbd_dev);
>   	int ret;
> @@ -7134,6 +7139,9 @@ static ssize_t do_rbd_add(const char *buf, size_t count)
>   	if (rc < 0)
>   		goto err_out_rbd_dev;
>   
> +	/* Acquired by rbd_dev_image_probe(rbd_dev, 0) */
> +	__acquire(&rbd_dev->header_rwsem);
> +
>   	if (rbd_dev->opts->alloc_size > rbd_dev->layout.object_size) {
>   		rbd_warn(rbd_dev, "alloc_size adjusted to %u",
>   			 rbd_dev->layout.object_size);


In addition to the above annotations, it looks like several variables and structure members
are consistently protected by specific locks:

1. rbd_dev_list is guarded by rbd_dev_list_lock.
2. rbd_client_list is guarded by rbd_client_list_lock.
3. rbd_device::acquiring_list and rbd_device::running_list are guarded by
    rbd_device::lock_lists_lock.
4. rbd_device::object_map is guarded by rbd_device::object_map_lock.
5. rbd_device::watch_cookie, rbd_device::watch_state, and rbd_device::watch_handle
    are guarded by rbd_device::watch_mutex.
6. rbd_device::lock_state and rbd_device::lock_cookie are guarded by rbd_device::lock_rwsem.

So I think we should add the corresponding `__guarded_by(...)` annotations for these
fields as part of enabling lock context analysis for this driver.

Thanks,
--Nilay

Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Bart Van Assche]

On 8/3/26 7:03 AM, Nilay Shroff wrote:
> In addition to the above annotations, it looks like several variables 
> and structure members
> are consistently protected by specific locks:
> 
> 1. rbd_dev_list is guarded by rbd_dev_list_lock.
> 2. rbd_client_list is guarded by rbd_client_list_lock.
> 3. rbd_device::acquiring_list and rbd_device::running_list are guarded by
>     rbd_device::lock_lists_lock.
> 4. rbd_device::object_map is guarded by rbd_device::object_map_lock.
> 5. rbd_device::watch_cookie, rbd_device::watch_state, and 
> rbd_device::watch_handle
>     are guarded by rbd_device::watch_mutex.
> 6. rbd_device::lock_state and rbd_device::lock_cookie are guarded by 
> rbd_device::lock_rwsem.
> 
> So I think we should add the corresponding `__guarded_by(...)` 
> annotations for these
> fields as part of enabling lock context analysis for this driver.

Hi Nilay,

Thank you for having looked this up. I took a closer look at the rbd
driver and came up with the patch below. Some observations:
- Some code obtains pointers to acquiring_list, running_list and
   object_map before the corresponding synchronization objects are
   acquired. Hence, I have not tried to annotate these struct members
   with __guarded_by().
- The locking convention followed by rbd_img_object_requests() is not
   a good fit for what Clang supports.

Further feedback is welcome.

Thanks,

Bart.

These are the changes I came up with (relative to the above patch):

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index f0f7a94e3e3e..4c9f566b7a91 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -406,26 +406,26 @@ struct rbd_device {
  	struct mutex		watch_mutex;
  	enum rbd_watch_state	watch_state;
  	struct ceph_osd_linger_request *watch_handle;
-	u64			watch_cookie;
+	u64			watch_cookie __guarded_by(&watch_mutex);
  	struct delayed_work	watch_dwork;

  	struct rw_semaphore	lock_rwsem;
-	enum rbd_lock_state	lock_state;
-	char			lock_cookie[32];
+	enum rbd_lock_state	lock_state __guarded_by(&lock_rwsem);
+	char			lock_cookie[32] __guarded_by(&lock_rwsem);
  	struct rbd_client_id	owner_cid;
  	struct work_struct	acquired_lock_work;
  	struct work_struct	released_lock_work;
  	struct delayed_work	lock_dwork;
  	struct work_struct	unlock_work;
  	spinlock_t		lock_lists_lock;
-	struct list_head	acquiring_list;
-	struct list_head	running_list;
+	struct list_head	acquiring_list; /* __guarded_by(&lock) */
+	struct list_head	running_list; /* __guarded_by(&lock) */
  	struct completion	acquire_wait;
  	int			acquire_err;
  	struct completion	quiescing_wait;

  	spinlock_t		object_map_lock;
-	u8			*object_map;
+	u8			*object_map; /*__guarded_by(&object_map_lock)*/
  	u64			object_map_size;	/* in objects */
  	u64			object_map_flags;

@@ -464,11 +464,13 @@ enum rbd_dev_flags {

  static DEFINE_MUTEX(client_mutex);	/* Serialize client creation */

-static LIST_HEAD(rbd_dev_list);    /* devices */
+/* devices */
  static DEFINE_SPINLOCK(rbd_dev_list_lock);
+static __guarded_by(&rbd_dev_list_lock) LIST_HEAD(rbd_dev_list);

-static LIST_HEAD(rbd_client_list);		/* clients */
+/* clients */
  static DEFINE_SPINLOCK(rbd_client_list_lock);
+static __guarded_by(&rbd_client_list_lock) LIST_HEAD(rbd_client_list);

  /* Slab caches for frequently-allocated structures */

@@ -521,6 +523,7 @@ static bool rbd_is_snap(struct rbd_device *rbd_dev)
  }

  static bool __rbd_is_lock_owner(struct rbd_device *rbd_dev)
+	__must_hold_shared(&rbd_dev->lock_rwsem)
  {
  	lockdep_assert_held(&rbd_dev->lock_rwsem);

@@ -1645,6 +1648,7 @@ static void __rbd_object_map_index(struct 
rbd_device *rbd_dev, u64 objno,
  }

  static u8 __rbd_object_map_get(struct rbd_device *rbd_dev, u64 objno)
+	__must_hold(&rbd_dev->object_map_lock)
  {
  	u64 index;
  	u8 shift;
@@ -1655,6 +1659,7 @@ static u8 __rbd_object_map_get(struct rbd_device 
*rbd_dev, u64 objno)
  }

  static void __rbd_object_map_set(struct rbd_device *rbd_dev, u64 
objno, u8 val)
+	__must_hold(&rbd_dev->object_map_lock)
  {
  	u64 index;
  	u8 shift;
@@ -3431,6 +3436,7 @@ static bool need_exclusive_lock(struct 
rbd_img_request *img_req)
  }

  static bool rbd_lock_add_request(struct rbd_img_request *img_req)
+	__must_hold_shared(&img_req->rbd_dev->lock_rwsem)
  {
  	struct rbd_device *rbd_dev = img_req->rbd_dev;
  	bool locked;
@@ -3448,6 +3454,7 @@ static bool rbd_lock_add_request(struct 
rbd_img_request *img_req)
  }

  static void rbd_lock_del_request(struct rbd_img_request *img_req)
+	__must_hold_shared(&img_req->rbd_dev->lock_rwsem)
  {
  	struct rbd_device *rbd_dev = img_req->rbd_dev;
  	bool need_wakeup = false;
@@ -3472,6 +3479,8 @@ static int rbd_img_exclusive_lock(struct 
rbd_img_request *img_req)
  	if (!need_exclusive_lock(img_req))
  		return 1;

+	__assume_ctx_lock(&img_req->rbd_dev->lock_rwsem);
+
  	if (rbd_lock_add_request(img_req))
  		return 1;

@@ -3490,6 +3499,12 @@ static void rbd_img_object_requests(struct 
rbd_img_request *img_req)
  	struct rbd_obj_request *obj_req;

  	rbd_assert(!img_req->pending.result && !img_req->pending.num_pending);
+	/*
+	 * The caller either obtains an exclusive lock or obtains
+	 * rbd_dev->lock_rwsem. Since this cannot be represented accurately
+	 * using lock context annotations, add an __assume_ctx_lock() statement.
+	 */
+	__assume_ctx_lock(&rbd_dev->lock_rwsem);
  	rbd_assert(!need_exclusive_lock(img_req) ||
  		   __rbd_is_lock_owner(rbd_dev));

@@ -3533,6 +3548,7 @@ static void rbd_img_object_requests(struct 
rbd_img_request *img_req)
  }

  static bool rbd_img_advance(struct rbd_img_request *img_req, int *result)
+	__must_hold(&img_req->state_mutex)
  {
  	int ret;

@@ -3651,6 +3667,7 @@ static struct rbd_client_id rbd_get_cid(struct 
rbd_device *rbd_dev)
   */
  static void rbd_set_owner_cid(struct rbd_device *rbd_dev,
  			      const struct rbd_client_id *cid)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	dout("%s rbd_dev %p %llu-%llu -> %llu-%llu\n", __func__, rbd_dev,
  	     rbd_dev->owner_cid.gid, rbd_dev->owner_cid.handle,
@@ -3666,6 +3683,7 @@ static void format_lock_cookie(struct rbd_device 
*rbd_dev, char *buf)
  }

  static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct rbd_client_id cid = rbd_get_cid(rbd_dev);

@@ -3679,6 +3697,7 @@ static void __rbd_lock(struct rbd_device *rbd_dev, 
const char *cookie)
   * lock_rwsem must be held for write
   */
  static int rbd_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
  	char cookie[32];
@@ -3702,6 +3721,7 @@ static int rbd_lock(struct rbd_device *rbd_dev)
   * lock_rwsem must be held for write
   */
  static void rbd_unlock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
  	int ret;
@@ -3840,6 +3860,7 @@ static int rbd_request_lock(struct rbd_device 
*rbd_dev)
   * (i.e. "rbd map").
   */
  static void wake_lock_waiters(struct rbd_device *rbd_dev, int result)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct rbd_img_request *img_req;

@@ -3950,6 +3971,7 @@ static struct ceph_locker 
*get_lock_owner_info(struct rbd_device *rbd_dev)

  static int find_watcher(struct rbd_device *rbd_dev,
  			const struct ceph_locker *locker)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
  	struct ceph_watch_item *watchers;
@@ -3999,6 +4021,7 @@ static int find_watcher(struct rbd_device *rbd_dev,
   * lock_rwsem must be held for write
   */
  static int rbd_try_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	struct ceph_client *client = rbd_dev->rbd_client->client;
  	struct ceph_locker *locker, *refreshed_locker;
@@ -4217,6 +4240,7 @@ static void rbd_pre_release_action(struct 
rbd_device *rbd_dev)
  }

  static void __rbd_release_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	rbd_assert(list_empty(&rbd_dev->running_list));

@@ -4256,6 +4280,7 @@ static void rbd_release_lock_work(struct 
work_struct *work)
  }

  static void maybe_kick_acquire(struct rbd_device *rbd_dev)
+	__must_hold_shared(&rbd_dev->lock_rwsem)
  {
  	bool have_requests;

@@ -5302,6 +5327,7 @@ static void rbd_spec_free(struct kref *kref)
  }

  static void rbd_dev_free(struct rbd_device *rbd_dev)
+	__context_unsafe(cleanup function that does not need locking)
  {
  	WARN_ON(rbd_dev->watch_state != RBD_WATCH_STATE_UNREGISTERED);
  	WARN_ON(rbd_dev->lock_state != RBD_LOCK_STATE_UNLOCKED);
@@ -5338,6 +5364,7 @@ static void rbd_dev_release(struct device *dev)
  }

  static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
+	__context_unsafe(initialization function that does not need locking)
  {
  	struct rbd_device *rbd_dev;

Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Nilay Shroff]

On 8/4/26 1:16 AM, Bart Van Assche wrote:
> On 8/3/26 7:03 AM, Nilay Shroff wrote:
>> In addition to the above annotations, it looks like several variables and structure members
>> are consistently protected by specific locks:
>>
>> 1. rbd_dev_list is guarded by rbd_dev_list_lock.
>> 2. rbd_client_list is guarded by rbd_client_list_lock.
>> 3. rbd_device::acquiring_list and rbd_device::running_list are guarded by
>>     rbd_device::lock_lists_lock.
>> 4. rbd_device::object_map is guarded by rbd_device::object_map_lock.
>> 5. rbd_device::watch_cookie, rbd_device::watch_state, and rbd_device::watch_handle
>>     are guarded by rbd_device::watch_mutex.
>> 6. rbd_device::lock_state and rbd_device::lock_cookie are guarded by rbd_device::lock_rwsem.
>>
>> So I think we should add the corresponding `__guarded_by(...)` annotations for these
>> fields as part of enabling lock context analysis for this driver.
> 
> Hi Nilay,
> 
> Thank you for having looked this up. I took a closer look at the rbd
> driver and came up with the patch below. Some observations:
> - Some code obtains pointers to acquiring_list, running_list and
>    object_map before the corresponding synchronization objects are
>    acquired. Hence, I have not tried to annotate these struct members
>    with __guarded_by().
> - The locking convention followed by rbd_img_object_requests() is not
>    a good fit for what Clang supports.
> 
> Further feedback is welcome.
> 
> Thanks,
> 
> Bart.
> 
> These are the changes I came up with (relative to the above patch):
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index f0f7a94e3e3e..4c9f566b7a91 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -406,26 +406,26 @@ struct rbd_device {
>       struct mutex        watch_mutex;
>       enum rbd_watch_state    watch_state;
>       struct ceph_osd_linger_request *watch_handle;
> -    u64            watch_cookie;
> +    u64            watch_cookie __guarded_by(&watch_mutex);
>       struct delayed_work    watch_dwork;
> 
>       struct rw_semaphore    lock_rwsem;
> -    enum rbd_lock_state    lock_state;
> -    char            lock_cookie[32];
> +    enum rbd_lock_state    lock_state __guarded_by(&lock_rwsem);
> +    char            lock_cookie[32] __guarded_by(&lock_rwsem);
>       struct rbd_client_id    owner_cid;
>       struct work_struct    acquired_lock_work;
>       struct work_struct    released_lock_work;
>       struct delayed_work    lock_dwork;
>       struct work_struct    unlock_work;
>       spinlock_t        lock_lists_lock;
> -    struct list_head    acquiring_list;
> -    struct list_head    running_list;
> +    struct list_head    acquiring_list; /* __guarded_by(&lock) */
> +    struct list_head    running_list; /* __guarded_by(&lock) */
>       struct completion    acquire_wait;
>       int            acquire_err;
>       struct completion    quiescing_wait;
> 
>       spinlock_t        object_map_lock;
> -    u8            *object_map;
> +    u8            *object_map; /*__guarded_by(&object_map_lock)*/
>       u64            object_map_size;    /* in objects */
>       u64            object_map_flags;
> 
> @@ -464,11 +464,13 @@ enum rbd_dev_flags {
> 
>   static DEFINE_MUTEX(client_mutex);    /* Serialize client creation */
> 
> -static LIST_HEAD(rbd_dev_list);    /* devices */
> +/* devices */
>   static DEFINE_SPINLOCK(rbd_dev_list_lock);
> +static __guarded_by(&rbd_dev_list_lock) LIST_HEAD(rbd_dev_list);
> 
> -static LIST_HEAD(rbd_client_list);        /* clients */
> +/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
> +static __guarded_by(&rbd_client_list_lock) LIST_HEAD(rbd_client_list);
> 
>   /* Slab caches for frequently-allocated structures */
> 
> @@ -521,6 +523,7 @@ static bool rbd_is_snap(struct rbd_device *rbd_dev)
>   }
> 
>   static bool __rbd_is_lock_owner(struct rbd_device *rbd_dev)
> +    __must_hold_shared(&rbd_dev->lock_rwsem)
>   {
>       lockdep_assert_held(&rbd_dev->lock_rwsem);
> 
> @@ -1645,6 +1648,7 @@ static void __rbd_object_map_index(struct rbd_device *rbd_dev, u64 objno,
>   }
> 
>   static u8 __rbd_object_map_get(struct rbd_device *rbd_dev, u64 objno)
> +    __must_hold(&rbd_dev->object_map_lock)
>   {
>       u64 index;
>       u8 shift;
> @@ -1655,6 +1659,7 @@ static u8 __rbd_object_map_get(struct rbd_device *rbd_dev, u64 objno)
>   }
> 
>   static void __rbd_object_map_set(struct rbd_device *rbd_dev, u64 objno, u8 val)
> +    __must_hold(&rbd_dev->object_map_lock)
>   {
>       u64 index;
>       u8 shift;
> @@ -3431,6 +3436,7 @@ static bool need_exclusive_lock(struct rbd_img_request *img_req)
>   }
> 
>   static bool rbd_lock_add_request(struct rbd_img_request *img_req)
> +    __must_hold_shared(&img_req->rbd_dev->lock_rwsem)
>   {
>       struct rbd_device *rbd_dev = img_req->rbd_dev;
>       bool locked;
> @@ -3448,6 +3454,7 @@ static bool rbd_lock_add_request(struct rbd_img_request *img_req)
>   }
> 
>   static void rbd_lock_del_request(struct rbd_img_request *img_req)
> +    __must_hold_shared(&img_req->rbd_dev->lock_rwsem)
>   {
>       struct rbd_device *rbd_dev = img_req->rbd_dev;
>       bool need_wakeup = false;
> @@ -3472,6 +3479,8 @@ static int rbd_img_exclusive_lock(struct rbd_img_request *img_req)
>       if (!need_exclusive_lock(img_req))
>           return 1;
> 
> +    __assume_ctx_lock(&img_req->rbd_dev->lock_rwsem);
> +
>       if (rbd_lock_add_request(img_req))
>           return 1;
> 
> @@ -3490,6 +3499,12 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)
>       struct rbd_obj_request *obj_req;
> 
>       rbd_assert(!img_req->pending.result && !img_req->pending.num_pending);
> +    /*
> +     * The caller either obtains an exclusive lock or obtains
> +     * rbd_dev->lock_rwsem. Since this cannot be represented accurately
> +     * using lock context annotations, add an __assume_ctx_lock() statement.
> +     */
> +    __assume_ctx_lock(&rbd_dev->lock_rwsem);
>       rbd_assert(!need_exclusive_lock(img_req) ||
>              __rbd_is_lock_owner(rbd_dev));
> 
> @@ -3533,6 +3548,7 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)
>   }
> 
>   static bool rbd_img_advance(struct rbd_img_request *img_req, int *result)
> +    __must_hold(&img_req->state_mutex)
>   {
>       int ret;
> 
> @@ -3651,6 +3667,7 @@ static struct rbd_client_id rbd_get_cid(struct rbd_device *rbd_dev)
>    */
>   static void rbd_set_owner_cid(struct rbd_device *rbd_dev,
>                     const struct rbd_client_id *cid)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       dout("%s rbd_dev %p %llu-%llu -> %llu-%llu\n", __func__, rbd_dev,
>            rbd_dev->owner_cid.gid, rbd_dev->owner_cid.handle,
> @@ -3666,6 +3683,7 @@ static void format_lock_cookie(struct rbd_device *rbd_dev, char *buf)
>   }
> 
>   static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct rbd_client_id cid = rbd_get_cid(rbd_dev);
> 
> @@ -3679,6 +3697,7 @@ static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
>    * lock_rwsem must be held for write
>    */
>   static int rbd_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       char cookie[32];
> @@ -3702,6 +3721,7 @@ static int rbd_lock(struct rbd_device *rbd_dev)
>    * lock_rwsem must be held for write
>    */
>   static void rbd_unlock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       int ret;
> @@ -3840,6 +3860,7 @@ static int rbd_request_lock(struct rbd_device *rbd_dev)
>    * (i.e. "rbd map").
>    */
>   static void wake_lock_waiters(struct rbd_device *rbd_dev, int result)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct rbd_img_request *img_req;
> 
> @@ -3950,6 +3971,7 @@ static struct ceph_locker *get_lock_owner_info(struct rbd_device *rbd_dev)
> 
>   static int find_watcher(struct rbd_device *rbd_dev,
>               const struct ceph_locker *locker)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       struct ceph_watch_item *watchers;
> @@ -3999,6 +4021,7 @@ static int find_watcher(struct rbd_device *rbd_dev,
>    * lock_rwsem must be held for write
>    */
>   static int rbd_try_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_client *client = rbd_dev->rbd_client->client;
>       struct ceph_locker *locker, *refreshed_locker;
> @@ -4217,6 +4240,7 @@ static void rbd_pre_release_action(struct rbd_device *rbd_dev)
>   }
> 
>   static void __rbd_release_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       rbd_assert(list_empty(&rbd_dev->running_list));
> 
> @@ -4256,6 +4280,7 @@ static void rbd_release_lock_work(struct work_struct *work)
>   }
> 
>   static void maybe_kick_acquire(struct rbd_device *rbd_dev)
> +    __must_hold_shared(&rbd_dev->lock_rwsem)
>   {
>       bool have_requests;
> 
> @@ -5302,6 +5327,7 @@ static void rbd_spec_free(struct kref *kref)
>   }
> 
>   static void rbd_dev_free(struct rbd_device *rbd_dev)
> +    __context_unsafe(cleanup function that does not need locking)
>   {
>       WARN_ON(rbd_dev->watch_state != RBD_WATCH_STATE_UNREGISTERED);
>       WARN_ON(rbd_dev->lock_state != RBD_LOCK_STATE_UNLOCKED);
> @@ -5338,6 +5364,7 @@ static void rbd_dev_release(struct device *dev)
>   }
> 
>   static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
> +    __context_unsafe(initialization function that does not need locking)
>   {
>       struct rbd_device *rbd_dev;
> 
> 
The above change look good.

However, I still think acquiring_list, running_list, and object_map should
ideally be guarded by their respective locks. The fact that some code obtains
pointers to these objects before acquiring the corresponding lock makes the
locking pattern worth looking at more closely rather than simply leaving
these fields unannotated.

Could you perhaps Cc the RBD maintainers and mailing list on the next revision
and ask for their feedback on these locking patterns? They may be able to clarify
whether obtaining these pointers outside the lock is intentional and safe, or
whether there are existing races that have gone unnoticed.

Thanks,
--Nilay

Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Bart Van Assche]

On 8/4/26 3:48 AM, Nilay Shroff wrote:
> However, I still think acquiring_list, running_list, and object_map should
> ideally be guarded by their respective locks. The fact that some code 
> obtains
> pointers to these objects before acquiring the corresponding lock makes the
> locking pattern worth looking at more closely rather than simply leaving
> these fields unannotated.

Marco, to implement what Nilay is asking I need support for annotating
function arguments with __guarded_by(). This would not only be useful
for the rbd driver but also for other Linux kernel drivers, e.g. the
loop driver. If I try to annotate function arguments in the loop driver
with __guarded_by() the following error message appears:

drivers/block/loop.c:2002:30: warning: 'guarded_by' attribute only 
applies to non-static data members and global variables 
[-Wignored-attributes]
  2002 |                 struct list_head *cmd_list 
__guarded_by(&lo->lo_work_lock))

Your opinion about this is welcome.

Thank you,

Bart.

Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Marco Elver]

On Tue, 4 Aug 2026 at 20:24, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 8/4/26 3:48 AM, Nilay Shroff wrote:
> > However, I still think acquiring_list, running_list, and object_map should
> > ideally be guarded by their respective locks. The fact that some code
> > obtains
> > pointers to these objects before acquiring the corresponding lock makes the
> > locking pattern worth looking at more closely rather than simply leaving
> > these fields unannotated.
>
> Marco, to implement what Nilay is asking I need support for annotating
> function arguments with __guarded_by(). This would not only be useful
> for the rbd driver but also for other Linux kernel drivers, e.g. the
> loop driver. If I try to annotate function arguments in the loop driver
> with __guarded_by() the following error message appears:
>
> drivers/block/loop.c:2002:30: warning: 'guarded_by' attribute only
> applies to non-static data members and global variables
> [-Wignored-attributes]
>   2002 |                 struct list_head *cmd_list
> __guarded_by(&lo->lo_work_lock))
>
> Your opinion about this is welcome.

Can you show the diff you're trying?

It doesn't seem right to me. For one, the source of truth is scattered
around, and it's not all that obvious which arguments should then have
this attribute and which should not.

I wonder if there's a cleaner way to design this, where the source of
truth is centralized (hence the restriction on struct members or
globals) and not scattered around.

In general the analysis warns on __guarded_by fields passed by
pointer, but from what I can tell that wouldn't work here because the
locks should not be acquired before the function call that takes the
pointer.

[PATCH v2 08/12] ublk: Enable lock context analysis

[Bart Van Assche]

Add the lock context annotations that are required by Clang.

Cc: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/ublk_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2a22f9dc1f2f..b18b50efd094 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -373,11 +373,13 @@ static inline bool ublk_support_batch_io(const struct ublk_queue *ubq)
 }
 
 static inline void ublk_io_lock(struct ublk_io *io)
+	__acquires(&io->lock)
 {
 	spin_lock(&io->lock);
 }
 
 static inline void ublk_io_unlock(struct ublk_io *io)
+	__releases(&io->lock)
 {
 	spin_unlock(&io->lock);
 }
@@ -3262,6 +3264,7 @@ static int ublk_check_fetch_buf(const struct ublk_device *ub, __u64 buf_addr)
 
 static int __ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub,
 			struct ublk_io *io, u16 q_id)
+	__must_hold(&ub->mutex)
 {
 	/* UBLK_IO_FETCH_REQ is only allowed before dev is setup */
 	if (ublk_dev_ready(ub))
@@ -3689,6 +3692,7 @@ static void ublk_batch_revert_prep_cmd(struct ublk_batch_io_iter *iter,
 static int ublk_batch_prep_io(struct ublk_queue *ubq,
 			      const struct ublk_batch_io_data *data,
 			      const struct ublk_elem_header *elem)
+	__must_hold(&data->ub->mutex)
 {
 	struct ublk_io *io = &ubq->ios[elem->tag];
 	const struct ublk_batch_io *uc = &data->header;
@@ -4422,6 +4426,7 @@ static bool ublk_validate_user_pid(struct ublk_device *ub, pid_t ublksrv_pid)
  * UNPREP, daemon death), so re-check it under the mutex and wait again.
  */
 static int ublk_wait_dev_ready_and_lock(struct ublk_device *ub)
+	__cond_acquires(0, &ub->mutex)
 {
 	while (true) {
 		if (wait_var_event_interruptible(&ub->nr_queue_ready,
@@ -5323,6 +5328,7 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
  * already holds ub->mutex when calling del_gendisk() which freezes the queue.
 */
 static unsigned int ublk_lock_buf_tree(struct ublk_device *ub)
+	__acquires(&ub->mutex)
 {
 	unsigned int memflags = 0;
 
@@ -5334,6 +5340,7 @@ static unsigned int ublk_lock_buf_tree(struct ublk_device *ub)
 }
 
 static void ublk_unlock_buf_tree(struct ublk_device *ub, unsigned int memflags)
+	__releases(&ub->mutex)
 {
 	if (ub->ub_disk)
 		blk_mq_unfreeze_queue(ub->ub_disk->queue, memflags);

[PATCH v2 09/12] xen-blkback: Enable lock context analysis

[Bart Van Assche]

Let Clang verify lock and unlock calls. Enable lock context analysis in the
Makefile.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/xen-blkback/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/xen-blkback/Makefile b/drivers/block/xen-blkback/Makefile
index b0ea5ab5b9a1..864ef423226c 100644
--- a/drivers/block/xen-blkback/Makefile
+++ b/drivers/block/xen-blkback/Makefile
@@ -1,4 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
+
+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_XEN_BLKDEV_BACKEND) := xen-blkback.o
 
 xen-blkback-y	:= blkback.o xenbus.o

[PATCH v2 10/12] zram: Enable lock context analysis

[Bart Van Assche]

Add the lock context annotations that are required by Clang. Enable lock
context analysis in the Makefile.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/zram/Makefile | 2 ++
 drivers/block/zram/zcomp.c  | 3 ++-
 drivers/block/zram/zcomp.h  | 6 ++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index 0fdefd576691..a5663ab01653 100644
--- a/drivers/block/zram/Makefile
+++ b/drivers/block/zram/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
+CONTEXT_ANALYSIS := y
+
 zram-y	:=	zcomp.o zram_drv.o
 
 zram-$(CONFIG_ZRAM_BACKEND_LZO)		+= backend_lzorle.o backend_lzo.o
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 974c4691887e..155dbeacab1b 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -112,7 +112,8 @@ ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at)
 	return at;
 }
 
-struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
+struct zcomp_strm *__zcomp_stream_get(struct zcomp *comp)
+	__context_unsafe(uses raw_cpu_ptr())
 {
 	for (;;) {
 		struct zcomp_strm *zstrm = raw_cpu_ptr(comp->stream);
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 81a0f3f6ff48..afe0c1bf348d 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -85,8 +85,10 @@ const char *zcomp_lookup_backend_name(const char *comp);
 struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params);
 void zcomp_destroy(struct zcomp *comp);
 
-struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
-void zcomp_stream_put(struct zcomp_strm *zstrm);
+#define zcomp_stream_get(...) __acquire_ret(__zcomp_stream_get(__VA_ARGS__), &__ret->lock)
+struct zcomp_strm *__zcomp_stream_get(struct zcomp *comp);
+void zcomp_stream_put(struct zcomp_strm *zstrm)
+	__releases(&zstrm->lock);
 
 int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
 		   const void *src, unsigned int *dst_len);

[PATCH v2 11/12] rnbd: Enable lock context analysis

[Bart Van Assche]

Let Clang verify lock and unlock calls. Enable lock context analysis in the
Makefile.

Acked-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/rnbd/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/rnbd/Makefile b/drivers/block/rnbd/Makefile
index 208e5f865497..42c2cccdb53d 100644
--- a/drivers/block/rnbd/Makefile
+++ b/drivers/block/rnbd/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+CONTEXT_ANALYSIS := y
+
 ccflags-y := -I$(srctree)/drivers/infiniband/ulp/rtrs
 
 rnbd-client-y := rnbd-clt.o \

[PATCH v2 12/12] block: Enable lock context analysis for all block drivers

[Bart Van Assche]

Now that all locking functions in block drivers have been annotated,
enable lock context analysis for all block drivers at the top level of
drivers/block/.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 2d8096eb8cdf..e17f6381b798 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -6,6 +6,8 @@
 # Rewritten to use lists instead of if-statements.
 # 
 
+CONTEXT_ANALYSIS := y
+
 # needed for trace events
 ccflags-y				+= -I$(src)
 

Re: [PATCH v2 00/12] Enable lock context analysis in drivers/block/

[Nilay Shroff]

Hi Bart,

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Hi Jens,
> 
> This patch series enables lock context analysis in all block drivers in the
> drivers/block/ directory except DRBD. Please consider these patches for the
> next merge window.
> 

One general comment I have about this series is that there are several functions
across these drivers that use the lockdep_assert_*() APIs. I think these functions
should also be annotated with the corresponding Clang lock context annotations,
such as __must_hold(...) or __must_not_hold(...), depending on the lockdep assertion
being used. This would help keep the runtime lockdep assertions and the Clang lock
context annotations consistent.

Thanks,
--Nilay