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

Christoph Hellwig
2024-04-10T15:12:17Z

> + /*
> + * If the caller wants us to do a scrub-by-handle and the file used to
> + * call the ioctl is not the same file, load the incore inode and pin
> + * it across all the scrubv actions to avoid repeated UNTRUSTED
> + * lookups. The reference is not passed to deeper layers of scrub
> + * because each scrubber gets to decide its own strategy for getting an
> + * inode.
> + */
> + if (vhead->svh_ino && vhead->svh_ino != ip_in->i_ino)
> + handle_ip = xchk_scrubv_open_by_handle(mp, vhead);

Oh. So we read the inode, keep a reference to it, but still hit the
inode cache every time. A little non-onvious and not perfect for
performance, but based on your numbers probably good enough.

Curious: what is the reason the scrubbers want/need different ways to
get at the inode?

> + /*
> + * If we're holding the only reference to an inode opened via handle
> + * and the scan was clean, mark it dontcache so that we don't pollute
> + * the cache.
> + */
> + if (handle_ip) {
> + if (set_dontcache &&
> + atomic_read(&VFS_I(handle_ip)->i_count) == 1)
> + d_mark_dontcache(VFS_I(handle_ip));
> + xfs_irele(handle_ip);
> + }

This looks a little weird to me. Can't we simply use XFS_IGET_DONTCACHE
at iget time and then clear I_DONTCACHE here if we want to keep the
inode around? Given that we only set the uncached flag from
XFS_IGET_DONTCACHE on a cache miss, we won't have set
DCACHE_DONTCACHE anywhere (and don't really care about the dentries to
start with).

But why do we care about keeping the inodes with errors in memory
here, but not elsewhere? Maybe this can be explained in an expanded