Re: [PATCH -next 1/2] nfs: nfs{,4}_file_flush should consume writeback error

[Trond Myklebust]

On Tue, 2022-04-12 at 22:12 +0800, chenxiaosong (A) wrote:
> 在 2022/4/12 21:56, Trond Myklebust 写道:
> > On Tue, 2022-04-12 at 21:46 +0800, chenxiaosong (A) wrote:
> > > 
> > > Other filesystem will _not_ clear writeback error on close().
> > > And other filesystem will _not_ clear writeback error on async
> > > write() too.
> > > 
> > > Other filesystem _only_ clear writeback error on fsync() or sync
> > > write().
> > > 
> > 
> > Yes. We might even consider not reporting writeback errors at all
> > in
> > close(), since most developers don't check it. We certainly don't
> > want
> > to clear those errors there because the manpages don't document
> > that as
> > being the case.
> > 
> > > Should NFS follow the same semantics as all the other
> > > filesystems?
> > 
> > It needs to follow the semantics described in the manpage for
> > write(2)
> > and fsync(2) as closely as possible, yes. That documentation is
> > supposed to be normative for application developers.
> > 
> > We won't guarantee to immediately report ENOSPC like other
> > filesystems
> > do (because that would require us to only support synchronous
> > writes),
> > however that behaviour is already documented in the manpage.
> > 
> > We may also report some errors that are not documented in the
> > manpage
> > (e.g. EACCES or EROFS) simply because those errors cannot always be
> > reported at open() time, as would be the case for a local
> > filesystem.
> > That's just how the NFS protocol works (particularly for the case
> > of
> > the stateless NFSv3 protocol).
> > 
> 
> After merging your patchset, NFS will clear wb error on async
> write(), 
> is this reasonable?
> 

It will clear ENOSPC, EDQUOT and EFBIG. It should not clear other
errors that are not supposed to be reported by write().

> And more importantly, we can not detect new error by using 
> filemap_sample_wb_err()/filemap_sample_wb_err() while
> nfs_wb_all(),just 
> as I described:
> 
> ```c
>    since = filemap_sample_wb_err() = 0
>      errseq_sample
>        if (!(old & ERRSEQ_SEEN)) // nobody see the error
>          return 0;
>    nfs_wb_all // no new error
>    error = filemap_check_wb_err(..., since) != 0 // unexpected error
> ```


As I keep repeating, that is _documented behaviour_!

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