[PATCH] nfsd: allow reaping files that are still under writeback

Subsystems: filesystems (vfs and infrastructure), kernel nfsd, sunrpc, and lockd servers, the rest

STALE1337d

7 messages, 3 authors, 2023-02-14 · open the first message on its own page

[PATCH] nfsd: allow reaping files that are still under writeback

From: Jeff Layton <jlayton@kernel.org>
Date: 2023-02-13 20:23:54

There's no reason to delay reaping an nfsd_file just because its
underlying inode is still under writeback. nfsd just relies on client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does it help
with tracking errors. Just allow it to close and let the kernel do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)
 	call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
 }
 
-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-	struct file *file = nf->nf_file;
-	struct address_space *mapping;
-
-	if (!file || !(file->f_mode & FMODE_WRITE))
-		return false;
-	mapping = file->f_mapping;
-	return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
-}
-
 static bool nfsd_file_lru_add(struct nfsd_file *nf)
 {
 	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
 	/* We should only be dealing with GC entries here */
 	WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));
 
-	/*
-	 * Don't throw out files that are still undergoing I/O or
-	 * that have uncleared errors pending.
-	 */
-	if (nfsd_file_check_writeback(nf)) {
-		trace_nfsd_file_gc_writeback(nf);
-		return LRU_SKIP;
-	}
-
 	/* If it was recently added to the list, skip it */
 	if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf->nf_flags)) {
 		trace_nfsd_file_gc_referenced(nf);

-- 
2.39.1

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Chuck Lever III <hidden>
Date: 2023-02-14 14:48:25

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref] wrote:

There's no reason to delay reaping an nfsd_file just because its
underlying inode is still under writeback. nfsd just relies on client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does it help
with tracking errors. Just allow it to close and let the kernel do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted hunk

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)

	call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-	struct file *file = nf->nf_file;
-	struct address_space *mapping;
-
-	if (!file || !(file->f_mode & FMODE_WRITE))
-		return false;
-	mapping = file->f_mapping;
-	return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,

	/* We should only be dealing with GC entries here */
	WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-	/*
-	 * Don't throw out files that are still undergoing I/O or
-	 * that have uncleared errors pending.
-	 */
-	if (nfsd_file_check_writeback(nf)) {
-		trace_nfsd_file_gc_writeback(nf);
-		return LRU_SKIP;
-	}
-
	/* If it was recently added to the list, skip it */
	if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf->nf_flags)) {
		trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Trond Myklebust <trondmy@kernel.org>
Date: 2023-02-14 15:00:59

On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:

quoted

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref]
wrote:

There's no reason to delay reaping an nfsd_file just because its
underlying inode is still under writeback. nfsd just relies on
client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does it
help
with tracking errors. Just allow it to close and let the kernel do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)

        call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-       struct file *file = nf->nf_file;
-       struct address_space *mapping;
-
-       if (!file || !(file->f_mode & FMODE_WRITE))
-               return false;
-       mapping = file->f_mapping;
-       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-               mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
        set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,

struct list_lru_one *lru,
        /* We should only be dealing with GC entries here */
        WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-       /*
-        * Don't throw out files that are still undergoing I/O or
-        * that have uncleared errors pending.
-        */
-       if (nfsd_file_check_writeback(nf)) {
-               trace_nfsd_file_gc_writeback(nf);
-               return LRU_SKIP;
-       }
-
        /* If it was recently added to the list, skip it */
        if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-

quoted

nf_flags)) {

                trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Wait... There is a good reason for wanting to do this in the case of
NFS re-exports, since close() is a very expensive operation if the file
has dirty data.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Chuck Lever III <hidden>
Date: 2023-02-14 15:05:22

On Feb 14, 2023, at 10:00 AM, Trond Myklebust [off-list ref] wrote:

On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:

quoted

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref]
wrote:

There's no reason to delay reaping an nfsd_file just because its
underlying inode is still under writeback. nfsd just relies on
client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does it
help
with tracking errors. Just allow it to close and let the kernel do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)
        call_rcu(&nf->nf_rcu, nfsd_file_slab_free);

}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-       struct file *file = nf->nf_file;
-       struct address_space *mapping;
-
-       if (!file || !(file->f_mode & FMODE_WRITE))
-               return false;
-       mapping = file->f_mapping;
-       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-               mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
        set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,

struct list_lru_one *lru,
        /* We should only be dealing with GC entries here */
        WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-       /*
-        * Don't throw out files that are still undergoing I/O or
-        * that have uncleared errors pending.
-        */
-       if (nfsd_file_check_writeback(nf)) {
-               trace_nfsd_file_gc_writeback(nf);
-               return LRU_SKIP;
-       }
-
        /* If it was recently added to the list, skip it */
        if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-

quoted

nf_flags)) {

                trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Wait... There is a good reason for wanting to do this in the case of
NFS re-exports, since close() is a very expensive operation if the file
has dirty data.

Then perhaps skipping these files can be gated on an EXPORT_OP flag?


--
Chuck Lever

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Trond Myklebust <trondmy@kernel.org>
Date: 2023-02-14 15:11:41

On Tue, 2023-02-14 at 15:05 +0000, Chuck Lever III wrote:

quoted

On Feb 14, 2023, at 10:00 AM, Trond Myklebust [off-list ref]
wrote:

On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:

quoted

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref]
wrote:

