All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Ensure mapping errors are reported only once
@ 2022-04-11 21:33 trondmy
  2022-04-11 21:33 ` [PATCH v2 1/5] NFS: Do not report EINTR/ERESTARTSYS as mapping errors trondmy
  0 siblings, 1 reply; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

The expectation since Linux 4.13 has been that EIO errors are always
reported in fsync(), whether or not it was detected are reported
earlier.
On the other hand, ENOSPC errors are reported as soon as detected, and
should only be reported once.

Trond Myklebust (5):
  NFS: Do not report EINTR/ERESTARTSYS as mapping errors
  NFS: fsync() should report filesystem errors over EINTR/ERESTARTSYS
  NFS: Don't report ENOSPC write errors twice
  NFS: Do not report flush errors in nfs_write_end()
  NFS: Don't report errors from nfs_pageio_complete() more than once

 fs/nfs/file.c  | 49 ++++++++++++++++++++-----------------------------
 fs/nfs/write.c | 11 ++---------
 2 files changed, 22 insertions(+), 38 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/5] NFS: Do not report EINTR/ERESTARTSYS as mapping errors
  2022-04-11 21:33 [PATCH v2 0/5] Ensure mapping errors are reported only once trondmy
@ 2022-04-11 21:33 ` trondmy
  2022-04-11 21:33   ` [PATCH v2 2/5] NFS: fsync() should report filesystem errors over EINTR/ERESTARTSYS trondmy
  0 siblings, 1 reply; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If the attempt to flush data was interrupted due to a local signal, then
just requeue the writes back for I/O.

Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism with generic one")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/write.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index e864ac836237..9d3ac6edc901 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1436,7 +1436,7 @@ static void nfs_async_write_error(struct list_head *head, int error)
 	while (!list_empty(head)) {
 		req = nfs_list_entry(head->next);
 		nfs_list_remove_request(req);
-		if (nfs_error_is_fatal(error))
+		if (nfs_error_is_fatal_on_server(error))
 			nfs_write_error(req, error);
 		else
 			nfs_redirty_request(req);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/5] NFS: fsync() should report filesystem errors over EINTR/ERESTARTSYS
  2022-04-11 21:33 ` [PATCH v2 1/5] NFS: Do not report EINTR/ERESTARTSYS as mapping errors trondmy
@ 2022-04-11 21:33   ` trondmy
  2022-04-11 21:33     ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice trondmy
  0 siblings, 1 reply; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If the commit to disk is interrupted, we should still first check for
filesystem errors so that we can report them in preference to the error
due to the signal.

Fixes: 2197e9b06c22 ("NFS: Fix up fsync() when the server rebooted")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/file.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 81c80548a5c6..95e1236d95c5 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -204,15 +204,16 @@ static int
 nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct inode *inode = file_inode(file);
-	int ret;
+	int ret, ret2;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	ret = nfs_commit_inode(inode, FLUSH_SYNC);
-	if (ret < 0)
-		return ret;
-	return file_check_and_advance_wb_err(file);
+	ret2 = file_check_and_advance_wb_err(file);
+	if (ret2 < 0)
+		return ret2;
+	return ret;
 }
 
 int
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-11 21:33   ` [PATCH v2 2/5] NFS: fsync() should report filesystem errors over EINTR/ERESTARTSYS trondmy
@ 2022-04-11 21:33     ` trondmy
  2022-04-11 21:33       ` [PATCH v2 4/5] NFS: Do not report flush errors in nfs_write_end() trondmy
  2022-04-12  6:24       ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice chenxiaosong (A)
  0 siblings, 2 replies; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Any errors reported by the write() system call need to be cleared from
the file descriptor's error tracking. The current call to nfs_wb_all()
causes the error to be reported, but since it doesn't call
file_check_and_advance_wb_err(), we can end up reporting the same error
a second time when the application calls fsync().

Note that since Linux 4.13, the rule is that EIO may be reported for
write(), but it must be reported by a subsequent fsync(), so let's just
drop reporting it in write.

The check for nfs_ctx_key_to_expire() is just a duplicate to the one
already in nfs_write_end(), so let's drop that too.

Reported-by: ChenXiaoSong <chenxiaosong2@huawei.com>
Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for writeback errors")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/file.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 95e1236d95c5..8211a7aa799c 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -598,18 +598,6 @@ static const struct vm_operations_struct nfs_file_vm_ops = {
 	.page_mkwrite = nfs_vm_page_mkwrite,
 };
 
