On Mon, Jun 28, 2021 at 3:09 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 2021-06-28 at 13:39 -0400, Dave Wysochanski wrote:
> > Earlier commits refactored some NFS read code and removed
> > nfs_readpage_async(), but neglected to properly fixup
> > nfs_readpage_from_fscache_complete(). The code path is
> > only hit when something unusual occurs with the cachefiles
> > backing filesystem, such as an IO error or while a cookie
> > is being invalidated.
> >
> > Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx>
> > ---
> > fs/nfs/fscache.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> > index c4c021c6ebbd..d308cb7e1dd4 100644
> > --- a/fs/nfs/fscache.c
> > +++ b/fs/nfs/fscache.c
> > @@ -381,15 +381,25 @@ static void
> > nfs_readpage_from_fscache_complete(struct page *page,
> > void *context,
> > int error)
> > {
> > + struct nfs_readdesc desc;
> > + struct inode *inode = page->mapping->host;
> > +
> > dfprintk(FSCACHE,
> > "NFS: readpage_from_fscache_complete
> > (0x%p/0x%p/%d)\n",
> > page, context, error);
> >
> > - /* if the read completes with an error, we just unlock the
> > page and let
> > - * the VM reissue the readpage */
> > if (!error) {
> > SetPageUptodate(page);
> > unlock_page(page);
> > + } else {
> > + desc.ctx = context;
> > + nfs_pageio_init_read(&desc.pgio, inode, false,
> > + &nfs_async_read_completion_ops);
> > + error = readpage_async_filler(&desc, page);
> > + if (error)
> > + return;
>
> This code path can clearly fail too. Why can we not fix this code to
> allow it to return that reported error so that we can handle the
> failure case in nfs_readpage() instead of dead-ending here?
>
Because the context of the process calling nfs_readpage() is already
gone - this is called from keventd context coming up from fscache.
So if fscache fails, and then we retry here, and that fails, what is left
to be done? This _is_ the retry code. I guess I'm not following. Do you
want the process to wait inside nfs_readpage() until fscache calls
nfs_readpage_from_fscache_complete? If so, that would be new as
we have not done this before - this patch was just putting back the hunk I
mistakenly removed in commit 1e83b173b266
nfs_readpage() -> nfs_readpage_from_fscache -> fscache_read_or_alloc_page
* nfs_readpage_from_fscache_complete() will be called by fscache when
IO completes or there's an error
* fscache_read_or_alloc_page() returns 0 saying we've submitted the BIO
nfs_readpage()
...
if (!IS_SYNC(inode)) {
ret = nfs_readpage_from_fscache(desc.ctx, inode, page);
if (ret == 0)
goto out;
}
int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
struct inode *inode, struct page *page)
{
...
ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
page,
nfs_readpage_from_fscache_complete,
ctx,
GFP_KERNEL);
switch (ret) {
case 0: /* read BIO submitted (page in fscache) */
dfprintk(FSCACHE,
"NFS: readpage_from_fscache: BIO submitted\n");
nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
return ret;
> > +
> > + nfs_pageio_complete_read(&desc.pgio);
> > }
> > }
> >
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@xxxxxxxxxxxxxxx
>
>
