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

[Trond Myklebust <trondmy@hammerspace.com>]

On Sun, 2022-03-06 at 09:04 -0500, Trond Myklebust wrote:
> On Sun, 2022-03-06 at 11:54 +0800, chenxiaosong (A) wrote:
> > It would be more clear if I update the reproducer like this:
> > 
> >          nfs server                 |       nfs client
> >   --------------------------------- |------------------------------
> > --
> > -
> >   # No space left on server         |
> >   fallocate -l 100G /server/nospace |
> >                                     | mount -t nfs $nfs_server_ip:/
> > /mnt
> >                                     |
> >                                     | # Expected error
> >                                     | dd if=/dev/zero of=/mnt/file
> >                                     |
> >                                     | # Release space on mountpoint
> >                                     | rm /mnt/nospace
> >                                     |
> >                                     | # Unexpected error
> >                                     | dd if=/dev/zero of=/mnt/file
> > 
> > The Unexpected error (No space left on device) when doing second
> > `dd`, 
> > is from unconsumed writeback error after close() the file when
> > doing 
> > first `dd`. There is enough space when doing second `dd`, we should
> > not 
> > report the nospace error.
> > 
> > We should report and consume the writeback error when userspace
> > call 
> > close()->flush(), the writeback error should not be left for next
> > open().
> > 
> > Currently, fsync() will consume the writeback error while calling 
> > file_check_and_advance_wb_err(), close()->flush() should also
> > consume
> > the writeback error.
> 
> No. That's not part of the API of any other filesystem. Why should we
> make an exception for NFS?
> 
> The problem here seems to be the way that filemap_sample_wb_err() is
> defined, and how it initialises f->f_wb_err in the function
> do_dentry_open(). It is designed to do exactly what you see above.
> 

Just to clarify a little.

I don't see a need to consume the writeback errors on close(), unless
other filesystems do the same. If the intention is that fsync() should
see _all_ errors that haven't already been seen, then NFS should follow
the same semantics as all the other filesystems.

However, if that is the case (i.e. if the semantics for
filemap_sample_wb_err() are deliberate, and the intention is that
open() should behave as it does today) then we should fix the various
instances of filemap_sample_wb_err() / filemap_check_wb_err() in the
NFS and nfsd code to ignore the old errors. Their intention is
definitely to only report errors that occur in the timeframe between
the two calls.

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