-static int nfs_need_check_write(struct file *filp, struct inode *inode,
-				int error)
-{
-	struct nfs_open_context *ctx;
-
-	ctx = nfs_file_open_context(filp);
-	if (nfs_error_is_fatal_on_server(error) ||
-	    nfs_ctx_key_to_expire(ctx, inode))
-		return 1;
-	return 0;
-}
-
 ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
@@ -637,7 +625,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 	if (iocb->ki_flags & IOCB_APPEND || iocb->ki_pos > i_size_read(inode)) {
 		result = nfs_revalidate_file_size(inode, file);
 		if (result)
-			goto out;
+			return result;
 	}
 
 	nfs_clear_invalid_mapping(file->f_mapping);
@@ -673,17 +661,22 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 	}
 	result = generic_write_sync(iocb, written);
 	if (result < 0)
-		goto out;
-
+		return result;
+out:
 	/* Return error values */
 	error = filemap_check_wb_err(file->f_mapping, since);
-	if (nfs_need_check_write(file, inode, error)) {
-		int err = nfs_wb_all(inode);
-		if (err < 0)
-			result = err;
+	switch (error) {
+	default:
+		break;
+	case -EDQUOT:
+	case -EFBIG:
+	case -ENOSPC:
+		nfs_wb_all(inode);
+		error = file_check_and_advance_wb_err(file);
+		if (error < 0)
+			result = error;
 	}
 	nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, written);
-out:
 	return result;
 
 out_swapfile:
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 4/5] NFS: Do not report flush errors in nfs_write_end()
  2022-04-11 21:33     ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice trondmy
@ 2022-04-11 21:33       ` trondmy
  2022-04-11 21:33         ` [PATCH v2 5/5] NFS: Don't report errors from nfs_pageio_complete() more than once trondmy
  2022-04-12  6:24       ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice chenxiaosong (A)
  1 sibling, 1 reply; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If we do flush cached writebacks in nfs_write_end() due to the imminent
expiration of an RPCSEC_GSS session, then we should defer reporting any
resulting errors until the calls to file_check_and_advance_wb_err() in
nfs_file_write() and nfs_file_fsync().

Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism with generic one")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/file.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 8211a7aa799c..16ddb258eef3 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -386,11 +386,8 @@ static int nfs_write_end(struct file *file, struct address_space *mapping,
 		return status;
 	NFS_I(mapping->host)->write_io += copied;
 
-	if (nfs_ctx_key_to_expire(ctx, mapping->host)) {
-		status = nfs_wb_all(mapping->host);
-		if (status < 0)
-			return status;
-	}
+	if (nfs_ctx_key_to_expire(ctx, mapping->host))
+		nfs_wb_all(mapping->host);
 
 	return copied;
 }
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 5/5] NFS: Don't report errors from nfs_pageio_complete() more than once
  2022-04-11 21:33       ` [PATCH v2 4/5] NFS: Do not report flush errors in nfs_write_end() trondmy
@ 2022-04-11 21:33         ` trondmy
  0 siblings, 0 replies; 11+ messages in thread
From: trondmy @ 2022-04-11 21:33 UTC (permalink / raw)
  To: linux-nfs; +Cc: ChenXiaoSong, Scott Mayhew

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Since errors from nfs_pageio_complete() are already being reported
through nfs_async_write_error(), we should not be returning them to the
callers of do_writepages() as well. They will end up being reported
through the generic mechanism instead.

Fixes: 6fbda89b257f ("NFS: Replace custom error reporting mechanism with generic one")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/write.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 9d3ac6edc901..0fe101e4e3db 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -677,11 +677,7 @@ static int nfs_writepage_locked(struct page *page,
 	err = nfs_do_writepage(page, wbc, &pgio);
 	pgio.pg_error = 0;
 	nfs_pageio_complete(&pgio);
-	if (err < 0)
-		return err;
-	if (nfs_error_is_fatal(pgio.pg_error))
-		return pgio.pg_error;
-	return 0;
+	return err;
 }
 
 int nfs_writepage(struct page *page, struct writeback_control *wbc)
@@ -739,9 +735,6 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 
 	if (err < 0)
 		goto out_err;
