On Mon, Jul 31, 2023 at 11:58:50AM +1000, Dave Chinner wrote: > On Thu, Jul 27, 2023 at 06:28:52PM +0200, Christian Brauner wrote: > > On Thu, Jul 27, 2023 at 05:17:30PM +0100, Pavel Begunkov wrote: > > > On 7/27/23 16:52, Christian Brauner wrote: > > > > On Thu, Jul 27, 2023 at 04:12:12PM +0100, Pavel Begunkov wrote: > > > > It would also solve it for writes which is what my kiocb_modified() > > > > comment was about. So right now you have: > > > > > > Great, I assumed there are stricter requirements for mtime not > > > transiently failing. > > > > But I mean then wouldn't this already be a problem today? > > kiocb_modified() can error out with EAGAIN today: > > > > ret = inode_needs_update_time(inode, &now); > > if (ret <= 0) > > return ret; > > if (flags & IOCB_NOWAIT) > > return -EAGAIN; > > > > return __file_update_time(file, &now, ret); > > > > the thing is that it doesn't matter for ->write_iter() - for xfs at > > least - because xfs does it as part of preparatory checks before > > actually doing any real work. The problem happens when you do actual > > work and afterwards call kiocb_modified(). That's why I think (2) is > > preferable. > > This has nothing to do with what "XFS does". It's actually an > IOCB_NOWAIT API design constraint. > > That is, IOCB_NOWAIT means "complete the whole operation without > blocking or return -EAGAIN having done nothing". If we have to do > something that might block (like a timestamp update) then we need to > punt the entire operation before anything has been modified. This > requires all the "do we need to modify this" checks to be done up > front before we start modifying anything. > > So while it looks like this might be "an XFS thing", that's because > XFS tends to be the first filesystem that most io_uring NOWAIT > functionality is implemented on. IOWs, what you see is XFS is doing > things the way IOCB_NOWAIT requires to be done. i.e. it's a > demonstration of how nonblocking filesystem modification operations > need to be run, not an "XFS thing"... Yes, I'm aware. I was trying to pay xfs a compliment for that but somehow that didn't come through. > > > > > I would prefer 2) which seems cleaner to me. But I might miss why this > > > > won't work. So input needed/wanted. > > > > > > Maybe I didn't fully grasp the (2) idea > > > > > > 2.1: all read_iter, write_iter, etc. callbacks should do file_accessed() > > > before doing IO, which sounds like a good option if everyone agrees with > > > that. Taking a look at direct block io, it's already like this. > > > > Yes, that's what I'm talking about. I'm asking whether that's ok for xfs > > maintainers basically. i_op->write_iter() already works like that since > > the dawn of time but i_op->read_iter doesn't and I'm proposing to make > > it work like that and wondering if there's any issues I'm unaware of. > > XFS already calls file_accessed() in the DIO read path before the > read gets issued. I don't see any problem with lifting it to before > the copy-out loop in filemap_read() because it is run regardless of > whether any data is read or any error occurred. Hence it just > doesn't look like it matters if it is run before or after the > copy-out loop to me.... Great.
