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

[Bart Van Assche <bvanassche@acm.org>]

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;