-	err = pgio.pg_error;
-	if (nfs_error_is_fatal(err))
-		goto out_err;
 	return 0;
 out_err:
 	return err;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-11 21:33     ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice trondmy
  2022-04-11 21:33       ` [PATCH v2 4/5] NFS: Do not report flush errors in nfs_write_end() trondmy
@ 2022-04-12  6:24       ` chenxiaosong (A)
  2022-04-12 12:16         ` Trond Myklebust
  1 sibling, 1 reply; 11+ messages in thread
From: chenxiaosong (A) @ 2022-04-12  6:24 UTC (permalink / raw)
  To: trondmy, linux-nfs; +Cc: Scott Mayhew, zhangxiaoxu (A), zhangyi (F)

在 2022/4/12 5:33, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Any errors reported by the write() system call need to be cleared from
> the file descriptor's error tracking. The current call to nfs_wb_all()
> causes the error to be reported, but since it doesn't call
> file_check_and_advance_wb_err(), we can end up reporting the same error
> a second time when the application calls fsync().
> 
> Note that since Linux 4.13, the rule is that EIO may be reported for
> write(), but it must be reported by a subsequent fsync(), so let's just
> drop reporting it in write.
> 
> The check for nfs_ctx_key_to_expire() is just a duplicate to the one
> already in nfs_write_end(), so let's drop that too.
> 
> Reported-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for writeback errors")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   fs/nfs/file.c | 33 +++++++++++++--------------------
>   1 file changed, 13 insertions(+), 20 deletions(-)


# 1. wb error mechanism of other filesystem

Other filesystem only clear the wb error when calling fsync(), async 
write will not clear the wb error.


# 2. still report unexpected error ... again

After merging this patchset(5 patches), second `dd` of the following 
reproducer will still report unexpected error: No space left on device.

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


# 3. my patchset

https://patchwork.kernel.org/project/linux-nfs/list/?series=628066

My patchset can fix bug of above reproducer.

filemap_sample_wb_err() always return 0 if old writeback error
have not been consumed. filemap_check_wb_err() will return the old error
even if there is no new writeback error between filemap_sample_wb_err() and
filemap_check_wb_err().

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

So we still need record writeback error in address_space flags. The 
writeback
error in address_space flags is not used to be reported to userspace, it 
is just
used to detect if there is new error while writeback.

if we want to report nuanced writeback error, it is better to detect wb 
error from filemap_check_errors(), and then return 
-(file->f_mapping->wb_err & MAX_ERRNO) to userspace without consume it.

```c
   nfs_mapping_set_error
     mapping_set_error
       __filemap_set_wb_err // record error sequence
         errseq_set
       set_bit(..., &mapping->flags) // record address_space flag

   // it is not used to be reported, just used to detect
   error = filemap_check_errors // -ENOSPC or -EIO
     test_and_clear_bit(..., &mapping->flags) // error bit cleared

   // now we try to return nuanced writeback error
   if (error)
   return filemap_check_wb_err(file->f_mapping, 0);
     return -(file->f_mapping->wb_err & MAX_ERRNO)