There's no reason to delay reaping an nfsd_file just because
its
underlying inode is still under writeback. nfsd just relies on
client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does
it
help
with tracking errors. Just allow it to close and let the kernel
do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)

        call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-       struct file *file = nf->nf_file;
-       struct address_space *mapping;
-
-       if (!file || !(file->f_mode & FMODE_WRITE))
-               return false;
-       mapping = file->f_mapping;
-       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-               mapping_tagged(mapping,
PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
        set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,

struct list_lru_one *lru,
        /* We should only be dealing with GC entries here */
        WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-       /*
-        * Don't throw out files that are still undergoing I/O
or
-        * that have uncleared errors pending.
-        */
-       if (nfsd_file_check_writeback(nf)) {
-               trace_nfsd_file_gc_writeback(nf);
-               return LRU_SKIP;
-       }
-
        /* If it was recently added to the list, skip it */
        if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-

quoted

nf_flags)) {

                trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Wait... There is a good reason for wanting to do this in the case
of
NFS re-exports, since close() is a very expensive operation if the
file
has dirty data.

Then perhaps skipping these files can be gated on an EXPORT_OP flag?

That would work.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Jeff Layton <jlayton@kernel.org>
Date: 2023-02-14 15:16:37

On Tue, 2023-02-14 at 10:00 -0500, Trond Myklebust wrote:

On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:

quoted

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref]
wrote:

There's no reason to delay reaping an nfsd_file just because its
underlying inode is still under writeback. nfsd just relies on
client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does it
help
with tracking errors. Just allow it to close and let the kernel do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)

        call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-       struct file *file = nf->nf_file;
-       struct address_space *mapping;
-
-       if (!file || !(file->f_mode & FMODE_WRITE))
-               return false;
-       mapping = file->f_mapping;
-       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-               mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
        set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,

struct list_lru_one *lru,
        /* We should only be dealing with GC entries here */
        WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-       /*
-        * Don't throw out files that are still undergoing I/O or
-        * that have uncleared errors pending.
-        */
-       if (nfsd_file_check_writeback(nf)) {
-               trace_nfsd_file_gc_writeback(nf);
-               return LRU_SKIP;
-       }
-
        /* If it was recently added to the list, skip it */
        if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-

quoted

nf_flags)) {

                trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Wait... There is a good reason for wanting to do this in the case of
NFS re-exports, since close() is a very expensive operation if the file
has dirty data.

Fair enough. What if we added a new EXPORT_OP_FLUSH_ON_CLOSE flag that
filesystems could set in the export_operations? Then we could make this
behavior conditional on that being set?
-- 
Jeff Layton [off-list ref]

Re: [PATCH] nfsd: allow reaping files that are still under writeback

From: Trond Myklebust <trondmy@kernel.org>
Date: 2023-02-14 15:22:24

On Tue, 2023-02-14 at 10:16 -0500, Jeff Layton wrote:

On Tue, 2023-02-14 at 10:00 -0500, Trond Myklebust wrote:

quoted

On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:

quoted

On Feb 13, 2023, at 3:23 PM, Jeff Layton [off-list ref]
wrote:

There's no reason to delay reaping an nfsd_file just because
its
underlying inode is still under writeback. nfsd just relies on
client
activity or the local flusher threads to do writeback.

Holding the file open does nothing to facilitate that, nor does
it
help
with tracking errors. Just allow it to close and let the kernel
do
writeback as it normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks! Applied to topic-filecache-cleanups.

quoted

---
fs/nfsd/filecache.c | 22 ----------------------
1 file changed, 22 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e6617431df7c..3b9a10378c83 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)

        call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
}

-static bool
-nfsd_file_check_writeback(struct nfsd_file *nf)
-{
-       struct file *file = nf->nf_file;
-       struct address_space *mapping;
-
-       if (!file || !(file->f_mode & FMODE_WRITE))
-               return false;
-       mapping = file->f_mapping;
-       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
-               mapping_tagged(mapping,
PAGECACHE_TAG_WRITEBACK);
-}
-
static bool nfsd_file_lru_add(struct nfsd_file *nf)
{
        set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);

@@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,

struct list_lru_one *lru,
        /* We should only be dealing with GC entries here */
        WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));

-       /*
-        * Don't throw out files that are still undergoing I/O
or
-        * that have uncleared errors pending.
-        */
-       if (nfsd_file_check_writeback(nf)) {
-               trace_nfsd_file_gc_writeback(nf);
-               return LRU_SKIP;
-       }
-
        /* If it was recently added to the list, skip it */
        if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-

quoted

nf_flags)) {

                trace_nfsd_file_gc_referenced(nf);
-- 
2.39.1

--
Chuck Lever

Wait... There is a good reason for wanting to do this in the case
of
NFS re-exports, since close() is a very expensive operation if the
file
has dirty data.

Fair enough. What if we added a new EXPORT_OP_FLUSH_ON_CLOSE flag
that
filesystems could set in the export_operations? Then we could make
this
behavior conditional on that being set?

Sure.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help