From: Trond Myklebust <email@example.com> To: "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com> Cc: "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com> Subject: Re: [PATCH v2 3/5] NFS: Don't report ENOSPC write errors twice Date: Tue, 12 Apr 2022 12:16:23 +0000 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> On Tue, 2022-04-12 at 14:24 +0800, chenxiaosong (A) wrote: > 在 2022/4/12 5:33, firstname.lastname@example.org 写道: > > From: Trond Myklebust <email@example.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 <firstname.lastname@example.org> > > Fixes: ce368536dd61 ("nfs: nfs_file_write() should check for > > writeback errors") > > Signed-off-by: Trond Myklebust <email@example.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 firstname.lastname@example.org
next prev parent reply other threads:[~2022-04-12 12:48 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 2022-04-12 13:13 ` chenxiaosong (A) 2022-04-12 13:29 ` chenxiaosong (A) 2022-04-12 14:25 ` chenxiaosong (A)
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v2 3/5] NFS: Don'\''t report ENOSPC write errors twice' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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.