Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200717205322.127694-1-pasha.tatashin@soleen.com>
 <20200717205322.127694-2-pasha.tatashin@soleen.com> <20200723180902.GV3673@sequoia>
 <CA+CK2bDC2ARTT2Q=c-p7586Xb8uedx-f6Rr7H9bYn-3U8x=d2Q@mail.gmail.com> <20200723183909.GW3673@sequoia>
In-Reply-To: <20200723183909.GW3673@sequoia>
From:   Pavel Tatashin <pasha.tatashin@soleen.com>
Date:   Thu, 23 Jul 2020 15:41:10 -0400
Message-ID: <CA+CK2bBv7UuCXQ-BDtrH=JiQRAJD9V885C-4tg+3eKG9viF=yA@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] loop: scale loop device by introducing per device lock
To:     Tyler Hicks <tyhicks@linux.microsoft.com>
Cc:     axboe@kernel.dk, linux-block@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> > > > -     atomic_inc(&lo->lo_refcnt);
> > > > -out:
> > > > +     err = mutex_lock_killable(&lo->lo_mutex);
> > > >       mutex_unlock(&loop_ctl_mutex);
> > >
> > > I don't see a possibility for deadlock but it bothers me a little that
> > > we're not unlocking in the reverse locking order here, as we do in
> > > loop_control_ioctl(). There should be no perf impact if we move the
> > > mutex_unlock(&loop_ctl_mutex) after mutex_unlock(&lo->lo_mutex).
> >
> > The lo_open() was one of the top functions that showed up in
> > contention profiling, and the only shared data that it updates is
> > lo_recnt which can be protected by lo_mutex. We must have
> > loop_ctl_mutex in order to get a valid lo pointer, otherwise we could
> > race with loop_control_ioctl(LOOP_CTL_REMOVE). Unlocking in a
> > different order is not an issue, as long as we always preserve the
> > locking order.
>
> It is probably a good idea to leave a comment about this in the
> lo_open() so that nobody comes along and tries to "correct" the
> unlocking order in the future and, as a result, introduces a perf
> regression.
>
Makes sense, I will add a comment about it.

Thank you,
Pasha