[PATCH v3] vfs: fix readahead(2) on block devices

[PATCH v3] vfs: fix readahead(2) on block devices

[Reuben Hawkins]

Readahead was factored to call generic_fadvise.  That refactor added an S_ISREG
restriction which broke readahead on block devices.

This change swaps out the existing restrictions with an FMODE_LSEEK check to
fix block device readahead.

The readahead01.c and readahead02.c tests pass in ltp/testcases/...

Fixes: 3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED")
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Reuben Hawkins <reubenhwk@gmail.com>
---
 mm/readahead.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index e815c114de21..0ff6fffe3c84 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -734,8 +734,7 @@ ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
 	 * 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))
+	if (!(f.file->f_mode & FMODE_LSEEK))
 		goto out;
 
 	ret = vfs_fadvise(f.file, offset, count, POSIX_FADV_WILLNEED);
-- 
2.34.1

Re: [PATCH v3] vfs: fix readahead(2) on block devices

[Matthew Wilcox]

On Sun, Sep 24, 2023 at 12:08:46AM -0500, Reuben Hawkins wrote:
> Readahead was factored to call generic_fadvise.  That refactor added an S_ISREG
> restriction which broke readahead on block devices.
> 
> This change swaps out the existing restrictions with an FMODE_LSEEK check to
> fix block device readahead.
> 
> The readahead01.c and readahead02.c tests pass in ltp/testcases/...

I realise we could add new test cases _basically_ forever, but I'd like
to see a little more coverage in test_invalid_fd().  It currently tests
both pipes and sockets, but we have so many more fd types.  Maybe there
are good abstractions inside LTP already for creating these?  I'd
like to see tests that the following also return -EINVAL:

 - an io_uring fd
 - /dev/zero
 - /proc/self/maps (or something else in /proc we can get unprivileged
   access to)
 - a directory (debatable!  maybe we should allow prefetching a
   directory!)

This ad-hoc approach to testing syscalls is probably not the best idea.
Have the LTP considered a more thorough approach where we have a central
iterator that returns a file descriptor of various types (the ones listed
above, plus block devices, and regular files), and individual syscall
testcases can express whether this syscall should pass/fail for each type
of fd?  That would give us one central place to add new fd types, and we
wouldn't be relying on syzbot to try fds at random until something fails.

Or something.  I'm not an expert on the LTP or testing in general; it
just feels like we could do better here.

Re: [PATCH v3] vfs: fix readahead(2) on block devices

[Christian Brauner]

> This ad-hoc approach to testing syscalls is probably not the best idea.
> Have the LTP considered a more thorough approach where we have a central
> iterator that returns a file descriptor of various types (the ones listed
> above, plus block devices, and regular files), and individual syscall
> testcases can express whether this syscall should pass/fail for each type
> of fd?  That would give us one central place to add new fd types, and we
> wouldn't be relying on syzbot to try fds at random until something fails.
> 
> Or something.  I'm not an expert on the LTP or testing in general; it
> just feels like we could do better here.

I honestly would love to see such tests all go into xfstests. IOW,
general VFS and fs-specific tests should be in one location. That's why
I added src/vfs/ under xfstests. Having to run multiple test-suites for
one subsystem isn't ideal. I mean, I'm doing it but I don't love it...

Re: [PATCH v3] vfs: fix readahead(2) on block devices

[Matthew Wilcox]

On Sun, Sep 24, 2023 at 12:30:30PM +0200, Christian Brauner wrote:
> > This ad-hoc approach to testing syscalls is probably not the best idea.
> > Have the LTP considered a more thorough approach where we have a central
> > iterator that returns a file descriptor of various types (the ones listed
> > above, plus block devices, and regular files), and individual syscall
> > testcases can express whether this syscall should pass/fail for each type
> > of fd?  That would give us one central place to add new fd types, and we
> > wouldn't be relying on syzbot to try fds at random until something fails.
> > 
> > Or something.  I'm not an expert on the LTP or testing in general; it
> > just feels like we could do better here.
> 
> I honestly would love to see such tests all go into xfstests. IOW,
> general VFS and fs-specific tests should be in one location. That's why
> I added src/vfs/ under xfstests. Having to run multiple test-suites for
> one subsystem isn't ideal. I mean, I'm doing it but I don't love it...

