Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/02/28 3:39, Douglas Gilbert wrote:
> On 2022-02-25 03:45, Damien Le Moal wrote:
>> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(),
>> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to
>> many warnings about unexpected unlocks in the resp_xxx() functions.
>>
>> Modify the lock/unlock functions using the __acquire() and __release()
>> inline annotations for the sdebug_no_rwlock == true case to avoid these
>> warnings.
> 
> I'm confused. The idea with sdebug_no_rwlock was that the application
> may know that the protection afforded by the driver's rwlock is not
> needed because locking is performed at a higher level (e.g. in the
> user space). Hence there is no need to use a read-write lock (or a
> full lock) in this driver to protect a read (say) against a co-incident
> write to the same memory region. So this was a performance enhancement.
> 
> The proposed patch seems to be replacing a read-write lock with a full
> lock. That would be going in the opposite direction to what I intended.

Not at all. The __acquire() and __release() calls are not locking functions.
They are annotations for sparse so that we get a correct +/-1 counting of the
lock/unlock calls. So there is no functional change here and no overhead added
when compiling without C=1 since these macros disappear without sparse.

> 
> If this is the only solution, a better idea might be to drop the
> patch (in staging I think) that introduced the sdebug_no_rwlock option.
> 
> The sdebug_no_rwlock option has been pretty thoroughly tested (for over
> a year) with memory to memory transfers (together with sgl to sgl
> additions to lib/scatterlist.h). Haven't seen any unexplained crashes
> that I could trace to this lack of locking. OTOH I haven't measured
> any improvement of the copy speed either, that may be because my tests
> are approaching the copy bandwidth of the ram.
> 
> 
> Does sparse understand guard variables (e.g. like 'bool lock_taken')?
>  From what I've seen with sg3_utils Coverity doesn't, leading to many false
> reports.
> 
> Doug Gilbert
> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++--------------
>>   1 file changed, 44 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 0d25b30922ef..f4e97f2224b2 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
>>   static inline void
>>   sdeb_read_lock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		read_lock(&sip->macc_lck);
>> -	else
>> -		read_lock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__acquire(&sip->macc_lck);
>> +		else
>> +			__acquire(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			read_lock(&sip->macc_lck);
>> +		else
>> +			read_lock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_read_unlock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		read_unlock(&sip->macc_lck);
>> -	else
>> -		read_unlock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__release(&sip->macc_lck);
>> +		else
>> +			__release(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			read_unlock(&sip->macc_lck);
>> +		else
>> +			read_unlock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_write_lock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		write_lock(&sip->macc_lck);
>> -	else
>> -		write_lock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__acquire(&sip->macc_lck);
>> +		else
>> +			__acquire(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			write_lock(&sip->macc_lck);
>> +		else
>> +			write_lock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_write_unlock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		write_unlock(&sip->macc_lck);
>> -	else
>> -		write_unlock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__release(&sip->macc_lck);
>> +		else
>> +			__release(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			write_unlock(&sip->macc_lck);
>> +		else
>> +			write_unlock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> 


-- 
Damien Le Moal
Western Digital Research