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

[Trond Myklebust <trondmy@hammerspace.com>]

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