Re: [PATCH 3/3] xfs: only iget the file once when doing vectored scrub-by-handle

Christoph Hellwig
2024-04-11T14:02:08Z

On Wed, Apr 10, 2024 at 10:21:07PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 10, 2024 at 10:02:23PM -0700, Christoph Hellwig wrote:
> > On Wed, Apr 10, 2024 at 09:56:45PM -0700, Darrick J. Wong wrote:
> > > > Well, someone needs to own it, it's just not just ext4 but could us.
> > >
> > > Er... I don't understand this? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > If we set current->journal and take a page faul we could not just
> > recurse into ext4 but into any fs including XFS. Any everyone
> > blindly dereferences is as only one fs can own it.
>
> Well back before we ripped it out I had said that XFS should just set
> current->journal to 1 to prevent memory corruption but then Jan Kara
> noted that ext4 changes its behavior wrt jbd2 if it sees nonzero
> current->journal. That's why Dave dropped it entirely.

If you are in a fs context you own current->journal_info. But you
also must make sure to not copy from and especially to user to not
recurse into another file system. A per-thread field can't work any
other way. So what ext4 is doing here is perfectly fine. What XFS
did was to set current->journal_info and then cause page faults, which
is not ok. I'm glad we fixed it.

> > That seems a bit dangerous to me. I guess we rely on the code inside
> > the transaction context to never race with unmount as lack of SB_ACTIVE
> > will make the VFS ignore the dontcache flag.
>
> That and we have an open fd to call the ioctl so any unmount will fail,
> and we can't enter scrub if unmount already starte.

Indeed.

So I'm still confused on why this new code keeps the inode around if an
error happend, but xchk_irele does not. What is the benefit of keeping
the inode around here? Why des it not apply to xchk_irele?

I also don't understand how d_mark_dontcache in
xfs_ioctl_setattr_prepare_dax is supposed to work. It'll make the inode
go away quicker than without, but it can't force the inode by itself.

I'm also lot on the interaction of that with the scrub inodes due to
both above. I'd still expect any scrub iget to set uncached for
a cache miss. If we then need to keep the inode around in transaction
context we just keep it. What is the benefit of playing racing
games with i_count to delay setting the dontcache flag until irele?
And why does the DAX mess matter for that?

Maybe I'm just thick and this is all obvious, but then it needs to
be documented in detailed comments.