Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Javier Gonzalez <javier@cnexlabs.com>
To:     Hua Su <suhua.tanke@gmail.com>
CC:     =?iso-8859-1?Q?Matias_Bj=F8rling?= <mb@lightnvm.io>,
        Jens Axboe <axboe@kernel.dk>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] lightnvm: pblk: adjust the position of the lock
Thread-Topic: [PATCH] lightnvm: pblk: adjust the position of the lock
Thread-Index: AQHUhfRSbN5Xcbn6ZEKx/nEy0nBdB6VjlbWA
Date:   Tue, 27 Nov 2018 12:57:54 +0000
Message-ID: <F75BC3C7-FACE-4C04-A9BE-5A3AA25D3466@cnexlabs.com>
References: <20181127015309.2374-1-suhua.tanke@gmail.com>
In-Reply-To: <20181127015309.2374-1-suhua.tanke@gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: cnexlabs.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: multipart/signed;
        boundary="Apple-Mail=_7E0671A7-9ABE-4CCD-BEF5-B1A4E13A5E97";
        protocol="application/pgp-signature";
        micalg=pgp-sha512
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 795aacde-5fa8-437f-5c94-08d65467f26e
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Nov 2018 12:57:54.4136
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: e40dfc2e-c6c1-463a-a598-38602b2c3cff
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR06MB4409
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

--Apple-Mail=_7E0671A7-9ABE-4CCD-BEF5-B1A4E13A5E97
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii


> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@gmail.com> wrote:
>=20
> Add lock protection for list operations.
> Signed-off-by: Hua Su <suhua.tanke@gmail.com>
> ---
> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>=20
> diff --git a/drivers/lightnvm/pblk-core.c =
b/drivers/lightnvm/pblk-core.c
> index 6944aac43b01..e490df217dac 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, =
struct pblk_line *line)
> 	list_del(&line->list);
>=20
> 	ret =3D pblk_line_prepare(pblk, line);
> -	if (ret) {
> -		list_add(&line->list, &l_mg->free_list);
> -		spin_unlock(&l_mg->free_lock);
> -		return ret;
> -	}
> -	spin_unlock(&l_mg->free_lock);
> +	if (ret)
> +		goto out;
>=20
> 	ret =3D pblk_line_alloc_bitmaps(pblk, line);
> 	if (ret)
> -		return ret;
> +		goto out;
>=20
> 	if (!pblk_line_init_bb(pblk, line, 0)) {
> 		list_add(&line->list, &l_mg->free_list);
> +		spin_unlock(&l_mg->free_lock);
> 		return -EINTR;
> 	}
> +	spin_unlock(&l_mg->free_lock);
>=20
> 	pblk_rl_free_lines_dec(&pblk->rl, line, true);
> 	return 0;
> +
> +out:
> +	list_add(&line->list, &l_mg->free_list);
> +	spin_unlock(&l_mg->free_lock);
> +	return ret;
> }
>=20
> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
> --
> 2.19.1

This path is only touched by the recovery path, which is single
threaded, so there is no race condition as is. Also, if recovery fails,
pblk will not create the instance at all. This said, it would be
good to protect the list_add on the pblk_line_init_bb() error path in
case this code is used for some other purpose in the future.

However, the implementation you propose has some issues due to the
extended critical zone, which covers pblk_line_alloc_bitmaps(). Here,
allocations are intentionally non-atomic (i.e., GFP_KERNEL).

You can instead do the following:

diff --git i/drivers/lightnvm/pblk-core.c w/drivers/lightnvm/pblk-core.c
index 615817bf97e3..02c4c1708c45 100644
--- i/drivers/lightnvm/pblk-core.c
+++ w/drivers/lightnvm/pblk-core.c
@@ -1301,15 +1301,22 @@ int pblk_line_recov_alloc(struct pblk *pblk, =
struct pblk_line *line)

        ret =3D pblk_line_alloc_bitmaps(pblk, line);
        if (ret)
-               return ret;
+               goto fail;

        if (!pblk_line_init_bb(pblk, line, 0)) {
-               list_add(&line->list, &l_mg->free_list);
-               return -EINTR;
+               ret =3D -EINTR;
+               goto fail;
        }

        pblk_rl_free_lines_dec(&pblk->rl, line, true);
        return 0;
+
+fail:
+       spin_lock(&l_mg->free_lock);
+       list_add(&line->list, &l_mg->free_list);
+       spin_unlock(&l_mg->free_lock);
+
+       return ret;
 }

 void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)

Javier


--Apple-Mail=_7E0671A7-9ABE-4CCD-BEF5-B1A4E13A5E97
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEE+ws7Qq+qZPG1bJoyIX4xUKFRnnQFAlv9P1EACgkQIX4xUKFR
nnRz7hAAxigfcUs5U3+0LuiUY0IczNsKJkhBCIeHP7QrBKjbu0+yxNVEt6DBW4ir
+xpZ5e/m82Ea96Qa4PQcdm/W1HaHEUoZSCf4bB42xmMlXikRq9LfHqC2csTUczD6
ypHR2okCSc/xSQGfxsuSAkGstC0qUNLfK0Afv1FTg4WjVgQrRN/ddFwitj39idDl
gMoGtI9m0nTA3j4WtqsVS8VPeQDC/M0zRAVJ2nG6uC3izmlTOW+/BHLkAtbbSN0v
Tem3hNyRnBFAQam0uIvjD5rwox+keXFtmlAC1ETduSlIYyqIbCSChSjrLbXOxwHH
3KOGNzcvn7gs4YR1kLoQBROUvI8zXgUjh8g1OmVCKz/9ds/B3SFzZMqTNfIfI7wx
UkGAgKft/6N94TixVecJVGcnYC0EmfPIazdqEhqFCy+7HoLDqw708dkQx8TbmaYP
ioJBqpSRNq9witcXu8ZbGIUM7xyQza/+SpzmgFL6iePPE6GM/f9Xjm5VZ2eOVzUr
pTd7+gXuBV2gb9ZZh/D5joRw+bOqE7jRAMZfrzeU8x40EH7+v4qtK8pDZ3zj82xA
b0AYVRZ15ZMW3ZlPdatVlUnmgzBsJRAyVaSdBC0tWNEpX31ynYfqgQr3+faKLxGm
kXccckYANTg6DjyOKRykJph0rFpzmKPpHpjM7SRzkYqqzXBxkR8=
=/USj
-----END PGP SIGNATURE-----

--Apple-Mail=_7E0671A7-9ABE-4CCD-BEF5-B1A4E13A5E97--