This may well be a subject on which reasonable people can disagree.
I'm going to lay out what I believe the positions of the various
parties are here, and plese feel free to speak up if you feel I'm
mischaracterising anyone.

The LTP people see it as their mandate to test all syscalls.  They focus
on getting the correct error code, checking corner cases of the API, etc.

The xfstests people see it as their mandate to test filesystems.
They focus on testing the corner cases of the filesystem.

These are quite different things, and I'm not sure that forcing them
together or moving test-cases from one test-suite to the other makes
sense.  I think it's reasonable to have two separate test suites for you
(and the various bots) to run, even though it's slightly more work.

At the end of the day, I don't much care, it doesn't significantly affect
my life.  If I could see a clear advantage to converting one to the other,
I'd be all for it, but I don't see a compelling reason to put much work
in here.

Re: [PATCH v3] vfs: fix readahead(2) on block devices

[Cyril Hrubis]

Hi!
> I realise we could add new test cases _basically_ forever, but I'd like
> to see a little more coverage in test_invalid_fd().  It currently tests
> both pipes and sockets, but we have so many more fd types.  Maybe there
> are good abstractions inside LTP already for creating these?  I'd
> like to see tests that the following also return -EINVAL:
> 
>  - an io_uring fd
>  - /dev/zero
>  - /proc/self/maps (or something else in /proc we can get unprivileged
>    access to)
>  - a directory (debatable!  maybe we should allow prefetching a
>    directory!)

This sounds like a good idea. We do have an API to iterate over
filesystems but not API to iterate over file descriptors, I suppose that
we will need an enum with fd type passed along with the file descriptor
so that we can set the expectations right and then just define a
function that would take the structure and do the test, something as:

enum tst_fd_type {
	TST_FD_IO_URING,
	TST_FD_DEV_ZERO,
	TST_FD_PROC_MAPS,
	...
};

struct tst_fd {
	enum tst_fd_type type;
	int fd;
};

static void test_fd(struct tst_fd *fd)
{
	if (fd->type == TST_FD...)
		TST_EXP_PASS(...);
	else
		TST_EXP_FAIL(...);
}

I can add something like this once we are done with the September LTP
release.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH v3] vfs: fix readahead(2) on block devices

[Christian Brauner]

On Sun, Sep 24, 2023 at 03:35:18PM +0100, Matthew Wilcox wrote:
> On Sun, Sep 24, 2023 at 12:30:30PM +0200, Christian Brauner wrote:
> > > This ad-hoc approach to testing syscalls is probably not the best idea.
> > > Have the LTP considered a more thorough approach where we have a central
> > > iterator that returns a file descriptor of various types (the ones listed
> > > above, plus block devices, and regular files), and individual syscall
> > > testcases can express whether this syscall should pass/fail for each type
> > > of fd?  That would give us one central place to add new fd types, and we
> > > wouldn't be relying on syzbot to try fds at random until something fails.
> > > 
> > > Or something.  I'm not an expert on the LTP or testing in general; it
> > > just feels like we could do better here.
> > 
> > I honestly would love to see such tests all go into xfstests. IOW,
> > general VFS and fs-specific tests should be in one location. That's why
> > I added src/vfs/ under xfstests. Having to run multiple test-suites for
> > one subsystem isn't ideal. I mean, I'm doing it but I don't love it...
> 
> This may well be a subject on which reasonable people can disagree.

Oh it sure is.

> I'm going to lay out what I believe the positions of the various
> parties are here, and plese feel free to speak up if you feel I'm
> mischaracterising anyone.
> 
> The LTP people see it as their mandate to test all syscalls.  They focus
> on getting the correct error code, checking corner cases of the API, etc.
> 
> The xfstests people see it as their mandate to test filesystems.
> They focus on testing the corner cases of the filesystem.
> 
> These are quite different things, and I'm not sure that forcing them

Yes, I agree that they are different things.
But the lines are quite often rather blurry as most vfs changes have an
immediate impact on fs testing and testing them involves validating the
vfs interface and the fses implementing them.

I've mostly just described my ideal world where the two things would be
tightly coupled even during testing.