[PATCH] NFSv4: Fix up mirror allocation

[PATCH] NFSv4: Fix up mirror allocation

[Trond Myklebust]

There are a number of callers of nfs_pageio_complete() that want to
continue using the nfs_pageio_descriptor without needing to call
nfs_pageio_init() again. Examples include nfs_pageio_resend() and
nfs_pageio_cond_complete().

The problem is that nfs_pageio_complete() also calls
nfs_pageio_cleanup_mirroring(), which frees up the array of mirrors.
This can lead to writeback errors, in the next call to
nfs_pageio_setup_mirroring().

Fix by simply moving the allocation of the mirrors to
nfs_pageio_setup_mirroring().

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196709
Reported-by: JianhongYin <yin-jianhong@163.com>
Cc: stable@vger.kernel.org # 4.0+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/pagelist.c | 73 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 548ebc7256ff..1c7e625824dc 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -685,9 +685,6 @@ void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
 		     int io_flags,
 		     gfp_t gfp_flags)
 {
-	struct nfs_pgio_mirror *new;
-	int i;
-
 	desc->pg_moreio = 0;
 	desc->pg_inode = inode;
 	desc->pg_ops = pg_ops;
@@ -703,21 +700,9 @@ void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
 	desc->pg_mirror_count = 1;
 	desc->pg_mirror_idx = 0;
 
-	if (pg_ops->pg_get_mirror_count) {
-		/* until we have a request, we don't have an lseg and no
-		 * idea how many mirrors there will be */
-		new = kcalloc(NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX,
-			      sizeof(struct nfs_pgio_mirror), gfp_flags);
-		desc->pg_mirrors_dynamic = new;
-		desc->pg_mirrors = new;
-
-		for (i = 0; i < NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX; i++)
-			nfs_pageio_mirror_init(&desc->pg_mirrors[i], bsize);
-	} else {
-		desc->pg_mirrors_dynamic = NULL;
-		desc->pg_mirrors = desc->pg_mirrors_static;
-		nfs_pageio_mirror_init(&desc->pg_mirrors[0], bsize);
-	}
+	desc->pg_mirrors_dynamic = NULL;
+	desc->pg_mirrors = desc->pg_mirrors_static;
+	nfs_pageio_mirror_init(&desc->pg_mirrors[0], bsize);
 }
 EXPORT_SYMBOL_GPL(nfs_pageio_init);
 
@@ -836,32 +821,52 @@ static int nfs_generic_pg_pgios(struct nfs_pageio_descriptor *desc)
 	return ret;
 }
 
+static struct nfs_pgio_mirror *
+nfs_pageio_alloc_mirrors(struct nfs_pageio_descriptor *desc,
+		unsigned int mirror_count)
+{
+	struct nfs_pgio_mirror *ret;
+	unsigned int i;
+
+	kfree(desc->pg_mirrors_dynamic);
+	desc->pg_mirrors_dynamic = NULL;
+	if (mirror_count == 1)
+		return desc->pg_mirrors_static;
+	ret = kmalloc_array(mirror_count, sizeof(*ret), GFP_NOFS);
+	if (ret != NULL) {
+		for (i = 0; i < mirror_count; i++)
+			nfs_pageio_mirror_init(&ret[i], desc->pg_bsize);
+		desc->pg_mirrors_dynamic = ret;
+	}
+	return ret;
+}
+
 /*
  * nfs_pageio_setup_mirroring - determine if mirroring is to be used
  *				by calling the pg_get_mirror_count op
  */