```

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-12  6:24       ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice chenxiaosong (A)
@ 2022-04-12 12:16         ` Trond Myklebust
  2022-04-12 13:13           ` chenxiaosong (A)
  0 siblings, 1 reply; 11+ messages in thread
From: Trond Myklebust @ 2022-04-12 12:16 UTC (permalink / raw)
  To: chenxiaosong2, linux-nfs, trondmy; +Cc: smayhew, zhangxiaoxu5, yi.zhang

On Tue, 2022-04-12 at 14:24 +0800, chenxiaosong (A) wrote:
> 在 2022/4/12 5:33, trondmy@kernel.org 写道:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Any errors reported by the write() system call need to be cleared
> > from
> > the file descriptor's error tracking. The current call to
> > nfs_wb_all()
> > causes the error to be reported, but since it doesn't call
> > file_check_and_advance_wb_err(), we can end up reporting the same
> > error
> > a second time when the application calls fsync().
> > 
> > Note that since Linux 4.13, the rule is that EIO may be reported
> > for
> > write(), but it must be reported by a subsequent fsync(), so let's
> > just
> > drop reporting it in write.
> > 
> > The check for nfs_ctx_key_to_expire() is just a duplicate to the
> > one
> > already in nfs_write_end(), so let's drop that too.
> > 
> > Reported-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> > Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for
> > writeback errors")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >   fs/nfs/file.c | 33 +++++++++++++--------------------
> >   1 file changed, 13 insertions(+), 20 deletions(-)
> 
> 
> # 1. wb error mechanism of other filesystem
> 
> Other filesystem only clear the wb error when calling fsync(), async 
> write will not clear the wb error.
> 
> 
> # 2. still report unexpected error ... again
> 
> After merging this patchset(5 patches), second `dd` of the following 
> reproducer will still report unexpected error: No space left on
> device.
> 
> Reproducer:
>          nfs server            |       nfs client
> ------------------------------|--------------------------------------
> -------
>   # No space left on server    |
>   fallocate -l 100G /svr/nospc |
>                                | mount -t nfs $nfs_server_ip:/ /mnt
>                                |
>                                | # Expected error: No space left on
> device
>                                | dd if=/dev/zero of=/mnt/file count=1
> ibs=10K
>                                |
>                                | # Release space on mountpoint
>                                | rm /mnt/nospc
>                                |
>                                | # Unexpected error: No space left on
> device
>                                | dd if=/dev/zero of=/mnt/file count=1
> ibs=10K
> 
> 
> # 3. my patchset
> 
> https://patchwork.kernel.org/project/linux-nfs/list/?series=628066
> 
> My patchset can fix bug of above reproducer.
> 
> filemap_sample_wb_err() always return 0 if old writeback error
> have not been consumed. filemap_check_wb_err() will return the old
> error
> even if there is no new writeback error between
> filemap_sample_wb_err() and
> filemap_check_wb_err().
> 
> ```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
> ```
> 
> So we still need record writeback error in address_space flags. The 
> writeback
> error in address_space flags is not used to be reported to userspace,
> it 
> is just
> used to detect if there is new error while writeback.

I understand all that. The point you appear to be missing is that this
is in fact in agreement with the documented behaviour in the write(2)
and fsync(2) manpages. These errors are supposed to be reported once,
even if they were caused by a write to a different file descriptor.

In fact, even with your change, if you make the second 'dd' call fsync
(by adding a conv=fsync), I would expect it to report the exact same
ENOSPC error, and that would be correct behaviour!

So my patches are more concerned with the fact that we appear to be
reporting the same error more than once, rather than the fact that
we're reporting them in the second attempt at I/O. As far as I'm
concerned, that is the main change that is needed to meet the behaviour
that is documented in the manpages.


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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-12 12:16         ` Trond Myklebust
@ 2022-04-12 13:13           ` chenxiaosong (A)
  2022-04-12 13:29             ` chenxiaosong (A)
  0 siblings, 1 reply; 11+ messages in thread
From: chenxiaosong (A) @ 2022-04-12 13:13 UTC (permalink / raw)
  To: Trond Myklebust, linux-nfs, trondmy; +Cc: smayhew, zhangxiaoxu5, yi.zhang

在 2022/4/12 20:16, Trond Myklebust 写道:
> I understand all that. The point you appear to be missing is that this
> is in fact in agreement with the documented behaviour in the write(2)
> and fsync(2) manpages. These errors are supposed to be reported once,
> even if they were caused by a write to a different file descriptor.
> 
> In fact, even with your change, if you make the second 'dd' call fsync
> (by adding a conv=fsync), I would expect it to report the exact same
> ENOSPC error, and that would be correct behaviour!
> 
> So my patches are more concerned with the fact that we appear to be
> reporting the same error more than once, rather than the fact that
> we're reporting them in the second attempt at I/O. As far as I'm
> concerned, that is the main change that is needed to meet the behaviour
> that is documented in the manpages.


After merging my patchset, when make the second 'dd' call fsync (by 
adding a conv=fsync), it can report ENOSPC error by calling fsync() syscall.

And when make the second 'dd' sync write (by adding a oflag=sync), it 
can report ENOSPC error too:

