[2/2] nfs: Fix a missed page unlock after pg_doio()
diff mbox series

Message ID 63d8e14cfc732a5224bfd7ecb662f2da4d708ad0.1539792123.git.bcodding@redhat.com
State New
Headers show
Series
  • [1/2] nfs: remove redundant call to nfs_context_set_write_error()
Related show

Commit Message

Benjamin Coddington Oct. 17, 2018, 4:05 p.m. UTC
We must check pg_error and call error_cleanup after any call to pg_doio.
Currently, we are skipping the unlock of a page if we encounter an error in
nfs_pageio_complete() before handing off the work to the RPC layer.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/pagelist.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Comments

Trond Myklebust Oct. 17, 2018, 5:34 p.m. UTC | #1
On Wed, 2018-10-17 at 12:05 -0400, Benjamin Coddington wrote:
> We must check pg_error and call error_cleanup after any call to
> pg_doio.
> Currently, we are skipping the unlock of a page if we encounter an
> error in
> nfs_pageio_complete() before handing off the work to the RPC layer.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/pagelist.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index cd3bc41ab68d..54c2bfc45a57 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -1110,6 +1110,20 @@ static int
> nfs_pageio_add_request_mirror(struct nfs_pageio_descriptor *desc,
>  	return ret;
>  }
>  
> +static void nfs_pageio_error_cleanup(struct nfs_pageio_descriptor
> *desc)
> +{
> +	u32 midx;
> +	struct nfs_pgio_mirror *mirror;
> +
> +	if (!desc->pg_error)
> +		return;
> +
> +	for (midx = 0; midx < desc->pg_mirror_count; midx++) {
> +		mirror = &desc->pg_mirrors[midx];
> +		desc->pg_completion_ops->error_cleanup(&mirror-
> >pg_list);
> +	}
> +}
> +
>  int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
>  			   struct nfs_page *req)
>  {
> @@ -1160,20 +1174,7 @@ int nfs_pageio_add_request(struct
> nfs_pageio_descriptor *desc,
>  	return 1;
>  
>  out_failed:
> -	/*
> -	 * We might have failed before sending any reqs over wire.
> -	 * Clean up rest of the reqs in mirror pg_list.
> -	 */
> -	if (desc->pg_error) {
> -		struct nfs_pgio_mirror *mirror;
> -		void (*func)(struct list_head *);
> -
> -		func = desc->pg_completion_ops->error_cleanup;
> -		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
> -			mirror = &desc->pg_mirrors[midx];
> -			func(&mirror->pg_list);
> -		}
> -	}
> +	nfs_pageio_error_cleanup(desc);
>  	return 0;
>  }
>  
> @@ -1245,7 +1246,9 @@ void nfs_pageio_complete(struct
> nfs_pageio_descriptor *desc)
>  	for (midx = 0; midx < desc->pg_mirror_count; midx++)
>  		nfs_pageio_complete_mirror(desc, midx);
>  
> -	if (desc->pg_ops->pg_cleanup)
> +	if (desc->pg_error < 0)
> +		nfs_pageio_error_cleanup(desc);
> +	else if (desc->pg_ops->pg_cleanup)
>  		desc->pg_ops->pg_cleanup(desc);

Nice catch, but shouldn't we be calling both nfs_pageio_error_cleanup()
and pg_cleanup()? The former would appear to be cleaning up the page
stuff, while the latter is mainly cleaning up the layout.

>  	nfs_pageio_cleanup_mirroring(desc);
>  }

Should this perhaps be a stable fix?
Benjamin Coddington Oct. 17, 2018, 6:38 p.m. UTC | #2
On 17 Oct 2018, at 13:34, Trond Myklebust wrote:

> On Wed, 2018-10-17 at 12:05 -0400, Benjamin Coddington wrote:
>> @@ -1245,7 +1246,9 @@ void nfs_pageio_complete(struct
>> nfs_pageio_descriptor *desc)
>>  	for (midx = 0; midx < desc->pg_mirror_count; midx++)
>>  		nfs_pageio_complete_mirror(desc, midx);
>>
>> -	if (desc->pg_ops->pg_cleanup)
>> +	if (desc->pg_error < 0)
>> +		nfs_pageio_error_cleanup(desc);
>> +	else if (desc->pg_ops->pg_cleanup)
>>  		desc->pg_ops->pg_cleanup(desc);
>
> Nice catch, but shouldn't we be calling both nfs_pageio_error_cleanup()
> and pg_cleanup()? The former would appear to be cleaning up the page
> stuff, while the latter is mainly cleaning up the layout.

Ah, yes .. I got pg_cleanup mixed up with pg_completion_ops->completion.
Hmm, pg_cleanup seems unnecessary at the moment.  They all point to
pnfs_generic_pg_cleanup.

I'll send a v2 after testing with some pNFS..

>
>>  	nfs_pageio_cleanup_mirroring(desc);
>>  }
>
> Should this perhaps be a stable fix?

There's a lot of churn in there so I gave up looking for a Fixes: tag.  I'll
take another look and see if I can figure out how far back to go.

Thanks for the look,
Ben

Patch
diff mbox series

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index cd3bc41ab68d..54c2bfc45a57 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -1110,6 +1110,20 @@  static int nfs_pageio_add_request_mirror(struct nfs_pageio_descriptor *desc,
 	return ret;
 }
 
+static void nfs_pageio_error_cleanup(struct nfs_pageio_descriptor *desc)
+{
+	u32 midx;
+	struct nfs_pgio_mirror *mirror;
+
+	if (!desc->pg_error)
+		return;
+
+	for (midx = 0; midx < desc->pg_mirror_count; midx++) {
+		mirror = &desc->pg_mirrors[midx];
+		desc->pg_completion_ops->error_cleanup(&mirror->pg_list);
+	}
+}
+
 int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 			   struct nfs_page *req)
 {
@@ -1160,20 +1174,7 @@  int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 	return 1;
 
 out_failed:
-	/*
-	 * We might have failed before sending any reqs over wire.
-	 * Clean up rest of the reqs in mirror pg_list.
-	 */
-	if (desc->pg_error) {
-		struct nfs_pgio_mirror *mirror;
-		void (*func)(struct list_head *);
-
-		func = desc->pg_completion_ops->error_cleanup;
-		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
-			mirror = &desc->pg_mirrors[midx];
-			func(&mirror->pg_list);
-		}
-	}
+	nfs_pageio_error_cleanup(desc);
 	return 0;
 }
 
@@ -1245,7 +1246,9 @@  void nfs_pageio_complete(struct nfs_pageio_descriptor *desc)
 	for (midx = 0; midx < desc->pg_mirror_count; midx++)
 		nfs_pageio_complete_mirror(desc, midx);
 
-	if (desc->pg_ops->pg_cleanup)
+	if (desc->pg_error < 0)
+		nfs_pageio_error_cleanup(desc);
+	else if (desc->pg_ops->pg_cleanup)
 		desc->pg_ops->pg_cleanup(desc);
 	nfs_pageio_cleanup_mirroring(desc);
 }