Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 Aug 2022 19:58:20 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Dave Wysochanski <dwysocha@redhat.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        David Howells <dhowells@redhat.com>, linux-nfs@vger.kernel.org,
        linux-cachefs@redhat.com, Benjamin Maynard <benmaynard@google.com>,
        Daire Byrne <daire.byrne@gmail.com>
Subject: Re: [PATCH v3 3/3] NFS: Convert buffered read paths to use netfs
 when fscache is enabled
Message-ID: <Yw+vTFdk4gAoNR27@casper.infradead.org>
References: <20220831005053.1287363-1-dwysocha@redhat.com>
 <20220831005053.1287363-4-dwysocha@redhat.com>
 <2c4f4fae20c702e805162f7fa780fc09f7f05aaa.camel@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2c4f4fae20c702e805162f7fa780fc09f7f05aaa.camel@kernel.org>
Precedence: bulk

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.