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.
