[PATCH -next,v2 0/3] nfs: handle writeback errors correctly

[PATCH -next,v2 0/3] nfs: handle writeback errors correctly

[ChenXiaoSong]

v1:
cover letter: (nfs: check writeback errors correctly)

v2:
- return more nuanced writeback errors in nfs_file_write().
- return writeback error in close()->flush() without consumed it.
- fix: nfs_file_write() will always call nfs_wb_all() even if there is no
new writeback error.


ChenXiaoSong (3):
  NFS: return more nuanced writeback errors in nfs_file_write()
  NFS: nfs{,4}_file_flush() return correct writeback errors
  Revert "nfs: nfs_file_write() should check for writeback errors"

 fs/nfs/file.c     | 23 ++++++++++-------------
 fs/nfs/nfs4file.c |  8 ++++----
 fs/nfs/write.c    |  5 +----
 3 files changed, 15 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH -next,v2 1/3] NFS: return more nuanced writeback errors in nfs_file_write()

[ChenXiaoSong]

Currently, writeback error in address_space flags will not be set in
nfs_mapping_set_error(), return value of nfs_wb_all() is always 0 even if
there is new writeback error. And error from filemap_check_wb_err() is just
used to check, will not be reported to userspace.

Furthermore, filemap_sample_wb_err() always return 0 if old writeback error
have not been consumed. filemap_check_wb_err() will return the old error
even if there is no new writeback error between filemap_sample_wb_err() and
filemap_check_wb_err().

So we still need record writeback error in address_space flags. The writeback
error in address_space flags is not used to be reported to userspace, it is just
used to detect if there is new error while writeback.

generic_perform_write() detect wb error by calling filemap_check_errors():
  generic_perform_write
    nfs_write_end
      nfs_wb_all
        filemap_write_and_wait
          filemap_write_and_wait_range
            filemap_check_errors

filemap_fdatawait_range() detect wb error by calling filemap_check_errors():
  filemap_fdatawait_range
    __filemap_fdatawait_range
    filemap_check_errors

As filemap_check_errors() only report -EIO or -ENOSPC, we must use the more nuanced
writeback error for -EIO by returning -(file->f_mapping->wb_err & MAX_ERRNO).

Fixes: 6c984083ec24 ("NFS: Use of mapping_set_error() results in spurious errors")
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c  | 3 +++
 fs/nfs/write.c | 5 +----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index b0ca244c50d0..5513ab63c108 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -683,6 +683,9 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 	}
 	nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, written);
 out:
+	/* Use the more nuanced writeback error for -EIO */
+	if (result == -EIO)
+		result = filemap_check_wb_err(file->f_mapping, 0);
 	return result;
 
 out_swapfile:
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index f00d45cf80ef..eec15efb41ab 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -312,10 +312,7 @@ static void nfs_mapping_set_error(struct page *page, int error)
 	struct address_space *mapping = page_file_mapping(page);
 
 	SetPageError(page);
-	filemap_set_wb_err(mapping, error);
-	if (mapping->host)
-		errseq_set(&mapping->host->i_sb->s_wb_err,
-			   error == -ENOSPC ? -ENOSPC : -EIO);
+	mapping_set_error(mapping, error);
 	nfs_set_pageerror(mapping);
 }
 
-- 
2.31.1

[PATCH -next,v2 2/3] NFS: nfs{,4}_file_flush() return correct writeback errors

[ChenXiaoSong]

filemap_sample_wb_err() will return 0 if old wb error have not been consumed,
then filemap_check_wb_err() will return the unchanged old wb error.

Reproducer:
        nfs server            |       nfs client
 -----------------------------|---------------------------------------------
 # No space left on server    |
 fallocate -l 100G /svr/nospc |
                              | mount -t nfs $nfs_server_ip:/ /mnt
                              |
                              | # Expected error: No space left on device
                              | dd if=/dev/zero of=/mnt/file count=1 ibs=10K
                              |
                              | # Release space on mountpoint
                              | rm /mnt/nospc
                              |
                              | # Unexpected error: No space left on device
                              | dd if=/dev/zero of=/mnt/file count=1 ibs=10K

Fix this by detecting writeback error from return value of nfs_wb_all(),
and return the more nuanced error -(file->f_mapping->wb_err & MAX_ERRNO) if
there is new wb error while nfs_wb_all().

Fixes: 67dd23f9e6fb ("nfs: ensure correct writeback errors are returned on close()")
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c     | 8 ++++----
 fs/nfs/nfs4file.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 5513ab63c108..353f1f832519 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -136,7 +136,7 @@ static int
 nfs_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
+	errseq_t error = 0;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -145,9 +145,9 @@ nfs_file_flush(struct file *file, fl_owner_t id)
 		return 0;
 
 	/* Flush writes to the server and return any errors */
