Re: kernel BUG scsi_dh_alua sleeping from invalid context && kernel WARNING do not call blocking ops when !TASK_RUNNING

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

 



On Tue, 2023-01-17 at 16:29 -0800, Bart Van Assche wrote:
> On 1/17/23 14:03, Martin Wilck wrote:
> > On Tue, 2023-01-17 at 13:52 -0800, Bart Van Assche wrote:
> > > On 1/17/23 13:48, Martin Wilck wrote:
> > > > Yes, that was my suggestion. Just defer the scsi_device_put()
> > > > call
> > > > in
> > > > alua_rtpg_queue() in the case where the actual RTPG handler is
> > > > not
> > > > queued. I won't have time for that before next week though.
> > > 
> > > Hi Martin,
> > > 
> > > Do you agree that the call trace shared by Steffen is not
> > > sufficient
> > > to
> > > conclude that this change is necessary?
> > 
> > Hmm, I suppose I missed your point... to re-iterate my thinking:
> > 
> >   1 alua_queue_rtpg() must take a ref to the sdev before queueing
> > work,
> >     whether or not the caller already has one
> >   2 queue_delayed_work() can fail
> >   3 if queue_delayed_work() fails, alua_queue_rtpg() must drop the
> > ref
> >     it just took
> >   4 BUT (and this is what I guess I missed) this ref can't be the
> > last
> >     one dropped, because the caller of alua_rtpg_queue() must still
> > hold
> >     a reference. And scsi_device_put() only sleeps if the last ref
> > is
> >     dropped. Therefore the issue in Steffen's call stack should
> >     indeed be fixed just by removing the might_sleep(). If all
> > callers
> >     callers of alua_rtpg_queue() must hold an sdev reference (I
> > believe
> >     they do), we can indeed remove the might_sleep() entirely.
> > 
> > Is this correct reasoning, and what you meant previously? If yes, I
> > agree, and I apologize for not realizing it in the first place.
> > But I think this is subtle enough to deserve a comment in the code.
> 
> Yes, that's what I'm thinking.
> 
> How about the patch below?
> 
> Thanks,
> 
> Bart.
> 
> [PATCH] scsi: device_handler: alua: Remove a might_sleep() annotation
> 
> The might_sleep() annotation in alua_rtpg_queue() is not correct
> since the
> command completion code may call this function from atomic context.
> Calling alua_rtpg_queue() from atomic context in the command
> completion
> path is fine since request submitters must hold an sdev reference
> until
> command execution has completed. This patch fixes the following
> kernel
> warning:
> 
> BUG: sleeping function called from invalid context at
> drivers/scsi/device_handler/scsi_dh_alua.c:992
> Call Trace:
>   dump_stack_lvl+0xac/0x100
>   __might_resched+0x284/0x2c8
>   alua_rtpg_queue+0x3c/0x98 [scsi_dh_alua]
>   alua_check+0x122/0x250 [scsi_dh_alua]
>   alua_check_sense+0x172/0x228 [scsi_dh_alua]
>   scsi_check_sense+0x8a/0x2e0
>   scsi_decide_disposition+0x286/0x298
>   scsi_complete+0x6a/0x108
>   blk_complete_reqs+0x6e/0x88
>   __do_softirq+0x13e/0x6b8
>   __irq_exit_rcu+0x14a/0x170
>   irq_exit_rcu+0x22/0x50
>   do_ext_irq+0x10a/0x1d0
> 
> Reported-by: Steffen Maier <maier@xxxxxxxxxxxxx>
> Cc: Steffen Maier <maier@xxxxxxxxxxxxx>
> Cc: Martin Wilck <mwilck@xxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>


Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>

> ---
>   drivers/scsi/device_handler/scsi_dh_alua.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 55a5073248f8..362fa631f39b 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -987,6 +987,9 @@ static void alua_rtpg_work(struct work_struct
> *work)
>    *
>    * Returns true if and only if alua_rtpg_work() will be called
> asynchronously.
>    * That function is responsible for calling @qdata->fn().
> + *
> + * Context: may be called from atomic context (alua_check()) only if
> the caller
> + *     holds an sdev reference.
>    */
>   static bool alua_rtpg_queue(struct alua_port_group *pg,
>                             struct scsi_device *sdev,
> @@ -995,8 +998,6 @@ static bool alua_rtpg_queue(struct
> alua_port_group *pg,
>         int start_queue = 0;
>         unsigned long flags;
> 
> -       might_sleep();
> -
>         if (WARN_ON_ONCE(!pg) || scsi_device_get(sdev))
>                 return false;
> 
> 





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux