Re: [PATCHv3 2/3] scsi: Do not rely on blk-mq for double completions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/21/18 6:12 AM, Christoph Hellwig wrote:
> On Mon, Nov 19, 2018 at 08:19:00AM -0700, Keith Busch wrote:
>> On Mon, Nov 19, 2018 at 12:58:15AM -0800, Christoph Hellwig wrote:
>>>> index 5d83a162d03b..c1d5e4e36125 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
>>>>  
>>>>  static void scsi_mq_done(struct scsi_cmnd *cmd)
>>>>  {
>>>> +	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
>>>> +		return;
>>>>  	trace_scsi_dispatch_cmd_done(cmd);
>>>> -	blk_mq_complete_request(cmd->request);
>>>> +	if (unlikely(!blk_mq_complete_request(cmd->request)))
>>>> +		clear_bit(__SCMD_COMPLETE, &cmd->flags);
>>>>  }
>>>
>>> This looks a little odd to me.  If we didn't complete the command
>>> someone else did.  Why would we clear the bit in this case?
>>
>> It's only to go along with the fake timeout. If we don't clear the bit,
>> then then scsi timeout handler will believe it has nothing to do because
>> scsi did its required part. The block layer just pretends the LLD didn't
>> do its part, so scsi has to play along too.
> 
> This just looks way to magic to me.  In other word - it needs a big fat
> comment explaining the situation.
> 
>>>> +#define __SCMD_COMPLETE		3
>>>> +#define SCMD_COMPLETE		(1 << __SCMD_COMPLETE)
>>>
>>> This mixing of atomic and non-atomic bitops looks rather dangerous
>>> to me.  Can you add a new atomic_flags just for the completed flag,
>>> and always use the bitops on it for now? I think we can eventually
>>> kill most of the existing flags except for SCMD_TAGGED over the
>>> next merge window or two and then move that over as well.
>>
>> The only concurrent access is completion + timeout, otherwise access is
>> single-threaded. I'm using the atomic operations only where it is
>> needed.
>>
>> We implicitly clear the SCMD_COMPLETED flag along with SCMD_TAGGED in
>> scsi_init_command() too, and I didn't want to add new overhead with
>> new atomics.
> 
> In general mixing access types on a single field (nevermind bit)
> is going to cause us problems further down the road sooner or later.
> 
> I'd be much happier with a separate field.

Keith, will you please respin with the separate field? Would be nice
to get this merged for 4.21.

-- 
Jens Axboe




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux