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

[Stefan Roesch <shr@meta.com>]

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