| Message ID | 20220614152817.271507-1-chenxiaosong2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v2] NFS: report and clear ENOSPC/EFBIG/EDQUOT writeback error on close() file | expand |
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
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
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
在 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().
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.
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
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(-)