-	since = filemap_sample_wb_err(file->f_mapping);
-	nfs_wb_all(inode);
-	return filemap_check_wb_err(file->f_mapping, since);
+	if (nfs_wb_all(inode))
+		error = filemap_check_wb_err(file->f_mapping, 0);
+	return error;
 }
 
 ssize_t
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index d258933cf8c8..cbbe66b54417 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -111,7 +111,7 @@ static int
 nfs4_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
+	errseq_t error = 0;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -127,9 +127,9 @@ nfs4_file_flush(struct file *file, fl_owner_t id)
 		return filemap_fdatawrite(file->f_mapping);
 
 	/* Flush writes to the server and return any errors */
-	since = filemap_sample_wb_err(file->f_mapping);
-	nfs_wb_all(inode);
-	return filemap_check_wb_err(file->f_mapping, since);
+	if (nfs_wb_all(inode))
+		error = filemap_check_wb_err(file->f_mapping, 0);
+	return error;
 }
 
 #ifdef CONFIG_NFS_V4_2
-- 
2.31.1

[PATCH -next,v2 3/3] Revert "nfs: nfs_file_write() should check for writeback errors"

[ChenXiaoSong]

This reverts commit ce368536dd614452407dc31e2449eb84681a06af.

Second `dd` of the following reproducer will be very very slow.
filemap_sample_wb_err() always return 0 if old wb error have not been consumed.
filemap_check_wb_err() will return the old error even if there is no new
writeback error between filemap_sample_wb_err() and filemap_check_wb_err().
Then nfs_need_check_write() always return true, nfs_wb_all() will be called
every time in nfs_file_write().

Reproducer:
        nfs server            |       nfs client
 -----------------------------|---------------------------------------------
 # No space left on server    |
 fallocate -l 100G /svr/nospc |
                              | mount -t nfs $nfs_server_ip:/ /mnt
                              |
                              | # Expected error: No space left on device
                              | dd if=/dev/zero of=/mnt/file count=1 ibs=1M
                              |
                              | # Release space on mountpoint
                              | rm /mnt/nospc
                              |
                              | # very very slow
                              | dd if=/dev/zero of=/mnt/file count=1 ibs=1M

generic_perform_write() detect wb error by calling filemap_check_errors():
  generic_perform_write
    nfs_write_end
      nfs_wb_all
        filemap_write_and_wait
          filemap_write_and_wait_range
            filemap_check_errors

filemap_fdatawait_range() detect wb error by calling filemap_check_errors():
  filemap_fdatawait_range
    __filemap_fdatawait_range
    filemap_check_errors

generic_write_sync() will also detect wb error by calling nfs_file_fsync():
  generic_write_sync
    vfs_fsync_range
      nfs_file_fsync
        file_write_and_wait_range
          file_check_and_advance_wb_err
            errseq_check_and_advance

When writeback error is detected in nfs_file_write(), we just goto label "out",
will not goto nfs_need_check_write(). So we remove the useless and problematic
checking.

Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 353f1f832519..49f1485a30bb 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -597,14 +597,12 @@ static const struct vm_operations_struct nfs_file_vm_ops = {
 	.page_mkwrite = nfs_vm_page_mkwrite,
 };
 
-static int nfs_need_check_write(struct file *filp, struct inode *inode,
-				int error)
+static int nfs_need_check_write(struct file *filp, struct inode *inode)
 {
 	struct nfs_open_context *ctx;
 
 	ctx = nfs_file_open_context(filp);
-	if (nfs_error_is_fatal_on_server(error) ||
-	    nfs_ctx_key_to_expire(ctx, inode))
+	if (nfs_ctx_key_to_expire(ctx, inode))
 		return 1;
 	return 0;
 }
@@ -615,8 +613,6 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 	struct inode *inode = file_inode(file);
 	unsigned int mntflags = NFS_SERVER(inode)->flags;
 	ssize_t result, written;
-	errseq_t since;
-	int error;
 
 	result = nfs_key_timeout_notify(file, inode);
 	if (result)
@@ -641,7 +637,6 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 
 	nfs_clear_invalid_mapping(file->f_mapping);
 
-	since = filemap_sample_wb_err(file->f_mapping);
 	nfs_start_io_write(inode);
 	result = generic_write_checks(iocb, from);
 	if (result > 0) {
@@ -675,8 +670,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 
 	/* Return error values */
-	error = filemap_check_wb_err(file->f_mapping, since);
-	if (nfs_need_check_write(file, inode, error)) {
+	if (nfs_need_check_write(file, inode)) {
 		int err = nfs_wb_all(inode);
 		if (err < 0)
 			result = err;
-- 
2.31.1

Re: [PATCH -next,v2 0/3] nfs: handle writeback errors correctly

[chenxiaosong (A)]

在 2022/4/1 11:44, ChenXiaoSong 写道:
> v1:
> cover letter: (nfs: check writeback errors correctly)
> 
> v2:
> - return more nuanced writeback errors in nfs_file_write().
> - return writeback error in close()->flush() without consumed it.
> - fix: nfs_file_write() will always call nfs_wb_all() even if there is no
> new writeback error.
> 
> 
> ChenXiaoSong (3):
>    NFS: return more nuanced writeback errors in nfs_file_write()
>    NFS: nfs{,4}_file_flush() return correct writeback errors
>    Revert "nfs: nfs_file_write() should check for writeback errors"
> 
>   fs/nfs/file.c     | 23 ++++++++++-------------
>   fs/nfs/nfs4file.c |  8 ++++----
>   fs/nfs/write.c    |  5 +----
>   3 files changed, 15 insertions(+), 21 deletions(-)
> 

It is not a good idea to modify error sequence mechanism, as the 
`lib/errseq.c` described:

	22  * Note that there is a risk of collisions if new errors are being 
recorded
	23  * frequently, since we have so few bits to use as a counter. 

	24  *
	25  * To mitigate this, one bit is used as a flag to tell whether the 
value has
	26  * been sampled since a new value was recorded. That allows us to 
avoid bumping
	27  * the counter if no one has sampled it since the last time an error was
	28  * recorded.


So, if we want to report nuanced writeback error, it is better to detect 
wb error from filemap_check_errors(), and then return 
-(file->f_mapping->wb_err & MAX_ERRNO) to userspace without consume it.

   nfs_mapping_set_error
     mapping_set_error
       __filemap_set_wb_err // record error sequence
         errseq_set
       set_bit(..., &mapping->flags) // record address_space flag

   // it is not used to be reported, just used to detect
   error = filemap_check_errors // -ENOSPC or -EIO
     test_and_clear_bit(..., &mapping->flags) // error bit cleared

   // now we try to return nuanced writeback error
   if (error)
   return filemap_check_wb_err(file->f_mapping, 0);
     return -(file->f_mapping->wb_err & MAX_ERRNO)

Re: [PATCH -next,v2 0/3] nfs: handle writeback errors correctly

[chenxiaosong (A)]

ping ...

在 2022/4/1 15:03, chenxiaosong (A) 写道:
> 在 2022/4/1 11:44, ChenXiaoSong 写道:
>> v1:
>> cover letter: (nfs: check writeback errors correctly)
>>
>> v2:
>> - return more nuanced writeback errors in nfs_file_write().
>> - return writeback error in close()->flush() without consumed it.
>> - fix: nfs_file_write() will always call nfs_wb_all() even if there is no
>> new writeback error.
>>
>>
>> ChenXiaoSong (3):
>>    NFS: return more nuanced writeback errors in nfs_file_write()
>>    NFS: nfs{,4}_file_flush() return correct writeback errors
>>    Revert "nfs: nfs_file_write() should check for writeback errors"
>>
>>   fs/nfs/file.c     | 23 ++++++++++-------------
>>   fs/nfs/nfs4file.c |  8 ++++----
>>   fs/nfs/write.c    |  5 +----
>>   3 files changed, 15 insertions(+), 21 deletions(-)
>>
> 
> It is not a good idea to modify error sequence mechanism, as the 
> `lib/errseq.c` described:
> 
>      22  * Note that there is a risk of collisions if new errors are 
> being recorded
>      23  * frequently, since we have so few bits to use as a counter.
>      24  *
>      25  * To mitigate this, one bit is used as a flag to tell whether 
> the value has
>      26  * been sampled since a new value was recorded. That allows us 
> to avoid bumping
>      27  * the counter if no one has sampled it since the last time an 
> error was
>      28  * recorded.
> 
> 
> So, if we want to report nuanced writeback error, it is better to detect 
> wb error from filemap_check_errors(), and then return 
> -(file->f_mapping->wb_err & MAX_ERRNO) to userspace without consume it.
> 
>    nfs_mapping_set_error
>      mapping_set_error
>        __filemap_set_wb_err // record error sequence
>          errseq_set
>        set_bit(..., &mapping->flags) // record address_space flag
> 
>    // it is not used to be reported, just used to detect
>    error = filemap_check_errors // -ENOSPC or -EIO
>      test_and_clear_bit(..., &mapping->flags) // error bit cleared
> 
>    // now we try to return nuanced writeback error
>    if (error)
>    return filemap_check_wb_err(file->f_mapping, 0);
>      return -(file->f_mapping->wb_err & MAX_ERRNO)
>