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.