-static int nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
+static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
 				       struct nfs_page *req)
 {
-	int mirror_count = 1;
+	unsigned int mirror_count = 1;
 
-	if (!pgio->pg_ops->pg_get_mirror_count)
-		return 0;
-
-	mirror_count = pgio->pg_ops->pg_get_mirror_count(pgio, req);
-
-	if (pgio->pg_error < 0)
-		return pgio->pg_error;
-
-	if (!mirror_count || mirror_count > NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX)
-		return -EINVAL;
+	if (pgio->pg_ops->pg_get_mirror_count)
+		mirror_count = pgio->pg_ops->pg_get_mirror_count(pgio, req);
+	if (mirror_count == pgio->pg_mirror_count || pgio->pg_error < 0)
+		return;
 
-	if (WARN_ON_ONCE(!pgio->pg_mirrors_dynamic))
-		return -EINVAL;
+	if (!mirror_count || mirror_count > NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX) {
+		pgio->pg_error = -EINVAL;
+		return;
+	}
 
+	pgio->pg_mirrors = nfs_pageio_alloc_mirrors(pgio, mirror_count);
+	if (pgio->pg_mirrors == NULL) {
+		pgio->pg_error = -ENOMEM;
+		pgio->pg_mirrors = pgio->pg_mirrors_static;
+		mirror_count = 1;
+	}
 	pgio->pg_mirror_count = mirror_count;
-
-	return 0;
 }
 
 /*
-- 
2.13.5

Re: [PATCH] NFSv4: Fix up mirror allocation

[Weston Andros Adamson]

Looks good to me.

Reviewed-by: Weston Andros Adamson <dros@primarydata.com>

-dros

> On Aug 20, 2017, at 10:32 AM, Trond Myklebust =
<trond.myklebust@primarydata.com> wrote:
>=20
> There are a number of callers of nfs_pageio_complete() that want to
> continue using the nfs_pageio_descriptor without needing to call
> nfs_pageio_init() again. Examples include nfs_pageio_resend() and
> nfs_pageio_cond_complete().
>=20
> The problem is that nfs_pageio_complete() also calls
> nfs_pageio_cleanup_mirroring(), which frees up the array of mirrors.
> This can lead to writeback errors, in the next call to
> nfs_pageio_setup_mirroring().
>=20
> Fix by simply moving the allocation of the mirrors to
> nfs_pageio_setup_mirroring().
>=20
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=3D196709
> Reported-by: JianhongYin <yin-jianhong@163.com>
> Cc: stable@vger.kernel.org # 4.0+
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> fs/nfs/pagelist.c | 73 =
+++++++++++++++++++++++++++++--------------------------
> 1 file changed, 39 insertions(+), 34 deletions(-)
>=20
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 548ebc7256ff..1c7e625824dc 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -685,9 +685,6 @@ void nfs_pageio_init(struct nfs_pageio_descriptor =
*desc,
> 		     int io_flags,
> 		     gfp_t gfp_flags)
> {
> -	struct nfs_pgio_mirror *new;
> -	int i;
> -
> 	desc->pg_moreio =3D 0;
> 	desc->pg_inode =3D inode;
> 	desc->pg_ops =3D pg_ops;
> @@ -703,21 +700,9 @@ void nfs_pageio_init(struct nfs_pageio_descriptor =
*desc,
> 	desc->pg_mirror_count =3D 1;
> 	desc->pg_mirror_idx =3D 0;
>=20
> -	if (pg_ops->pg_get_mirror_count) {
> -		/* until we have a request, we don't have an lseg and no
> -		 * idea how many mirrors there will be */
> -		new =3D kcalloc(NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX,
> -			      sizeof(struct nfs_pgio_mirror), =
gfp_flags);
> -		desc->pg_mirrors_dynamic =3D new;
> -		desc->pg_mirrors =3D new;
> -
> -		for (i =3D 0; i < NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX; i++)
> -			nfs_pageio_mirror_init(&desc->pg_mirrors[i], =
bsize);
> -	} else {
> -		desc->pg_mirrors_dynamic =3D NULL;
> -		desc->pg_mirrors =3D desc->pg_mirrors_static;
> -		nfs_pageio_mirror_init(&desc->pg_mirrors[0], bsize);
> -	}
> +	desc->pg_mirrors_dynamic =3D NULL;
> +	desc->pg_mirrors =3D desc->pg_mirrors_static;
> +	nfs_pageio_mirror_init(&desc->pg_mirrors[0], bsize);
> }
> EXPORT_SYMBOL_GPL(nfs_pageio_init);
>=20
> @@ -836,32 +821,52 @@ static int nfs_generic_pg_pgios(struct =
nfs_pageio_descriptor *desc)
> 	return ret;
> }
>=20
> +static struct nfs_pgio_mirror *
> +nfs_pageio_alloc_mirrors(struct nfs_pageio_descriptor *desc,
> +		unsigned int mirror_count)
> +{
> +	struct nfs_pgio_mirror *ret;
> +	unsigned int i;
> +
> +	kfree(desc->pg_mirrors_dynamic);
> +	desc->pg_mirrors_dynamic =3D NULL;
> +	if (mirror_count =3D=3D 1)
> +		return desc->pg_mirrors_static;
> +	ret =3D kmalloc_array(mirror_count, sizeof(*ret), GFP_NOFS);
> +	if (ret !=3D NULL) {
> +		for (i =3D 0; i < mirror_count; i++)
> +			nfs_pageio_mirror_init(&ret[i], desc->pg_bsize);
> +		desc->pg_mirrors_dynamic =3D ret;
> +	}
> +	return ret;
> +}
> +
> /*
>  * nfs_pageio_setup_mirroring - determine if mirroring is to be used
>  *				by calling the pg_get_mirror_count op
>  */
> -static int nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor =
*pgio,
> +static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor =
*pgio,
> 				       struct nfs_page *req)
> {
> -	int mirror_count =3D 1;
> +	unsigned int mirror_count =3D 1;
>=20
> -	if (!pgio->pg_ops->pg_get_mirror_count)
> -		return 0;
> -
> -	mirror_count =3D pgio->pg_ops->pg_get_mirror_count(pgio, req);
> -
> -	if (pgio->pg_error < 0)
> -		return pgio->pg_error;
> -
> -	if (!mirror_count || mirror_count > =
NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX)
> -		return -EINVAL;
> +	if (pgio->pg_ops->pg_get_mirror_count)
> +		mirror_count =3D pgio->pg_ops->pg_get_mirror_count(pgio, =
req);
> +	if (mirror_count =3D=3D pgio->pg_mirror_count || pgio->pg_error =
< 0)
> +		return;
>=20
> -	if (WARN_ON_ONCE(!pgio->pg_mirrors_dynamic))
> -		return -EINVAL;
> +	if (!mirror_count || mirror_count > =
NFS_PAGEIO_DESCRIPTOR_MIRROR_MAX) {
> +		pgio->pg_error =3D -EINVAL;
> +		return;
> +	}
>=20
> +	pgio->pg_mirrors =3D nfs_pageio_alloc_mirrors(pgio, =
mirror_count);
> +	if (pgio->pg_mirrors =3D=3D NULL) {
> +		pgio->pg_error =3D -ENOMEM;
> +		pgio->pg_mirrors =3D pgio->pg_mirrors_static;
> +		mirror_count =3D 1;
> +	}
> 	pgio->pg_mirror_count =3D mirror_count;
> -
> -	return 0;
> }
>=20
> /*
> --=20
> 2.13.5
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html