From: Trond Myklebust <[email protected]>
nfs_direct_write_scan_commit_list() will lock the request and bump
the reference count, but we also need to account for the reference
that was taken when we initially added the request to the commit list.
Fixes: fb5f7f20cdb9 ("NFS: commit errors should be fatal")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/direct.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 51ab4627c4d6..8074304fd5b4 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -646,6 +646,7 @@ static void nfs_direct_write_clear_reqs(struct nfs_direct_req *dreq)
while (!list_empty(&reqs)) {
req = nfs_list_entry(reqs.next);
nfs_list_remove_request(req);
+ nfs_release_request(req);
nfs_unlock_and_release_request(req);
}
}
--
2.25.1
From: Trond Myklebust <[email protected]>
If we just set the mirror count to 1 without first clearing out
the mirrors, we can leak queued up requests.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/pagelist.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 99eb839c5778..1fd4862217e0 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -900,15 +900,6 @@ static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
pgio->pg_mirror_count = mirror_count;
}
-/*
- * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
- */
-void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
-{
- pgio->pg_mirror_count = 1;
- pgio->pg_mirror_idx = 0;
-}
-
static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
{
pgio->pg_mirror_count = 1;
@@ -1334,6 +1325,14 @@ void nfs_pageio_cond_complete(struct nfs_pageio_descriptor *desc, pgoff_t index)
}
}
+/*
+ * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
+ */
+void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
+{
+ nfs_pageio_complete(pgio);
+}
+
int __init nfs_init_nfspagecache(void)
{
nfs_page_cachep = kmem_cache_create("nfs_page",
--
2.25.1
From: Trond Myklebust <[email protected]>
We need to trust that desc->pg_mirror_idx is set correctly, whether
or not mirroring is enabled.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/internal.h | 6 ------
fs/nfs/pagelist.c | 7 ++-----
2 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 78f317fac940..1f32a9fbfdaf 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -274,12 +274,6 @@ void nfs_free_request(struct nfs_page *req);
struct nfs_pgio_mirror *
nfs_pgio_current_mirror(struct nfs_pageio_descriptor *desc);
-static inline bool nfs_pgio_has_mirroring(struct nfs_pageio_descriptor *desc)
-{
- WARN_ON_ONCE(desc->pg_mirror_count < 1);
- return desc->pg_mirror_count > 1;
-}
-
static inline bool nfs_match_open_context(const struct nfs_open_context *ctx1,
const struct nfs_open_context *ctx2)
{
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 1fd4862217e0..f535a92403bf 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -33,9 +33,7 @@ static const struct rpc_call_ops nfs_pgio_common_ops;
struct nfs_pgio_mirror *
nfs_pgio_current_mirror(struct nfs_pageio_descriptor *desc)
{
- return nfs_pgio_has_mirroring(desc) ?
- &desc->pg_mirrors[desc->pg_mirror_idx] :
- &desc->pg_mirrors[0];
+ return &desc->pg_mirrors[desc->pg_mirror_idx];
}
EXPORT_SYMBOL_GPL(nfs_pgio_current_mirror);
@@ -1231,8 +1229,7 @@ static void nfs_pageio_complete_mirror(struct nfs_pageio_descriptor *desc,
struct nfs_pgio_mirror *mirror = &desc->pg_mirrors[mirror_idx];
u32 restore_idx = desc->pg_mirror_idx;
- if (nfs_pgio_has_mirroring(desc))
- desc->pg_mirror_idx = mirror_idx;
+ desc->pg_mirror_idx = mirror_idx;
for (;;) {
nfs_pageio_doio(desc);
if (desc->pg_error < 0 || !mirror->pg_recoalesce)
--
2.25.1
Hi Trond,
On 4/1/20 2:56 PM, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> If we just set the mirror count to 1 without first clearing out
> the mirrors, we can leak queued up requests.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/pagelist.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 99eb839c5778..1fd4862217e0 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -900,15 +900,6 @@ static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
> pgio->pg_mirror_count = mirror_count;
> }
>
> -/*
> - * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
> - */
> -void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> -{
> - pgio->pg_mirror_count = 1;
> - pgio->pg_mirror_idx = 0;
> -}
> -
> static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
> {
> pgio->pg_mirror_count = 1;
> @@ -1334,6 +1325,14 @@ void nfs_pageio_cond_complete(struct nfs_pageio_descriptor *desc, pgoff_t index)
> }
> }
>
> +/*
> + * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
> + */
> +void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> +{
> + nfs_pageio_complete(pgio);
> +}
> +
Would it make sense to replace calls to nfs_pageio_stop_mirroring() with nfs_pageio_complete() instead?
Anna
> int __init nfs_init_nfspagecache(void)
> {
> nfs_page_cachep = kmem_cache_create("nfs_page",
On Thu, 2020-04-02 at 12:14 -0400, Anna Schumaker wrote:
> Hi Trond,
>
> On 4/1/20 2:56 PM, [email protected] wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > If we just set the mirror count to 1 without first clearing out
> > the mirrors, we can leak queued up requests.
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > fs/nfs/pagelist.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index 99eb839c5778..1fd4862217e0 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
> > @@ -900,15 +900,6 @@ static void nfs_pageio_setup_mirroring(struct
> > nfs_pageio_descriptor *pgio,
> > pgio->pg_mirror_count = mirror_count;
> > }
> >
> > -/*
> > - * nfs_pageio_stop_mirroring - stop using mirroring (set mirror
> > count to 1)
> > - */
> > -void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> > -{
> > - pgio->pg_mirror_count = 1;
> > - pgio->pg_mirror_idx = 0;
> > -}
> > -
> > static void nfs_pageio_cleanup_mirroring(struct
> > nfs_pageio_descriptor *pgio)
> > {
> > pgio->pg_mirror_count = 1;
> > @@ -1334,6 +1325,14 @@ void nfs_pageio_cond_complete(struct
> > nfs_pageio_descriptor *desc, pgoff_t index)
> > }
> > }
> >
> > +/*
> > + * nfs_pageio_stop_mirroring - stop using mirroring (set mirror
> > count to 1)
> > + */
> > +void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> > +{
> > + nfs_pageio_complete(pgio);
> > +}
> > +
>
> Would it make sense to replace calls to nfs_pageio_stop_mirroring()
> with nfs_pageio_complete() instead?
>
No. Let's keep the wrapper, since it makes the writeback code easier to
read (it expresses the intent more clearly).
> Anna
>
> > int __init nfs_init_nfspagecache(void)
> > {
> > nfs_page_cachep = kmem_cache_create("nfs_page",
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]