Re: [PATCH V2 1/3] block: add blk_mq_enter_no_io() and blk_mq_exit_no_io()

[Ming Lei <ming.lei@redhat.com>]

On Thu, Apr 03, 2025 at 07:44:27AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 03, 2025 at 10:52:08AM +0800, Ming Lei wrote:
> > Add blk_mq_enter_no_io() and blk_mq_exit_no_io() for preventing queue
> > from handling any FS or passthrough IO, meantime the queue is kept in
> > non-freeze state.
> 
> How does that differ from the actual freeze?  Please document that
> clearly in the commit log and in kerneldoc comments, and do an analysis
> of which callers should do the full freeze and which the limited I/O
> freeze.
> 
> Also the name is really unfortunate - no_io has a very clear connotation
> for memory allocations, so this should be using something else.
> 
> > Also add two variants of memsave version, since no fs_reclaim is allowed
> > in case of blk_mq_enter_no_io().
> 
> Please explain why.
> 
> 
> > index ae8494d88897..d117fa18b394 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -222,8 +222,7 @@ bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
> >  	bool unfreeze;
> >  
> >  	mutex_lock(&q->mq_freeze_lock);
> > -	if (force_atomic)
> > -		q->q_usage_counter.data->force_atomic = true;
> > +	q->q_usage_counter.data->force_atomic = force_atomic;
> >  	q->mq_freeze_depth--;
> >  	WARN_ON_ONCE(q->mq_freeze_depth < 0);
> >  	if (!q->mq_freeze_depth) {
> 
> This is a completely unrelated cleanup.
> 
> > +void blk_mq_enter_no_io(struct request_queue *q)
> > +{
> > +	blk_mq_freeze_queue_nomemsave(q);
> > +	q->no_io = true;
> > +	if (__blk_mq_unfreeze_queue(q, true))
> > +		blk_unfreeze_release_lock(q);
> 
> So this freezes the queue, sets a flag to not do I/O then unfreezes
> it.   So AFAIK it just is a freeze without the automatic recursion.
> 
> But maybe I'm missing something?

Yeah, looks lockdep modeling for blk_mq_enter_no_io() is wrong, and the
part in bio_enter_queue() is missed.

So this approach doesn't work.

Now the dependency between freeze lock and elevator lock looks one trouble,
such as [1], which is one real deadlock risk.

And there should be more.

[1] https://lore.kernel.org/linux-block/7755.1743228130@turing-police/#tReviewed-by


Thanks,
Ming