Re: [PATCH v7 15/15] xfs: Add async buffered write support

[Jens Axboe <axboe@kernel.dk>]

On 7/1/22 12:05 PM, Darrick J. Wong wrote:
> On Fri, Jul 01, 2022 at 08:38:07AM -0600, Jens Axboe wrote:
>> On 7/1/22 8:30 AM, Jens Axboe wrote:
>>> On 7/1/22 8:19 AM, Jens Axboe wrote:
>>>> On 6/30/22 10:39 PM, Al Viro wrote:
>>>>> On Wed, Jun 01, 2022 at 02:01:41PM -0700, Stefan Roesch wrote:
>>>>>> This adds the async buffered write support to XFS. For async buffered
>>>>>> write requests, the request will return -EAGAIN if the ilock cannot be
>>>>>> obtained immediately.
>>>>>
>>>>> breaks generic/471...
>>>>
>>>> That test case is odd, because it makes some weird assumptions about
>>>> what RWF_NOWAIT means. Most notably that it makes it mean if we should
>>>> instantiate blocks or not. Where did those assumed semantics come from?
>>>> On the read side, we have clearly documented that it should "not wait
>>>> for data which is not immediately available".
>>>>
>>>> Now it is possible that we're returning a spurious -EAGAIN here when we
>>>> should not be. And that would be a bug imho. I'll dig in and see what's
>>>> going on.
>>>
>>> This is the timestamp update that needs doing which will now return
>>> -EAGAIN if IOCB_NOWAIT is set as it may block.
>>>
>>> I do wonder if we should just allow inode time updates with IOCB_NOWAIT,
>>> even on the io_uring side. Either that, or passed in RWF_NOWAIT
>>> semantics don't map completely to internal IOCB_NOWAIT semantics. At
>>> least in terms of what generic/471 is doing, but I'm not sure who came
>>> up with that and if it's established semantics or just some made up ones
>>> from whomever wrote that test. I don't think they make any sense, to be
>>> honest.
>>
>> Further support that generic/471 is just randomly made up semantics,
>> it needs to special case btrfs with nocow or you'd get -EAGAIN anyway
>> for that test.
>>
>> And it's relying on some random timing to see if this works. I really
>> think that test case is just hot garbage, and doesn't test anything
>> meaningful.
> 
> <shrug> I had thought that NOWAIT means "don't wait for *any*thing",
> which would include timestamp updates... but then I've never been all
> that clear on what specifically NOWAIT will and won't wait for. :/

Agree, at least the read semantics (kind of) make sense, but the ones
seemingly made up by generic/471 don't seem to make any sense at all.

-- 
Jens Axboe