| Message ID | 20200731174614.1299346-2-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs: two writeback error reporting fixes | expand |
On Fri, 2020-07-31 at 13:46 -0400, Scott Mayhew wrote: > nfs_wb_all() calls filemap_write_and_wait(), which uses > filemap_check_errors() to determine the error to return. > filemap_check_errors() only looks at the mapping->flags and will > therefore only return either -ENOSPC or -EIO. To ensure that the > correct error is returned on close(), nfs{,4}_file_flush() should > call > file_check_and_advance_wb_err() which looks at the errseq value in > mapping->wb_err. > > Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism > with > generic one") > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > fs/nfs/file.c | 3 ++- > fs/nfs/nfs4file.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > index f96367a2463e..eeef6580052f 100644 > --- a/fs/nfs/file.c > +++ b/fs/nfs/file.c > @@ -148,7 +148,8 @@ nfs_file_flush(struct file *file, fl_owner_t id) > return 0; > > /* Flush writes to the server and return any errors */ > - return nfs_wb_all(inode); > + nfs_wb_all(inode); > + return file_check_and_advance_wb_err(file); > } > > ssize_t > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > index 8e5d6223ddd3..77bf9c12734c 100644 > --- a/fs/nfs/nfs4file.c > +++ b/fs/nfs/nfs4file.c > @@ -125,7 +125,8 @@ 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 */ > - return nfs_wb_all(inode); > + nfs_wb_all(inode); > + return file_check_and_advance_wb_err(file); > } > > #ifdef CONFIG_NFS_V4_2 I don't think this one is correct. The contract with POSIX is that we always deliver the error on fsync(). If we call file_check_and_advance_wb_err() here in nfs_file_flush(), then that means we eat the error before it can get delivered to fsync().
On Fri, 31 Jul 2020, Trond Myklebust wrote: > On Fri, 2020-07-31 at 13:46 -0400, Scott Mayhew wrote: > > nfs_wb_all() calls filemap_write_and_wait(), which uses > > filemap_check_errors() to determine the error to return. > > filemap_check_errors() only looks at the mapping->flags and will > > therefore only return either -ENOSPC or -EIO. To ensure that the > > correct error is returned on close(), nfs{,4}_file_flush() should > > call > > file_check_and_advance_wb_err() which looks at the errseq value in > > mapping->wb_err. > > > > Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism > > with > > generic one") > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > fs/nfs/file.c | 3 ++- > > fs/nfs/nfs4file.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > > index f96367a2463e..eeef6580052f 100644 > > --- a/fs/nfs/file.c > > +++ b/fs/nfs/file.c > > @@ -148,7 +148,8 @@ nfs_file_flush(struct file *file, fl_owner_t id) > > return 0; > > > > /* Flush writes to the server and return any errors */ > > - return nfs_wb_all(inode); > > + nfs_wb_all(inode); > > + return file_check_and_advance_wb_err(file); > > } > > > > ssize_t > > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > > index 8e5d6223ddd3..77bf9c12734c 100644 > > --- a/fs/nfs/nfs4file.c > > +++ b/fs/nfs/nfs4file.c > > @@ -125,7 +125,8 @@ 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 */ > > - return nfs_wb_all(inode); > > + nfs_wb_all(inode); > > + return file_check_and_advance_wb_err(file); > > } > > > > #ifdef CONFIG_NFS_V4_2 > > I don't think this one is correct. The contract with POSIX is that we > always deliver the error on fsync(). If we call > file_check_and_advance_wb_err() here in nfs_file_flush(), then that > means we eat the error before it can get delivered to fsync(). I was looking at callers of the flush f_op and the only one I saw was filp_close(), so I assumed that there wouldn't be any other calls to fsync() for that struct file... I guess that's not the case if the file descriptor was duplicated though. Would a solution using filemap_sample_wb_err() & filemap_check_wb_err() be acceptable (like in the 2nd patch)? -Scott > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Fri, 2020-07-31 at 15:43 -0400, Scott Mayhew wrote: > On Fri, 31 Jul 2020, Trond Myklebust wrote: > > > On Fri, 2020-07-31 at 13:46 -0400, Scott Mayhew wrote: > > > nfs_wb_all() calls filemap_write_and_wait(), which uses > > > filemap_check_errors() to determine the error to return. > > > filemap_check_errors() only looks at the mapping->flags and will > > > therefore only return either -ENOSPC or -EIO. To ensure that the > > > correct error is returned on close(), nfs{,4}_file_flush() should > > > call > > > file_check_and_advance_wb_err() which looks at the errseq value > > > in > > > mapping->wb_err. > > > > > > Fixes: 6fbda89b257f ("NFS: Replace custom error reporting > > > mechanism > > > with > > > generic one") > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > --- > > > fs/nfs/file.c | 3 ++- > > > fs/nfs/nfs4file.c | 3 ++- > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > > > index f96367a2463e..eeef6580052f 100644 > > > --- a/fs/nfs/file.c > > > +++ b/fs/nfs/file.c > > > @@ -148,7 +148,8 @@ nfs_file_flush(struct file *file, fl_owner_t > > > id) > > > return 0; > > > > > > /* Flush writes to the server and return any errors */ > > > - return nfs_wb_all(inode); > > > + nfs_wb_all(inode); > > > + return file_check_and_advance_wb_err(file); > > > } > > > > > > ssize_t > > > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > > > index 8e5d6223ddd3..77bf9c12734c 100644 > > > --- a/fs/nfs/nfs4file.c > > > +++ b/fs/nfs/nfs4file.c > > > @@ -125,7 +125,8 @@ 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 */ > > > - return nfs_wb_all(inode); > > > + nfs_wb_all(inode); > > > + return file_check_and_advance_wb_err(file); > > > } > > > > > > #ifdef CONFIG_NFS_V4_2 > > > > I don't think this one is correct. The contract with POSIX is that > > we > > always deliver the error on fsync(). If we call > > file_check_and_advance_wb_err() here in nfs_file_flush(), then that > > means we eat the error before it can get delivered to fsync(). > > I was looking at callers of the flush f_op and the only one I saw was > filp_close(), so I assumed that there wouldn't be any other calls to > fsync() for that struct file... I guess that's not the case if the > file > descriptor was duplicated though. > > Would a solution using filemap_sample_wb_err() & > filemap_check_wb_err() > be acceptable (like in the 2nd patch)? > I think that would be more appropriate, yes.
diff --git a/fs/nfs/file.c b/fs/nfs/file.c index f96367a2463e..eeef6580052f 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -148,7 +148,8 @@ nfs_file_flush(struct file *file, fl_owner_t id) return 0; /* Flush writes to the server and return any errors */ - return nfs_wb_all(inode); + nfs_wb_all(inode); + return file_check_and_advance_wb_err(file); } ssize_t diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 8e5d6223ddd3..77bf9c12734c 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -125,7 +125,8 @@ 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 */ - return nfs_wb_all(inode); + nfs_wb_all(inode); + return file_check_and_advance_wb_err(file); } #ifdef CONFIG_NFS_V4_2
nfs_wb_all() calls filemap_write_and_wait(), which uses filemap_check_errors() to determine the error to return. filemap_check_errors() only looks at the mapping->flags and will therefore only return either -ENOSPC or -EIO. To ensure that the correct error is returned on close(), nfs{,4}_file_flush() should call file_check_and_advance_wb_err() which looks at the errseq value in mapping->wb_err. Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism with generic one") Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- fs/nfs/file.c | 3 ++- fs/nfs/nfs4file.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)