[PATCH -next 0/2] nfs: check writeback errors correctly

[PATCH -next 0/2] nfs: check writeback errors correctly

[ChenXiaoSong]

This series fixes nfs writeback error checking bugs.

If there is an error during the writeback process, it should be returned
when user space calls close() or fsync().

filemap_sample_wb_err() will return 0 if nobody has seen the error yet,
then filemap_check_wb_err() will return the unchanged writeback error.

ChenXiaoSong (2):
  nfs: nfs{,4}_file_flush should consume writeback error
  nfs: nfs_file_write() check writeback errors correctly

 fs/nfs/file.c     | 8 +++-----
 fs/nfs/nfs4file.c | 4 +---
 2 files changed, 4 insertions(+), 8 deletions(-)

-- 
2.31.1

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

[ChenXiaoSong]

filemap_sample_wb_err() will return 0 if nobody has seen the error yet,
then filemap_check_wb_err() will return the unchanged writeback error.

Reproducer:
        nfs server               |       nfs client
 --------------------------------|-----------------------------------------------
 # No space left on server       |
 fallocate -l 100G /server/file1 |
                                 | mount -t nfs $nfs_server_ip:/ /mnt
                                 | # Expected error: No space left on device
                                 | dd if=/dev/zero of=/mnt/file2 count=1 ibs=100K
 # Release space on server       |
 rm /server/file1                |
                                 | # Unexpected error: No space left on device
                                 | dd if=/dev/zero of=/mnt/file2 count=1 ibs=100K

Fix this by using file_check_and_advance_wb_err(). If there is an error during
the writeback process, it should be returned when user space calls close() or fsync(),
flush() is called when user space calls close().

Note that fsync() will not be called after close().

Fixes: 67dd23f9e6fb ("nfs: ensure correct writeback errors are returned on close()")
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c     | 4 +---
 fs/nfs/nfs4file.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 76d76acbc594..83d63bce9596 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -141,7 +141,6 @@ static int
 nfs_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -150,9 +149,8 @@ nfs_file_flush(struct file *file, fl_owner_t id)
 		return 0;
 
 	/* 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);
+	return file_check_and_advance_wb_err(file);
 }
 
 ssize_t
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index e79ae4cbc395..63a57e5b6db7 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -111,7 +111,6 @@ static int
 nfs4_file_flush(struct file *file, fl_owner_t id)
 {
 	struct inode	*inode = file_inode(file);
-	errseq_t since;
 
 	dprintk("NFS: flush(%pD2)\n", file);
 
@@ -127,9 +126,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 */
-	since = filemap_sample_wb_err(file->f_mapping);
 	nfs_wb_all(inode);
-	return filemap_check_wb_err(file->f_mapping, since);
+	return file_check_and_advance_wb_err(file);
 }
 
 #ifdef CONFIG_NFS_V4_2
-- 
2.31.1

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

[Trond Myklebust]

On Sat, 2022-03-05 at 20:46 +0800, ChenXiaoSong wrote:
> filemap_sample_wb_err() will return 0 if nobody has seen the error
> yet,
> then filemap_check_wb_err() will return the unchanged writeback
> error.
> 
> Reproducer:
>         nfs server               |       nfs client
>  --------------------------------|-----------------------------------
> ------------
>  # No space left on server       |
>  fallocate -l 100G /server/file1 |
>                                  | mount -t nfs $nfs_server_ip:/ /mnt
>                                  | # Expected error: No space left on
> device
>                                  | dd if=/dev/zero of=/mnt/file2
> count=1 ibs=100K
>  # Release space on server       |
>  rm /server/file1                |
>                                  | # Unexpected error: No space left
> on device
>                                  | dd if=/dev/zero of=/mnt/file2
> count=1 ibs=100K
> 

'rm' doesn't open any files or do any I/O, so it shouldn't be returning
any errors from the page cache.

IOW: The problem here is not that we're failing to clear an error from
the page cache. It is that something in 'rm' is checking the page cache
and returning any errors that it finds there.

