Re: [linus:master] [pidfd] cb12fd8e0d: ltp.readahead01.fail

[Christian Brauner]

On Fri, Mar 15, 2024 at 04:16:33PM +0800, kernel test robot wrote:
> 
> 
> Hello,
> 
> kernel test robot noticed "ltp.readahead01.fail" on:
> 
> commit: cb12fd8e0dabb9a1c8aef55a6a41e2c255fcdf4b ("pidfd: add pidfs")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> 
> [test failed on linus/master 65d287c7eb1d14e0f4d56f19cec30d97fc7e8f66]
> [test failed on linux-next/master a1184cae56bcb96b86df3ee0377cec507a3f56e0]
> 
> in testcase: ltp
> version: ltp-x86_64-14c1f76-1_20240309
> with following parameters:
> 
> 	disk: 1HDD
> 	fs: f2fs
> 	test: syscalls-00/readahead01
> 
> 
> 
> compiler: gcc-12
> test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz (Ivy Bridge) with 8G memory

Yes, this is an expected failure.
Before moving pidfds to pidfs they were based on anonymous inodes.
Anonymous inodes have a strange property: yhey have no file type. IOW,
(stat.st_mode & S_IFMT) == 0.

The readhead code looks at the filetype and if it isn't a regular file
then you'll get EINVAL. This is the case for anonymous inode based
pidfds:

        /*
         * The readahead() syscall is intended to run only on files
         * that can execute readahead. If readahead is not possible
         * on this file, then we must return -EINVAL.
         */
        ret = -EINVAL;
        if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
            (!S_ISREG(file_inode(f.file)->i_mode) &&
            !S_ISBLK(file_inode(f.file)->i_mode)))
                goto out;

However, pidfs makes them regular files so they're not caught by that
check anymore.

However, pidfs doesn't implement any readahead support. Specifically,
it'll have sb->s_bdi == noop_backing_dev_info. Which will mean the
readahead request is just ignored:

        if (IS_DAX(inode) || (bdi == &noop_backing_dev_info)) {
                switch (advice) {
                case POSIX_FADV_NORMAL:
                case POSIX_FADV_RANDOM:
                case POSIX_FADV_SEQUENTIAL:
                case POSIX_FADV_WILLNEED:
                case POSIX_FADV_NOREUSE:
                case POSIX_FADV_DONTNEED:
                        /* no bad return value, but ignore advice */
                        break;
                default:
                        return -EINVAL;
                }
                return 0;
        }

So I'd just remove that test. It's meaningless for pseudo fses.