| Message ID | 1668609741-14-1-git-send-email-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Call kiocb_modified() for buffered write | expand |
On 11/16/22 6:42 AM, Xiao Yang wrote: > kiocb_modified() should be used for sync/async buffered write > because it will return -EAGAIN when IOCB_NOWAIT is set. Unfortunately, > kiocb_modified() is used by the common xfs_file_write_checks() > which is called by all types of write(i.e. buffered/direct/dax write). > This issue makes generic/471 with xfs always get the following error: > -------------------------------------------------------- > QA output created by 471 > pwrite: Resource temporarily unavailable > wrote 8388608/8388608 bytes at offset 0 > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > pwrite: Resource temporarily unavailable > ... > -------------------------------------------------------- > There have been earlier discussions about this. Snippet from the earlier discussion: "generic/471 complains because it expects any write done with RWF_NOWAIT to succeed as long as the blocks for the write are already instantiated. This isn't necessarily a correct assumption, as there are other conditions that can cause an RWF_NOWAIT write to fail with -EAGAIN even if the range is already there." So the test itself probably needs fixing. > Fixes: 1aa91d9c9933 ("xfs: Add async buffered write support") > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > fs/xfs/xfs_file.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index e462d39c840e..561fab3a49c7 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -417,6 +417,9 @@ xfs_file_write_checks( > spin_unlock(&ip->i_flags_lock); > > out: > + if (IS_DAX(inode) || (iocb->ki_flags & IOCB_DIRECT)) > + return file_modified(file); > + > return kiocb_modified(iocb); > } >
On 2022/11/17 3:00, Stefan Roesch write: > > On 11/16/22 6:42 AM, Xiao Yang wrote: >> kiocb_modified() should be used for sync/async buffered write >> because it will return -EAGAIN when IOCB_NOWAIT is set. Unfortunately, >> kiocb_modified() is used by the common xfs_file_write_checks() >> which is called by all types of write(i.e. buffered/direct/dax write). >> This issue makes generic/471 with xfs always get the following error: >> -------------------------------------------------------- >> QA output created by 471 >> pwrite: Resource temporarily unavailable >> wrote 8388608/8388608 bytes at offset 0 >> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> pwrite: Resource temporarily unavailable >> ... >> -------------------------------------------------------- >> > There have been earlier discussions about this. Snippet from the > earlier discussion: > > "generic/471 complains because it expects any write done with RWF_NOWAIT > to succeed as long as the blocks for the write are already instantiated. > This isn't necessarily a correct assumption, as there are other conditions > that can cause an RWF_NOWAIT write to fail with -EAGAIN even if the range > is already there." Hi Stefan, Thanks for your reply. Could you give me the URL about the earlier discussions? kiocb_modified() makes all types of write always get -EAGAIN when RWF_NOWAIT is set. I don't think this patch[1] is correct because it changed the original logic. The original logic only makes buffered write get -EOPNOTSUPP when RWF_NOWAIT is set. --------------------------------------------- static int file_modified_flags(struct file *file, int flags) { ... if (flags & IOCB_NOWAIT) return -EAGAIN; ... } int kiocb_modified(struct kiocb *iocb) { return file_modified_flags(iocb->ki_filp, iocb->ki_flags); } --------------------------------------------- PS: kiocb_modified() is used by the common xfs_file_write_checks() which is called by all types of write(i.e. buffered/direct/dax write). > > So the test itself probably needs fixing. In my opinion, both kernel and the test probably need to be fixed. [1] 1aa91d9c9933 ("xfs: Add async buffered write support") Best Regards, Xiao Yang
Hi Kindly ping. ^_^ Best Regards, Xiao Yang -----Original Message----- From: Yang, Xiao/杨 晓 <yangx.jy@fujitsu.com> Sent: 2022年11月17日 10:28 To: Stefan Roesch <shr@meta.com>; shr@fb.com; djwong@kernel.org Cc: linux-xfs@vger.kernel.org; Ruan, Shiyang/阮 世阳 <ruansy.fnst@fujitsu.com> Subject: Re: [PATCH] xfs: Call kiocb_modified() for buffered write On 2022/11/17 3:00, Stefan Roesch write: > > On 11/16/22 6:42 AM, Xiao Yang wrote: >> kiocb_modified() should be used for sync/async buffered write because >> it will return -EAGAIN when IOCB_NOWAIT is set. Unfortunately, >> kiocb_modified() is used by the common xfs_file_write_checks() which >> is called by all types of write(i.e. buffered/direct/dax write). >> This issue makes generic/471 with xfs always get the following error: >> -------------------------------------------------------- >> QA output created by 471 >> pwrite: Resource temporarily unavailable wrote 8388608/8388608 bytes >> at offset 0 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX >> ops/sec) >> pwrite: Resource temporarily unavailable ... >> -------------------------------------------------------- >> > There have been earlier discussions about this. Snippet from the > earlier discussion: > > "generic/471 complains because it expects any write done with > RWF_NOWAIT to succeed as long as the blocks for the write are already instantiated. > This isn't necessarily a correct assumption, as there are other > conditions that can cause an RWF_NOWAIT write to fail with -EAGAIN > even if the range is already there." Hi Stefan, Thanks for your reply. Could you give me the URL about the earlier discussions? kiocb_modified() makes all types of write always get -EAGAIN when RWF_NOWAIT is set. I don't think this patch[1] is correct because it changed the original logic. The original logic only makes buffered write get -EOPNOTSUPP when RWF_NOWAIT is set. --------------------------------------------- static int file_modified_flags(struct file *file, int flags) { ... if (flags & IOCB_NOWAIT) return -EAGAIN; ... } int kiocb_modified(struct kiocb *iocb) { return file_modified_flags(iocb->ki_filp, iocb->ki_flags); } --------------------------------------------- PS: kiocb_modified() is used by the common xfs_file_write_checks() which is called by all types of write(i.e. buffered/direct/dax write). > > So the test itself probably needs fixing. In my opinion, both kernel and the test probably need to be fixed. [1] 1aa91d9c9933 ("xfs: Add async buffered write support") Best Regards, Xiao Yang
This has been discussed in https://lore.kernel.org/linux-xfs/b2865bd6-2346-8f4d-168b-17f06bbedbed@kernel.dk/ Here is Jens comment: From: Jens Axboe <axboe@kernel.dk> To: "Darrick J. Wong" <djwong@kernel.org>, fstests <fstests@vger.kernel.org> Cc: io-uring@vger.kernel.org, kernel-team@fb.com, linux-mm@kvack.org, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, david@fromorbit.com, jack@suse.cz, hch@infradead.org, willy@infradead.org, Stefan Roesch <shr@fb.com> Subject: Re: generic/471 regression with async buffered writes? Date: Thu, 18 Aug 2022 11:00:38 -0600 [thread overview] Message-ID: <b2865bd6-2346-8f4d-168b-17f06bbedbed@kernel.dk> (raw) In-Reply-To: <Yv5quvRMZXlDXED/@magnolia> On 8/18/22 10:37 AM, Darrick J. Wong wrote: > Hi everyone, > > I noticed the following fstest failure on XFS on 6.0-rc1 that wasn't > there in 5.19: > > --- generic/471.out > +++ generic/471.out.bad > @@ -2,12 +2,10 @@ > pwrite: Resource temporarily unavailable > wrote 8388608/8388608 bytes at offset 0 > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -RWF_NOWAIT time is within limits. > +pwrite: Resource temporarily unavailable > +(standard_in) 1: syntax error > +RWF_NOWAIT took seconds > 00000000: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ > * > -00200000: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................ > -* > -00300000: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ > -* > read 8388608/8388608 bytes at offset 0 > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > Is this related to the async buffered write changes, or should I keep > looking? AFAICT nobody else has mentioned problems with 471... The test is just broken. It made some odd assumptions on what RWF_NOWAIT means with buffered writes. There's been a discussion on it previously, I'll see if I can find the links. IIRC, the tldr is that the test doesn't really tie RWF_NOWAIT to whether we'll block or not.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index e462d39c840e..561fab3a49c7 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -417,6 +417,9 @@ xfs_file_write_checks( spin_unlock(&ip->i_flags_lock); out: + if (IS_DAX(inode) || (iocb->ki_flags & IOCB_DIRECT)) + return file_modified(file); + return kiocb_modified(iocb); }
kiocb_modified() should be used for sync/async buffered write because it will return -EAGAIN when IOCB_NOWAIT is set. Unfortunately, kiocb_modified() is used by the common xfs_file_write_checks() which is called by all types of write(i.e. buffered/direct/dax write). This issue makes generic/471 with xfs always get the following error: -------------------------------------------------------- QA output created by 471 pwrite: Resource temporarily unavailable wrote 8388608/8388608 bytes at offset 0 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) pwrite: Resource temporarily unavailable ... -------------------------------------------------------- Fixes: 1aa91d9c9933 ("xfs: Add async buffered write support") Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> --- fs/xfs/xfs_file.c | 3 +++ 1 file changed, 3 insertions(+)