On Mon, Aug 21, 2023 at 6:39 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Sat, Aug 19, 2023 at 05:25:35PM +0100, Filipe Manana wrote: > > On Fri, Aug 18, 2023 at 1:43 PM Zorro Lang <zlang@xxxxxxxxxx> wrote: > > > On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote: > > > > I think the url that Jens mention should be this[1] when he reviewed > > > > Stefan V7 patch for "io-uring/xfs: support async buffered writes". > > > > > > > > [1]https://lkml.kernel.org/linux-fsdevel/ca60a7dc-b16d-d8ce-f56c-547b449da8c9@xxxxxxxxx/ > > > > > > Hi Filipe, > > > > > > Does above explanation make sense to you? > > > > Not completely. > > > > It justifies that the test's assumptions are not necessarily correct, > > that I understood and it's reasonable. > > > > However I don't see, neither in that thread nor in this patch's > > changelog, why the test started to fail (on xfs, it still passes on > > btrfs and ext4) after adding support for async buffered IO writes to > > xfs and iomap - as all the writes done by the test are using direct > > IO. > > We changed how timestamps are updated so that they are aware of > IOCB_NOWAIT. If the IOCB_NOWIAT DIO write now needs to update the > inode timestamps, it will return -EAGAIN instead of doing > potentially blocking operations that require IO to complete (i.e. > taking a transaction reservation). > > Hence the first time we go to do a DIO read an inode, it's going to > do an atime update, which now occurrs from an IOCB_NOWAIT context > and we return -EAGAIN.... > > Yes, we added non-blocking timestamp updates as part of the async > buffered write support, but this was a general XFS IO path change of > behaviour to address a potential blocking point in *all* IOCB_NOWAIT > reads and writes, buffered or direct. Ok, now that's the kind of explanation I would expect in the changelog. > > > At least the changelog should point to that thread, and not the one it > > currently refers to because it doesn't provide any useful information. > > For completeness it should also justify why the async buffered write > > support broke the test, as it points out the test fails after those 2 > > commits for buffered write support, yet there are no buffered writes > > performed by the test. > > async buffered writes didn't break the test. The addition of > nonblocking timestamp updates in XFS is what causes the test to now > fail. Ok, so the changelog is very misleading, as it points to commits that, as far as I can see at least, have nothing to do with the changes that make timestamp updates aware of NOWAIT semantics. So it should be the following commit to be referred (and possibly others): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=66fa3cedf16abc82d19b943e3289c82e685419d5 > > Indeed, we may change this XFS behaviour again some day - if we can > guarantee that we can get a transaciton reservation without blocking > then we -could- allow the timestamp update to run in IOCB_NOWAIT > context. Doing this would then mean the test might randomly fail, > depending on whether the timestamp update can be done without > blocking or not.... > > Put simply, the test is not validating that RWF_NOWAIT is behaving > correctly - it just was a simple operation that kinda exercised > RWF_NOWAIT semantics when we had no other way to test this code. It > has outlived it's original purpose, so it should be removed... Thanks for the detailed explanation. A simpler version of this, or perhaps a lore link to your reply should be added to the changelog, because the current one is more like "remove this test because someone else told to do so" without any relevant details. > > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