Is 'rm' perhaps doing a stat() on the file it is deleting? If so, does
this patch fix the bug?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
it/?id=d19e0183a883

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

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

[chenxiaosong (A)]

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.


在 2022/3/6 0:53, Trond Myklebust 写道:
> 'rm' doesn't open any files or do any I/O, so it shouldn't be returning
> any errors from the page cache.
> 
> IOW: The problem here is not that we're failing to clear an error from
> the page cache. It is that something in 'rm' is checking the page cache
> and returning any errors that it finds there.
> 
> Is 'rm' perhaps doing a stat() on the file it is deleting? If so, does
> this patch fix the bug?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
> it/?id=d19e0183a883
> 

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

[Trond Myklebust]

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.



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

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

[Trond Myklebust]

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

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

[chenxiaosong (A)]

在 2022/3/6 23:08, Trond Myklebust 写道:
> 
> 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.
> 

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().

Should NFS follow the same semantics as all the other filesystems?

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

[Trond Myklebust]

On Tue, 2022-04-12 at 21:46 +0800, chenxiaosong (A) wrote:
> 在 2022/3/6 23:08, Trond Myklebust 写道:
> > 
> > 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.
> > 
> 
> 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).

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

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

[chenxiaosong (A)]

在 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?

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
```

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@hammerspace.com

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

[chenxiaosong (A)]

在 2022/4/12 22:27, Trond Myklebust 写道:

>
> It will clear ENOSPC, EDQUOT and EFBIG. It should not clear other
> errors that are not supposed to be reported by write().
> 
> As I keep repeating, that is _documented behaviour_!
> 

Hi Trond:

You may mean that 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)).

The manpage mentioned that "reported by a subsequent fsync(2)", your 
patch[1] clear the wb err on _async_ write(), and wb err will _not_ be 
reported by subsequent fsync(2), is it documented behaviour?

All other filesystems will _not_ clear any wb err on _async_ write().

[1] 
https://patchwork.kernel.org/project/linux-nfs/patch/20220411213346.762302-4-trondmy@kernel.org/

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

[chenxiaosong (A)]

在 2022/4/20 16:50, chenxiaosong (A) 写道:
> 在 2022/4/12 22:27, Trond Myklebust 写道:
> 
>>
>> It will clear ENOSPC, EDQUOT and EFBIG. It should not clear other
>> errors that are not supposed to be reported by write().
>>
>> As I keep repeating, that is _documented behaviour_!
>>
> 
> Hi Trond:
> 
> You may mean that 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)).
> 
> The manpage mentioned that "reported by a subsequent fsync(2)", your 
> patch[1] clear the wb err on _async_ write(), and wb err will _not_ be 
> reported by subsequent fsync(2), is it documented behaviour?
> 
> All other filesystems will _not_ clear any wb err on _async_ write().
> 
> [1] 
> https://patchwork.kernel.org/project/linux-nfs/patch/20220411213346.762302-4-trondmy@kernel.org/ 
> 


Hi Trond:

write(2) manpage described:

> On some filesystems, including NFS, it does not even guarantee that
> space has successfully been reserved for the data.  In this case, some
> errors might be delayed until a future write(2), fsync(2), or even
> close(2).  The only way to be sure is to call fsync(2) after you are
> done writing all your data.

Maybe it mean that: writeback errors of NFS is delayed until future sync 
write() and fsync(), because sync write() will call fsync(). We all 
agreed that close()->flush() should not clear writeback errors, should 
async write() do the same thing like other filesystems?

[PATCH -next 2/2] nfs: nfs_file_write() check writeback errors correctly

[ChenXiaoSong]

If nobody has seen the writeback error yet, then filemap_sample_wb_err()
always return 0. Even if there is no new writeback error between
filemap_sample_wb_err() and filemap_check_wb_err(),
filemap_check_wb_err() will return the old error.

Fix this by using file->f_mapping->wb_err as the old error.

Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for writeback errors")
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
 fs/nfs/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 83d63bce9596..8763f89c176a 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -635,7 +635,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 
 	nfs_clear_invalid_mapping(file->f_mapping);
 
-	since = filemap_sample_wb_err(file->f_mapping);
+	since = file->f_mapping->wb_err;
 	nfs_start_io_write(inode);
 	result = generic_write_checks(iocb, from);
 	if (result > 0) {
@@ -669,7 +669,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 
 	/* Return error values */
-	error = filemap_check_wb_err(file->f_mapping, since);
+	error = errseq_check(&file->f_mapping->wb_err, since);
 	if (nfs_need_check_write(file, inode, error)) {
 		int err = nfs_wb_all(inode);
 		if (err < 0)
-- 
2.31.1

Re: [PATCH -next 2/2] nfs: nfs_file_write() check writeback errors correctly

[Trond Myklebust]

On Sat, 2022-03-05 at 20:46 +0800, ChenXiaoSong wrote:
> If nobody has seen the writeback error yet, then
> filemap_sample_wb_err()
> always return 0. Even if there is no new writeback error between
> filemap_sample_wb_err() and filemap_check_wb_err(),
> filemap_check_wb_err() will return the old error.
> 
> Fix this by using file->f_mapping->wb_err as the old error.
> 
> Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for
> writeback errors")
> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> ---
>  fs/nfs/file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 83d63bce9596..8763f89c176a 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -635,7 +635,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct
> iov_iter *from)
>  
>         nfs_clear_invalid_mapping(file->f_mapping);
>  
> -       since = filemap_sample_wb_err(file->f_mapping);
> +       since = file->f_mapping->wb_err;
>         nfs_start_io_write(inode);
>         result = generic_write_checks(iocb, from);
>         if (result > 0) {
> @@ -669,7 +669,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct
> iov_iter *from)
>                 goto out;
>  
>         /* Return error values */
> -       error = filemap_check_wb_err(file->f_mapping, since);
> +       error = errseq_check(&file->f_mapping->wb_err, since);
>         if (nfs_need_check_write(file, inode, error)) {
>                 int err = nfs_wb_all(inode);
>                 if (err < 0)

Hmm... Why isn't this considered a bug with filemap_sample_wb_err()? If
what you say is true, then do_dentry_open() could be picking up
existing errors from the filesystem and from the inode and propagating
them to random processes.

It basically means everyone who cares about correctness of the error
return values needs to do a fsync() immediately after open() in order
to sync up the value in file->f_wb_err.

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

Re: [PATCH -next 2/2] nfs: nfs_file_write() check writeback errors correctly

[chenxiaosong (A)]

filemap_sample_wb_err() -> errseq_sample() initialise errseq_t variable 
`since`, the caller of this function will checks for an error using 
filemap_check_wb_err(since) -> errseq_check().

filemap_sample_wb_err's purpose is just sampling consumed (seen) 
writeback error to initialise errseq_t variable. I understand that 
filemap_sample_wb_err()/filemap_check_wb_err() cannot detect the new 
error between filemap_sample_wb_err() and filemap_check_wb_err().

It would be better using file->f_mapping->wb_err instead of 
filemap_sample_wb_err() in nfs_file_write() to sample wb_err at that 
time point.

In do_dentry_open(), we just sample consumed(seen) writeback error. It 
is necessary to consume the writeback error before close() over.

There is some cases that writeback error have not been consumed(seen) 
after close() file over, I think it is unexpected behavior, is this a 
bug? It is worth noting that fsync() will not be called after close() in 
nfs.

在 2022/3/6 1:12, Trond Myklebust 写道:
> Hmm... Why isn't this considered a bug with filemap_sample_wb_err()? If
> what you say is true, then do_dentry_open() could be picking up
> existing errors from the filesystem and from the inode and propagating
> them to random processes.
> 
> It basically means everyone who cares about correctness of the error
> return values needs to do a fsync() immediately after open() in order
> to sync up the value in file->f_wb_err.
>