Re: [PATCH 5/8] loop: only take lo_mutex for the first reference in lo_open

[Jan Kara <jack@suse.cz>]

On Sat 05-02-22 09:28:33, Tetsuo Handa wrote:
> Ping?
> 
> I sent https://lkml.kernel.org/r/20220129071500.3566-1-penguin-kernel@I-love.SAKURA.ne.jp
> based on ideas from your series.
> 
> Since automated kernel tests are failing, can't we apply
> [PATCH 1/7] loop: revert "make autoclear operation asynchronous"
> for now if we can't come to a conclusion?

That's certainly a good start so feel free to add my Acked-by to the
revert. I agree it should be merged quickly as I think it is better to have
a theoretical deadlock in the code than userspace breakage hit in the wild.
I'll find some more time to look into this but it will take a while.

								Honza

> 
> On 2022/01/28 22:13, Tetsuo Handa wrote:
> > On 2022/01/28 22:00, Christoph Hellwig wrote:
> >> +	if (atomic_inc_return(&lo->lo_refcnt) > 1)
> >> +		return 0;
> >> +
> >>  	err = mutex_lock_killable(&lo->lo_mutex);
> >>  	if (err)
> > 
> > You did not notice my diff here...
> 
> You need to drop lo->lo_refcnt before return.
> 
> But my latest series no longer uses task work context and no longer
> holds lo->lo_mutex from lo_open()/lo_release().
> 
> > 
> >>  		return err;
> >> -	if (lo->lo_state == Lo_deleting)
> >> +	if (lo->lo_state == Lo_deleting) {
> >> +		atomic_dec(&lo->lo_refcnt);
> >>  		err = -ENXIO;
> >> -	else
> >> -		atomic_inc(&lo->lo_refcnt);
> >> +	}
> > 
> > Why do we need [PATCH 1/8] [PATCH 2/8] [PATCH 3/8] in this series?
> > Shouldn't we first make a clean revert, and keep the changes for
> > this release cycle as small as possible?
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR