[PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[ChenXiaoSong]

Currently, we report and clear ENOSPC/EFBIG/EDQUOT writeback error on write(),
write() file will report unexpected error if previous writeback error have not
been cleared.

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
                              |
                              | # Just write 512B and report unexpected error
                              | dd if=/dev/zero of=/mnt/file count=1 ibs=10K

Fix this by clearing ENOSPC/EFBIG/EDQUOT writeback error on close file,
it will not clear other errors that are not supposed to be reported by close().

Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c     | 16 ++++++++--------
 fs/nfs/internal.h | 10 ++++++++++
 fs/nfs/nfs4file.c |  9 +++++++--
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 2d72b1b7ed74..275d1fdc7f9a 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -138,7 +138,7 @@ static int
 nfs_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
+	errseq_t since, error;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -149,7 +149,12 @@ nfs_file_flush(struct file *file, fl_owner_t id)
 	/* 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);
+	error = filemap_check_wb_err(file->f_mapping, since);
+
+	if (nfs_should_clear_wb_err(error))
+		file_check_and_advance_wb_err(file);
+
+	return error;
 }
 
 ssize_t
@@ -673,12 +678,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 out:
 	/* Return error values */
 	error = filemap_check_wb_err(file->f_mapping, since);
-	switch (error) {
-	default:
-		break;
-	case -EDQUOT:
-	case -EFBIG:
-	case -ENOSPC:
+	if (nfs_should_clear_wb_err(error)) {
 		nfs_wb_all(inode);
 		error = file_check_and_advance_wb_err(file);
 		if (error < 0)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 8f8cd6e2d4db..e49aad8f7d09 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -859,3 +859,13 @@ static inline void nfs_set_port(struct sockaddr *sap, int *port,
 
 	rpc_set_port(sap, *port);
 }
+
+static inline bool nfs_should_clear_wb_err(int error) {
+	switch (error) {
+	case -EDQUOT:
+	case -EFBIG:
+	case -ENOSPC:
+		return true;
+	}
+	return false;
+}
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 03d3a270eff4..ddf3f0abd55a 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -113,7 +113,7 @@ static int
 nfs4_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
+	errseq_t since, error;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -131,7 +131,12 @@ nfs4_file_flush(struct file *file, fl_owner_t id)
 	/* 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);
+	error = filemap_check_wb_err(file->f_mapping, since);
+
+	if (nfs_should_clear_wb_err(error))
+		file_check_and_advance_wb_err(file);
+
+	return error;
 }
 
 #ifdef CONFIG_NFS_V4_2
-- 
2.31.1

Re: [PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[Trond Myklebust]

On Tue, 2022-06-14 at 23:28 +0800, ChenXiaoSong wrote:
> Currently, we report and clear ENOSPC/EFBIG/EDQUOT writeback error on
> write(),
> write() file will report unexpected error if previous writeback error
> have not
> been cleared.
> 
> 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
>                               |
>                               | # Just write 512B and report
> unexpected error
>                               | dd if=/dev/zero of=/mnt/file count=1
> ibs=10K
> 
> Fix this by clearing ENOSPC/EFBIG/EDQUOT writeback error on close
> file,
> it will not clear other errors that are not supposed to be reported
> by close().
> 
> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> ---
>  fs/nfs/file.c     | 16 ++++++++--------
>  fs/nfs/internal.h | 10 ++++++++++
>  fs/nfs/nfs4file.c |  9 +++++++--
>  3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 2d72b1b7ed74..275d1fdc7f9a 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -138,7 +138,7 @@ static int
>  nfs_file_flush(struct file *file, fl_owner_t id)
>  {
>         struct inode    *inode = file_inode(file);
> -       errseq_t since;
> +       errseq_t since, error;
>  
>         dprintk("NFS: flush(%pD2)\n", file);
>  
> @@ -149,7 +149,12 @@ nfs_file_flush(struct file *file, fl_owner_t id)
>         /* 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);
> +       error = filemap_check_wb_err(file->f_mapping, since);
> +
> +       if (nfs_should_clear_wb_err(error))
> +               file_check_and_advance_wb_err(file);

NACK. How many times do I have to repeat that we do NOT clear the error
log in flush()?



> +
> +       return error;
>  }
>  
>  ssize_t
> @@ -673,12 +678,7 @@ ssize_t nfs_file_write(struct kiocb *iocb,
> struct iov_iter *from)
>  out:
>         /* Return error values */
>         error = filemap_check_wb_err(file->f_mapping, since);
> -       switch (error) {
> -       default:
> -               break;
> -       case -EDQUOT:
> -       case -EFBIG:
> -       case -ENOSPC:
> +       if (nfs_should_clear_wb_err(error)) {
>                 nfs_wb_all(inode);
>                 error = file_check_and_advance_wb_err(file);
>                 if (error < 0)
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 8f8cd6e2d4db..e49aad8f7d09 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -859,3 +859,13 @@ static inline void nfs_set_port(struct sockaddr
> *sap, int *port,
>  
>         rpc_set_port(sap, *port);
>  }
> +
> +static inline bool nfs_should_clear_wb_err(int error) {
> +       switch (error) {
> +       case -EDQUOT:
> +       case -EFBIG:
> +       case -ENOSPC:
> +               return true;
> +       }
> +       return false;
> +}
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index 03d3a270eff4..ddf3f0abd55a 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -113,7 +113,7 @@ static int
>  nfs4_file_flush(struct file *file, fl_owner_t id)
>  {
>         struct inode    *inode = file_inode(file);
> -       errseq_t since;
> +       errseq_t since, error;
>  
>         dprintk("NFS: flush(%pD2)\n", file);
>  
> @@ -131,7 +131,12 @@ nfs4_file_flush(struct file *file, fl_owner_t
> id)
>         /* 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);
> +       error = filemap_check_wb_err(file->f_mapping, since);
> +
> +       if (nfs_should_clear_wb_err(error))
> +               file_check_and_advance_wb_err(file);
> +
> +       return error;
>  }
>  
>  #ifdef CONFIG_NFS_V4_2

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

Re: [PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[kernel test robot]

Hi ChenXiaoSong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc2]
[also build test WARNING on next-20220614]
[cannot apply to trondmy-nfs/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/ChenXiaoSong/NFS-report-and-clear-ENOSPC-EFBIG-EDQUOT-writeback-error-on-close-file/20220614-231738
base:    b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20220615/202206150810.4h1FZM2Z-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f65252667ed27ca5a3e7f2182d1819d009dc98d7
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review ChenXiaoSong/NFS-report-and-clear-ENOSPC-EFBIG-EDQUOT-writeback-error-on-close-file/20220614-231738
        git checkout f65252667ed27ca5a3e7f2182d1819d009dc98d7
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash fs/nfs/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/nfs/file.c: In function 'nfs_file_flush':
>> fs/nfs/file.c:155:17: warning: ignoring return value of 'file_check_and_advance_wb_err' declared with attribute 'warn_unused_result' [-Wunused-result]
     155 |                 file_check_and_advance_wb_err(file);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +155 fs/nfs/file.c

   133	
   134	/*
   135	 * Flush all dirty pages, and check for write errors.
   136	 */
   137	static int
   138	nfs_file_flush(struct file *file, fl_owner_t id)
   139	{
   140		struct inode	*inode = file_inode(file);
   141		errseq_t since, error;
   142	
   143		dprintk("NFS: flush(%pD2)\n", file);
   144	
   145		nfs_inc_stats(inode, NFSIOS_VFSFLUSH);
   146		if ((file->f_mode & FMODE_WRITE) == 0)
   147			return 0;
   148	
   149		/* Flush writes to the server and return any errors */
   150		since = filemap_sample_wb_err(file->f_mapping);
   151		nfs_wb_all(inode);
   152		error = filemap_check_wb_err(file->f_mapping, since);
   153	
   154		if (nfs_should_clear_wb_err(error))
 > 155			file_check_and_advance_wb_err(file);
   156	
   157		return error;
   158	}
   159	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[kernel test robot]

Hi ChenXiaoSong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc2]
[also build test WARNING on next-20220614]
[cannot apply to trondmy-nfs/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/ChenXiaoSong/NFS-report-and-clear-ENOSPC-EFBIG-EDQUOT-writeback-error-on-close-file/20220614-231738
base:    b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
config: x86_64-rhel-8.3-func (https://download.01.org/0day-ci/archive/20220615/202206150903.fMetZz83-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/f65252667ed27ca5a3e7f2182d1819d009dc98d7
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review ChenXiaoSong/NFS-report-and-clear-ENOSPC-EFBIG-EDQUOT-writeback-error-on-close-file/20220614-231738
        git checkout f65252667ed27ca5a3e7f2182d1819d009dc98d7
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/nfs/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/nfs/nfs4file.c: In function 'nfs4_file_flush':
>> fs/nfs/nfs4file.c:137:17: warning: ignoring return value of 'file_check_and_advance_wb_err' declared with attribute 'warn_unused_result' [-Wunused-result]
     137 |                 file_check_and_advance_wb_err(file);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +137 fs/nfs/nfs4file.c

   108	
   109	/*
   110	 * Flush all dirty pages, and check for write errors.
   111	 */
   112	static int
   113	nfs4_file_flush(struct file *file, fl_owner_t id)
   114	{
   115		struct inode	*inode = file_inode(file);
   116		errseq_t since, error;
   117	
   118		dprintk("NFS: flush(%pD2)\n", file);
   119	
   120		nfs_inc_stats(inode, NFSIOS_VFSFLUSH);
   121		if ((file->f_mode & FMODE_WRITE) == 0)
   122			return 0;
   123	
   124		/*
   125		 * If we're holding a write delegation, then check if we're required
   126		 * to flush the i/o on close. If not, then just start the i/o now.
   127		 */
   128		if (!nfs4_delegation_flush_on_close(inode))
   129			return filemap_fdatawrite(file->f_mapping);
   130	
   131		/* Flush writes to the server and return any errors */
   132		since = filemap_sample_wb_err(file->f_mapping);
   133		nfs_wb_all(inode);
   134		error = filemap_check_wb_err(file->f_mapping, since);
   135	
   136		if (nfs_should_clear_wb_err(error))
 > 137			file_check_and_advance_wb_err(file);
   138	
   139		return error;
   140	}
   141	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[chenxiaosong (A)]

在 2022/6/15 3:48, Trond Myklebust 写道:
> NACK. How many times do I have to repeat that we do NOT clear the error
> log in flush()?
> 

close(2) manpage described:

> ENOSPC, EDQUOT: On NFS, these errors are not normally reported
> against the first write which exceeds the available storage space,
> but instead against a subsequent write(2), fsync(2), or close(2).

> A  careful programmer will check the return value of close(), since
> it is quite possible that errors on a previous write(2) operation are
> reported only on the final close() that releases the open file
> description.  Failing to check the return value when closing a file
> may lead to silent loss of data.  This can especially be observed 
> with NFS and with disk quota.

write(2) manpage described:

> Since Linux 4.13, errors from write-back come with a promise that they
> may be reported by subsequent.  write(2) requests, and will be
> reported by a subsequent fsync(2) (whether or not they were also
> reported by write(2)).

Both close(2) and write(2) manpage described: report writeback error 
(not clear error), especially the write(2) manpage described: will be 
reported by a subsequent fsync(2) whether or not they were also reported 
by write(2).

If ENOSPC/EFBIG/EDQUOT writeback error can be cleared on write(), maybe 
it is better to be cleared on close() instead of saving the error for 
next open().

Re: [PATCH -next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file

[Trond Myklebust]

On Wed, 2022-06-15 at 09:34 +0800, chenxiaosong (A) wrote:
> 在 2022/6/15 3:48, Trond Myklebust 写道:
> > NACK. How many times do I have to repeat that we do NOT clear the
> > error
> > log in flush()?
> > 
> 
> close(2) manpage described:
> 
> > ENOSPC, EDQUOT: On NFS, these errors are not normally reported
> > against the first write which exceeds the available storage space,
> > but instead against a subsequent write(2), fsync(2), or close(2).
> 
> > A  careful programmer will check the return value of close(), since
> > it is quite possible that errors on a previous write(2) operation
> > are
> > reported only on the final close() that releases the open file
> > description.  Failing to check the return value when closing a file
> > may lead to silent loss of data.  This can especially be observed 
> > with NFS and with disk quota.
> 
> write(2) manpage described:
> 
> > Since Linux 4.13, errors from write-back come with a promise that
> > they
> > may be reported by subsequent.  write(2) requests, and will be
> > reported by a subsequent fsync(2) (whether or not they were also
> > reported by write(2)).
> 
> Both close(2) and write(2) manpage described: report writeback error 
> (not clear error), especially the write(2) manpage described: will be
> reported by a subsequent fsync(2) whether or not they were also
> reported 
> by write(2).
> 
> If ENOSPC/EFBIG/EDQUOT writeback error can be cleared on write(),
> maybe 
> it is better to be cleared on close() instead of saving the error for
> next open().

We've already had this discussion. I'm not making any exceptions for
NFS to rules that apply to all filesystems.

If you want the rules to change, then you need to talk to the entire
filesystem community and get them to accept that the VFS level
implementation of error handling is incorrect.

That's my final word on this subject.

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