```c
write
   ksys_write
     vfs_write
       new_sync_write
         call_write_iter
           nfs_file_write
             generic_write_sync
               vfs_fsync_range
                 nfs_file_fsync
```


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-12 13:13           ` chenxiaosong (A)
@ 2022-04-12 13:29             ` chenxiaosong (A)
  2022-04-12 14:25               ` chenxiaosong (A)
  0 siblings, 1 reply; 11+ messages in thread
From: chenxiaosong (A) @ 2022-04-12 13:29 UTC (permalink / raw)
  To: Trond Myklebust, linux-nfs, trondmy; +Cc: smayhew, zhangxiaoxu5, yi.zhang

在 2022/4/12 21:13, chenxiaosong (A) 写道:
> 在 2022/4/12 20:16, Trond Myklebust 写道:
>> I understand all that. The point you appear to be missing is that this
>> is in fact in agreement with the documented behaviour in the write(2)
>> and fsync(2) manpages. These errors are supposed to be reported once,
>> even if they were caused by a write to a different file descriptor.
>>
>> In fact, even with your change, if you make the second 'dd' call fsync
>> (by adding a conv=fsync), I would expect it to report the exact same
>> ENOSPC error, and that would be correct behaviour!
>>
>> So my patches are more concerned with the fact that we appear to be
>> reporting the same error more than once, rather than the fact that
>> we're reporting them in the second attempt at I/O. As far as I'm
>> concerned, that is the main change that is needed to meet the behaviour
>> that is documented in the manpages.
> 
> 
> After merging my patchset, when make the second 'dd' call fsync (by 
> adding a conv=fsync), it can report ENOSPC error by calling fsync() 
> syscall.
> 
> And when make the second 'dd' sync write (by adding a oflag=sync), it 
> can report ENOSPC error too:
> 
> ```c
> write
>    ksys_write
>      vfs_write
>        new_sync_write
>          call_write_iter
>            nfs_file_write
>              generic_write_sync
>                vfs_fsync_range
>                  nfs_file_fsync
> ```
> 

On other filesystem, wb error is only cleared when doing fsync() or sync 
write(), it should not clear the wb error when doing async write(). Your 
patch will clear the wb error when doing async write().

And my patchset mainly fix following problem:

filemap_sample_wb_err() always return 0 if old writeback error
have not been consumed. filemap_check_wb_err() will return the old error
even if there is no new writeback error between filemap_sample_wb_err() and
filemap_check_wb_err().

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice
  2022-04-12 13:29             ` chenxiaosong (A)
@ 2022-04-12 14:25               ` chenxiaosong (A)
  0 siblings, 0 replies; 11+ messages in thread
From: chenxiaosong (A) @ 2022-04-12 14:25 UTC (permalink / raw)
  To: Trond Myklebust, linux-nfs, trondmy; +Cc: smayhew, zhangxiaoxu5, yi.zhang

在 2022/4/12 21:29, chenxiaosong (A) 写道:
> 在 2022/4/12 21:13, chenxiaosong (A) 写道:
> On other filesystem, wb error is only cleared when doing fsync() or sync 
> write(), it should not clear the wb error when doing async write(). Your 
> patch will clear the wb error when doing async write().
> 
> And my patchset mainly fix following problem:
> 
> filemap_sample_wb_err() always return 0 if old writeback error
> have not been consumed. filemap_check_wb_err() will return the old error
> even if there is no new writeback error between filemap_sample_wb_err() and
> filemap_check_wb_err().
> 
> ```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
> ```
> 

After merging your patchset, NFS will report and clear wb error on 
_async_ write(), even there is no new wb error while nfs_wb_all, is this 
reasonable?

And more importantly, we can not detect new error by using 
filemap_sample_wb_err()/filemap_check_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
```
.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-04-12 14:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 21:33 [PATCH v2 0/5] Ensure mapping errors are reported only once trondmy
2022-04-11 21:33 ` [PATCH v2 1/5] NFS: Do not report EINTR/ERESTARTSYS as mapping errors trondmy
2022-04-11 21:33   ` [PATCH v2 2/5] NFS: fsync() should report filesystem errors over EINTR/ERESTARTSYS trondmy
2022-04-11 21:33     ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice trondmy
2022-04-11 21:33       ` [PATCH v2 4/5] NFS: Do not report flush errors in nfs_write_end() trondmy
2022-04-11 21:33         ` [PATCH v2 5/5] NFS: Don't report errors from nfs_pageio_complete() more than once trondmy
2022-04-12  6:24       ` [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice chenxiaosong (A)
2022-04-12 12:16         ` Trond Myklebust
2022-04-12 13:13           ` chenxiaosong (A)
2022-04-12 13:29             ` chenxiaosong (A)
2022-04-12 14:25               ` chenxiaosong (A)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.