Re: [PATCH v3 3/3] NFS: Convert buffered read paths to use netfs when fscache is enabled

[Matthew Wilcox <willy@infradead.org>]

On Wed, Aug 31, 2022 at 02:21:23PM -0400, Jeff Layton wrote:
> > +static int nfs_netfs_init_request(struct netfs_io_request *rreq, struct file *file)
> >  {
> > -	struct netfs_cache_resources cres;
> > -	struct fscache_cookie *cookie = netfs_i_cookie(&NFS_I(inode)->netfs);
> > -	struct iov_iter iter;
> > -	struct bio_vec bvec[1];
> > -	int ret;
> > -
> > -	memset(&cres, 0, sizeof(cres));
> > -	bvec[0].bv_page		= page;
> > -	bvec[0].bv_offset	= 0;
> > -	bvec[0].bv_len		= PAGE_SIZE;
> > -	iov_iter_bvec(&iter, READ, bvec, ARRAY_SIZE(bvec), PAGE_SIZE);
> > -
> > -	ret = fscache_begin_read_operation(&cres, cookie);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = fscache_read(&cres, page_offset(page), &iter, NETFS_READ_HOLE_FAIL,
> > -			   NULL, NULL);
> > -	fscache_end_operation(&cres);
> > -	return ret;
> > +	struct nfs_open_context *ctx;
> > +
> > +	if (file == NULL) {
> > +		ctx = nfs_find_open_context(rreq->inode, NULL, FMODE_READ);
> > +		if (!ctx)
> > +			return -ENOMEM;
> 
> That error return seems like an odd choice. A NULL return here just
> means that we don't have a suitable open file, not that we're out of
> memory.
> 
> I think a NULL file pointer from netfs can only happen in readahead, and
> the comments over readahead_control say:
> 
>  * @file: The file, used primarily by network filesystems for authentication.
>  *        May be NULL if invoked internally by the filesystem.
> 
> AFAICT though, only f2fs and ext4 invoke it internally.
> 
> Maybe instead of doing this, it ought to just throw a WARN if we get a
> NULL file pointer and return -EINVAL or something?
> 
> Willy, am I correct on when ractl->file can be NULL?

Yes.  Just to quickly verify it:

$ git grep -w DEFINE_READAHEAD
fs/ext4/verity.c:       DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
fs/f2fs/file.c: DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, page_idx);
fs/f2fs/verity.c:       DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
fs/netfs/buffered_read.c:       DEFINE_READAHEAD(ractl, file, NULL, mapping, index);
fs/verity/enable.c:     DEFINE_READAHEAD(ractl, file, ra, file->f_mapping, index);
include/linux/pagemap.h:#define DEFINE_READAHEAD(ractl, f, r, m, i)                             \
include/linux/pagemap.h:        DEFINE_READAHEAD(ractl, file, ra, mapping, index);
include/linux/pagemap.h:        DEFINE_READAHEAD(ractl, file, ra, mapping, index);
mm/filemap.c:   DEFINE_READAHEAD(ractl, file, &file->f_ra, mapping, folio->index);
mm/filemap.c:   DEFINE_READAHEAD(ractl, file, ra, mapping, vmf->pgoff);
mm/filemap.c:   DEFINE_READAHEAD(ractl, file, ra, file->f_mapping, vmf->pgoff);
mm/internal.h:  DEFINE_READAHEAD(ractl, file, &file->f_ra, mapping, index);

Those two uses in pagemap.h are wrappers, so we need to check their
callers too:

$ git grep 'page_cache_\(a\)*sync_readahead'
mm/filemap.c:           page_cache_sync_readahead(mapping, ra, filp, index,
mm/khugepaged.c:                                page_cache_sync_readahead(mapping, &file->f_ra,
(ignoring the ones inside filesystems)

So yes, they all pass in a real struct file.  I wouldn't even check
whether the file pointer is NULL; just assume that it's not and the
crash will be obvious to debug.