Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Christoph Hellwig]

On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
>  static inline void
>  sdeb_read_lock(rwlock_t *lock)
> +	__acquires_shared(lock)
> +	__context_unsafe(/*conditional locking*/)
>  {
> -	if (sdebug_no_rwlock)
> -		__acquire(lock);
> -	else
> +	if (!sdebug_no_rwlock)
>  		read_lock(lock);
>  }

Is there no way we can make the spare-style __acquire/__release work for
clang?  That would be this and similar patterns so much better and
safer than the blanket __context_unsafe.

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Marco Elver]

On Wed, 7 Oct 2026 at 15:14, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
> >  static inline void
> >  sdeb_read_lock(rwlock_t *lock)
> > +     __acquires_shared(lock)
> > +     __context_unsafe(/*conditional locking*/)
> >  {
> > -     if (sdebug_no_rwlock)
> > -             __acquire(lock);
> > -     else
> > +     if (!sdebug_no_rwlock)
> >               read_lock(lock);
> >  }
>
> Is there no way we can make the spare-style __acquire/__release work for
> clang?  That would be this and similar patterns so much better and
> safer than the blanket __context_unsafe.

__acquire/__release/etc. fake acquire/release functions do already
work with Clang Context Analysis. I spent quite a while making them
work because of patterns like above, and lots of macros which rely on
them. See definition of context_lock_struct() in
include/linux/compiler-context-analysis.h where they are implemented
(tldr; they are "overloaded" empty inline functions which exist per
context lock type).

Thanks,
-- Marco

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

On 10/7/26 6:14 AM, Christoph Hellwig wrote:
> On Tue, Oct 06, 2026 at 10:07:29PM -0700, Bart Van Assche wrote:
>>   static inline void
>>   sdeb_read_lock(rwlock_t *lock)
>> +	__acquires_shared(lock)
>> +	__context_unsafe(/*conditional locking*/)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		__acquire(lock);
>> -	else
>> +	if (!sdebug_no_rwlock)
>>   		read_lock(lock);
>>   }
> 
> Is there no way we can make the spare-style __acquire/__release work for
> clang?  That would be this and similar patterns so much better and
> safer than the blanket __context_unsafe.

Hi Christoph,

Do you like the alternative below better? If so then I will replace
patch 7/8 with the changes below.

Thanks,

Bart.


diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index b339f022af26..e8d8b693751e 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4052,42 +4052,47 @@ static inline struct sdeb_store_info 
*devip2sip(struct sdebug_dev_info *devip,

  static inline void
  sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
  {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
  		read_lock(lock);
+	else
+		__acquire_shared(lock);
  }

  static inline void
  sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
  {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
  		read_unlock(lock);
+	else
+		__release_shared(lock);
  }

  static inline void
  sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
  {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
  		write_lock(lock);
+	else
+		__acquire(lock);
  }

  static inline void
  sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
  {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
  		write_unlock(lock);
+	else
+		__release(lock);
  }

  static inline void
  sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4096,6 +4101,7 @@ sdeb_data_read_lock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4104,6 +4110,7 @@ sdeb_data_read_unlock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4112,6 +4119,7 @@ sdeb_data_write_lock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_data_lck)
  {
  	BUG_ON(!sip);

@@ -4120,6 +4128,7 @@ sdeb_data_write_unlock(struct sdeb_store_info *sip)

  static inline void
  sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4128,6 +4137,7 @@ sdeb_data_sector_read_lock(struct sdeb_store_info 
*sip)

  static inline void
  sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4136,6 +4146,7 @@ sdeb_data_sector_read_unlock(struct 
sdeb_store_info *sip)

  static inline void
  sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4144,6 +4155,7 @@ sdeb_data_sector_write_lock(struct sdeb_store_info 
*sip)

  static inline void
  sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_sector_lck)
  {
  	BUG_ON(!sip);

@@ -4165,102 +4177,122 @@ sdeb_data_sector_write_unlock(struct 
sdeb_store_info *sip)

  static inline void
  sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
+	__acquires(&sip->macc_data_lck)
  {
-	if (atomic)
+	if (atomic) {
  		sdeb_data_write_lock(sip);
-	else
+	} else {
  		sdeb_data_read_lock(sip);
+		__release_shared(&sip->macc_data_lck);
+		__acquire(&sip->macc_data_lck);
+	}
  }

  static inline void
  sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->macc_data_lck)
  {
-	if (atomic)
+	if (atomic) {
  		sdeb_data_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_data_lck);
+		__acquire_shared(&sip->macc_data_lck);
  		sdeb_data_read_unlock(sip);
+	}
  }

  /* Allow many reads but only 1x write per sector */
  static inline void
  sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
+	__acquires(&sip->macc_sector_lck)
  {
-	if (do_write)
+	if (do_write) {
  		sdeb_data_sector_write_lock(sip);
-	else
+	} else {
  		sdeb_data_sector_read_lock(sip);
+		__release_shared(&sip->macc_sector_lck);
+		__acquire(&sip->macc_sector_lck);
+	}
  }

  static inline void
  sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->macc_sector_lck)
  {
-	if (do_write)
+	if (do_write) {
  		sdeb_data_sector_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_sector_lck);
+		__acquire_shared(&sip->macc_sector_lck);
  		sdeb_data_sector_read_unlock(sip);
+	}
  }

  static inline void
  sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			read_lock(&sip->macc_meta_lck);
-		else
+		} else {
  			read_lock(&sdeb_fake_rw_lck);
+			__release_shared(&sdeb_fake_rw_lck);
+			__acquire_shared(&sip->macc_meta_lck);
+		}
+	} else {
+		__acquire_shared(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			read_unlock(&sip->macc_meta_lck);
-		else
+		} else {
+			__acquire_shared(&sdeb_fake_rw_lck);
  			read_unlock(&sdeb_fake_rw_lck);
+			__release_shared(&sip->macc_meta_lck);
+		}
+	} else {
+		__release_shared(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			write_lock(&sip->macc_meta_lck);
-		else
+		} else {
  			write_lock(&sdeb_fake_rw_lck);
+			__release(&sdeb_fake_rw_lck);
+			__acquire(&sip->macc_meta_lck);
+		}
+	} else {
+		__acquire(&sip->macc_meta_lck);
  	}
  }

  static inline void
  sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_meta_lck)
  {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
-		if (sip)
+	if (!sdebug_no_rwlock) {
+		if (sip) {
  			write_unlock(&sip->macc_meta_lck);
-		else
+		} else {
+			__acquire(&sdeb_fake_rw_lck);
  			write_unlock(&sdeb_fake_rw_lck);
+			__release(&sip->macc_meta_lck);
+		}
+	} else {
+		__release(&sip->macc_meta_lck);
  	}
  }

Re: [PATCH v4 7/8] scsi: scsi_debug: Improve lock context annotations

[Christoph Hellwig]

On Wed, Oct 07, 2026 at 01:10:27PM -0700, Bart Van Assche wrote:
> Do you like the alternative below better? If so then I will replace
> patch 7/8 with the changes below.

Much better, although I'd drop the pointless inversion of the
condition and keep the flow as in the old version:

static inline void
sdeb_read_lock(rwlock_t *lock)
	__acquires_shared(lock)
{
	if (sdebug_no_rwlock)
		__acquire_shared(lock);
	else
 		read_lock(lock);
}