| Message ID | 20220523014838.1647498-1-chenxiaosong2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v2] fuse: return the more nuanced writeback error on close() | expand |
On Mon, 23 May 2022 at 03:35, ChenXiaoSong <chenxiaosong2@huawei.com> wrote: > > As filemap_check_errors() only report -EIO or -ENOSPC, we return more nuanced > writeback error -(file->f_mapping->wb_err & MAX_ERRNO). > > filemap_write_and_wait > filemap_write_and_wait_range > filemap_check_errors > -ENOSPC or -EIO > filemap_check_wb_err > errseq_check > return -(file->f_mapping->wb_err & MAX_ERRNO) > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > --- > fs/fuse/file.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index f18d14d5fea1..9917bc2795e6 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, fl_owner_t id) > inode_unlock(inode); > > err = filemap_check_errors(file->f_mapping); > + /* return more nuanced writeback errors */ > if (err) > - return err; > + return filemap_check_wb_err(file->f_mapping, 0); I'm wondering if this should be file_check_and_advance_wb_err() instead. Is there a difference between ->flush() and ->fsync()? Jeff, can you please help? Thanks, Miklos
On Mon, 2022-05-30 at 14:13 +0200, Miklos Szeredi wrote: > On Mon, 23 May 2022 at 03:35, ChenXiaoSong <chenxiaosong2@huawei.com> wrote: > > > > As filemap_check_errors() only report -EIO or -ENOSPC, we return more nuanced > > writeback error -(file->f_mapping->wb_err & MAX_ERRNO). > > > > filemap_write_and_wait > > filemap_write_and_wait_range > > filemap_check_errors > > -ENOSPC or -EIO > > filemap_check_wb_err > > errseq_check > > return -(file->f_mapping->wb_err & MAX_ERRNO) > > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > --- > > fs/fuse/file.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index f18d14d5fea1..9917bc2795e6 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, fl_owner_t id) > > inode_unlock(inode); > > > > err = filemap_check_errors(file->f_mapping); > > + /* return more nuanced writeback errors */ > > if (err) > > - return err; > > + return filemap_check_wb_err(file->f_mapping, 0); > > I'm wondering if this should be file_check_and_advance_wb_err() instead. > I think that it probably shouldn't be, actually. Reason below... > Is there a difference between ->flush() and ->fsync()? > > Jeff, can you please help? > > The main difference is that ->flush is called from filp_close, so it's called when a file descriptor (or equivalent) is being torn down out, whereas ->fsync is (obviously) called from the fsync codepath. We _must_ report writeback errors on fsync, but reporting them on the close() syscall is less clear. The thing about close() is that it's going be successful no matter what is returned. The file descriptor will no longer work afterward regardless. fsync also must also initiate writeback of all the buffered data, but it's not required for filesystems to do that on close() (and in fact, there are good reasons not to if you can). A successful close() tells you nothing about whether your data made it to the backing store. It might just not have been synced out yet. Personally, I think it's probably best to _not_ return writeback errors on close at all. The only "legitimate" error on close is -EBADF. Arguably, we should make ->flush be void return. Note that most filp_close callers ignore the error anyway, so it's not much of a stretch. In any case, if you do decide to return errors in fuse_flush, then advancing the cursor would also have the effect of masking writeback errors on dup()'ed file descriptors, and I don't think you want to do that.
On Mon, May 30, 2022 at 10:02:06AM -0400, Jeff Layton wrote: > The main difference is that ->flush is called from filp_close, so it's > called when a file descriptor (or equivalent) is being torn down out, > whereas ->fsync is (obviously) called from the fsync codepath. > > We _must_ report writeback errors on fsync, but reporting them on the > close() syscall is less clear. The thing about close() is that it's > going be successful no matter what is returned. The file descriptor will > no longer work afterward regardless. > > fsync also must also initiate writeback of all the buffered data, but > it's not required for filesystems to do that on close() (and in fact, > there are good reasons not to if you can). A successful close() tells > you nothing about whether your data made it to the backing store. It > might just not have been synced out yet. > > Personally, I think it's probably best to _not_ return writeback errors > on close at all. The only "legitimate" error on close is -EBADF. > Arguably, we should make ->flush be void return. Note that most > filp_close callers ignore the error anyway, so it's not much of a > stretch. > > In any case, if you do decide to return errors in fuse_flush, then > advancing the cursor would also have the effect of masking writeback > errors on dup()'ed file descriptors, and I don't think you want to do > that. Thanks for clarifying. Chen, would the following patch make sense for your case? Thanks, Miklos --- fs/fuse/file.c | 5 ----- 1 file changed, 5 deletions(-) --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -487,11 +487,6 @@ static int fuse_flush(struct file *file, fuse_sync_writes(inode); inode_unlock(inode); - err = filemap_check_errors(file->f_mapping); - if (err) - return err; - - err = 0; if (fm->fc->no_flush) goto inval_attr_out;
On Tue, 31 May 2022, Jeff Layton wrote: > On Mon, 2022-05-30 at 14:13 +0200, Miklos Szeredi wrote: > > On Mon, 23 May 2022 at 03:35, ChenXiaoSong <chenxiaosong2@huawei.com> wrote: > > > > > > As filemap_check_errors() only report -EIO or -ENOSPC, we return more nuanced > > > writeback error -(file->f_mapping->wb_err & MAX_ERRNO). > > > > > > filemap_write_and_wait > > > filemap_write_and_wait_range > > > filemap_check_errors > > > -ENOSPC or -EIO > > > filemap_check_wb_err > > > errseq_check > > > return -(file->f_mapping->wb_err & MAX_ERRNO) > > > > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > > --- > > > fs/fuse/file.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > > index f18d14d5fea1..9917bc2795e6 100644 > > > --- a/fs/fuse/file.c > > > +++ b/fs/fuse/file.c > > > @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, fl_owner_t id) > > > inode_unlock(inode); > > > > > > err = filemap_check_errors(file->f_mapping); > > > + /* return more nuanced writeback errors */ > > > if (err) > > > - return err; > > > + return filemap_check_wb_err(file->f_mapping, 0); > > > > I'm wondering if this should be file_check_and_advance_wb_err() instead. > > > > I think that it probably shouldn't be, actually. Reason below... > > > Is there a difference between ->flush() and ->fsync()? > > > > Jeff, can you please help? > > > > > > The main difference is that ->flush is called from filp_close, so it's > called when a file descriptor (or equivalent) is being torn down out, > whereas ->fsync is (obviously) called from the fsync codepath. ->flush is for cache coherence. It is best-effort ->fsync is for data safety. Obviously errors are important. > > We _must_ report writeback errors on fsync, but reporting them on the > close() syscall is less clear. The thing about close() is that it's > going be successful no matter what is returned. The file descriptor will > no longer work afterward regardless. > > fsync also must also initiate writeback of all the buffered data, but > it's not required for filesystems to do that on close() (and in fact, > there are good reasons not to if you can). A successful close() tells > you nothing about whether your data made it to the backing store. It > might just not have been synced out yet. > > Personally, I think it's probably best to _not_ return writeback errors > on close at all. The only "legitimate" error on close is -EBADF. > Arguably, we should make ->flush be void return. Note that most ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Excellent idea! NeilBrown > filp_close callers ignore the error anyway, so it's not much of a > stretch. > > In any case, if you do decide to return errors in fuse_flush, then > advancing the cursor would also have the effect of masking writeback > errors on dup()'ed file descriptors, and I don't think you want to do > that. > -- > Jeff Layton <jlayton@kernel.org> >
On Mon, 2022-05-30 at 14:13 +0200, Miklos Szeredi wrote: > On Mon, 23 May 2022 at 03:35, ChenXiaoSong <chenxiaosong2@huawei.com> > wrote: > > > > As filemap_check_errors() only report -EIO or -ENOSPC, we return > > more nuanced > > writeback error -(file->f_mapping->wb_err & MAX_ERRNO). > > > > filemap_write_and_wait > > filemap_write_and_wait_range > > filemap_check_errors > > -ENOSPC or -EIO > > filemap_check_wb_err > > errseq_check > > return -(file->f_mapping->wb_err & MAX_ERRNO) > > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > --- > > fs/fuse/file.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index f18d14d5fea1..9917bc2795e6 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, > > fl_owner_t id) > > inode_unlock(inode); > > > > err = filemap_check_errors(file->f_mapping); > > + /* return more nuanced writeback errors */ > > if (err) > > - return err; > > + return filemap_check_wb_err(file->f_mapping, 0); > > I'm wondering if this should be file_check_and_advance_wb_err() > instead. > > Is there a difference between ->flush() and ->fsync()? > > Jeff, can you please help? > Hi Miklos, We just went through this discussion for the case of NFS. The point is that ->flush() is only called on close(). While you can report errors in close(), the 'man 2 fsync' manpage documents that post-Linux 4.13, the writeback errors are required to be returned on - >fsync(). You should therefore not be calling file_check_and_advance_wb_err() in anything that is being called as part of close() since that will clear the error from the errseq_t and prevent it from being reported in a future fsync() call from a dup()ed file descriptor, etc. NFS also wants to make a special case out of write() when we know that the error is one of EDQUOT, EFBIG or ENOSPC, in which case we will also use file_check_and_advance_wb_err() to return the error immediately, and clear it from the errseq_t.
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index f18d14d5fea1..9917bc2795e6 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, fl_owner_t id) inode_unlock(inode); err = filemap_check_errors(file->f_mapping); + /* return more nuanced writeback errors */ if (err) - return err; + return filemap_check_wb_err(file->f_mapping, 0); - err = 0; if (fm->fc->no_flush) goto inval_attr_out;
As filemap_check_errors() only report -EIO or -ENOSPC, we return more nuanced writeback error -(file->f_mapping->wb_err & MAX_ERRNO). filemap_write_and_wait filemap_write_and_wait_range filemap_check_errors -ENOSPC or -EIO filemap_check_wb_err errseq_check return -(file->f_mapping->wb_err & MAX_ERRNO) Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> --- fs/fuse/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)