On Thu, Jul 27, 2023 at 04:27:30PM +0200, Christian Brauner wrote:
> On Thu, Jul 27, 2023 at 07:51:19PM +0800, Hao Xu wrote:
> > I actually saw this semaphore, and there is another xfs lock in
> > file_accessed
> > --> touch_atime
> > --> inode_update_time
> > --> inode->i_op->update_time == xfs_vn_update_time
> >
> > Forgot to point them out in the cover-letter..., I didn't modify them
> > since I'm not very sure about if we should do so, and I saw Stefan's
> > patchset didn't modify them too.
> >
> > My personnal thinking is we should apply trylock logic for this
> > inode->i_rwsem. For xfs lock in touch_atime, we should do that since it
> > doesn't make sense to rollback all the stuff while we are almost at the
> > end of getdents because of a lock.
>
> That manoeuvres around the problem. Which I'm slightly more sensitive
> too as this review is a rather expensive one.
>
> Plus, it seems fixable in at least two ways:
>
> For both we need to be able to tell the filesystem that a nowait atime
> update is requested. Simple thing seems to me to add a S_NOWAIT flag to
> file_time_flags and passing that via i_op->update_time() which already
> has a flag argument. That would likely also help kiocb_modified().
Wait - didn't we already fix this for mtime updates on IOCB_NOWAIT
modification operations? Yeah, we did:
kiocb_modified(iocb)
file_modified_flags(iocb->ki_file, iocb->ki_flags)
....
ret = inode_needs_update_time()
if (ret <= 0)
return ret;
if (flags & IOCB_NOWAIT)
return -EAGAIN;
<does timestamp update>
> file_accessed()
> -> touch_atime()
> -> inode_update_time()
> -> i_op->update_time == xfs_vn_update_time()
Yeah, so this needs the same treatment as file_modified_flags() -
touch_atime() needs a flag variant that passes IOCB_NOWAIT, and
after atime_needs_update() returns trues we should check IOCB_NOWAIT
and return EAGAIN if it is set. That will punt the operation that
needs to the update to a worker thread that can block....
> Then we have two options afaict:
>
> (1) best-effort atime update
>
> file_accessed() already has the builtin assumption that updating atime
> might fail for other reasons - see the comment in there. So it is
> somewhat best-effort already.
>
> (2) move atime update before calling into filesystem
>
> If we want to be sure that access time is updated when a readdir request
> is issued through io_uring then we need to have file_accessed() give a
> return value and expose a new helper for io_uring or modify
> vfs_getdents() to do something like:
>
> vfs_getdents()
> {
> if (nowait)
> down_read_trylock()
>
> if (!IS_DEADDIR(inode)) {
> ret = file_accessed(file);
> if (ret == -EAGAIN)
> goto out_unlock;
>
> f_op->iterate_shared()
> }
> }
Yup, that's the sort of thing that needs to be done.
But as I said in the "llseek for io-uring" thread, we need to stop
the game of whack-a-mole passing random nowait boolean flags to VFS
operations before it starts in earnest. We really need a common
context structure (like we have a kiocb for IO operations) that
holds per operation control state so we have consistency across all
the operations that we need different behaviours for.
> It's not unprecedented to do update atime before the actual operation
> has been done afaict. That's already the case in xfs_file_write_checks()
> which is called before anything is written. So that seems ok.
Writes don't update atime - they update mtime, and there are other
considerations for doing the mtime update before the data
modification. e.g. we have to strip permissions from the inode prior
to any changes that are going to be made, so we've already got to do
all the "change inode metadata" checks and modifications before the
data is written anyway....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
