[PATCH] xfs: Call kiocb_modified() for buffered write

[PATCH] xfs: Call kiocb_modified() for buffered write

[Xiao Yang]

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

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);
 }
 
-- 
2.21.0

Re: [PATCH] xfs: Call kiocb_modified() for buffered write

[Stefan Roesch]

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);
>  }
>  

Re: [PATCH] xfs: Call kiocb_modified() for buffered write

[Yang, Xiao/杨 晓]

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

RE: [PATCH] xfs: Call kiocb_modified() for buffered write

[yangx.jy]

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

Re: [PATCH] xfs: Call kiocb_modified() for buffered write

[Stefan Roesch]

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.

-- 
Jens Axboe


On 1/12/23 8:55 PM, yangx.jy@fujitsu.com wrote:
> !-------------------------------------------------------------------|
>   This Message Is From an External Sender
> 
> |-------------------------------------------------------------------!
> 
> 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