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

[Douglas Gilbert <dgilbert@interlog.com>]

On 2022-02-28 01:58, Damien Le Moal wrote:
> 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.

Grrr. If those functions are dummies then I think it would be
reasonable if their names had a word like "fake" or "dummy" in
them.

That being the case:
Acked-by: Douglas Gilbert <dgilbert@interlog.com>


Note: these patches should probably be against Martin's 5.18/scsi-staging
       tree as he has taken 5 or 6 of my scsi_debug patches in this cycle.

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