2021-10-05 12:53:35

by David Howells

Subject: Re: [PATCH v1 1/7] NFS: Fixup patch 3/8 of fscache-iter-3 v2

Dave Wysochanski <[email protected]> wrote:

>
> Handle failed return values of fscache_fallback_read_page() in
> switch statement.

After some discussion on IRC, the attached is probably better. Returning 1
might result in 1 being returned through ->readpage(), which could be a
problem.

David
---
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 5b0e78742444..68e266a37675 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -346,33 +346,18 @@ int __nfs_readpage_from_fscache(struct inode *inode, struct page *page)

ret = fscache_fallback_read_page(nfs_i_fscache(inode), page);
if (ret < 0) {
- dfprintk(FSCACHE, "NFS: readpage_from_fscache: "
- "fscache_fallback_read_page failed ret = %d\n", ret);
- return ret;
- }
-
- switch (ret) {
- case 0: /* Read completed synchronously */
- dfprintk(FSCACHE,
- "NFS: readpage_from_fscache: read successful\n");
- nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
- SetPageUptodate(page);
- return 0;
-
- case -ENOBUFS: /* inode not in cache */
- case -ENODATA: /* page not in cache */
nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
dfprintk(FSCACHE,
- "NFS: readpage_from_fscache %d\n", ret);
- SetPageChecked(page);
- return 1;
-
- default:
- dfprintk(FSCACHE, "NFS: readpage_from_fscache %d\n", ret);
- nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
+ "NFS: readpage_from_fscache failed %d\n", ret);
SetPageChecked(page);
+ return ret;
}
- return ret;
+
+ /* Read completed synchronously */
+ dfprintk(FSCACHE, "NFS: readpage_from_fscache: read successful\n");
+ nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
+ SetPageUptodate(page);
+ return 0;
}

/*

2021-10-05 13:26:04

by David Wysochanski

Subject: Re: [PATCH v1 1/7] NFS: Fixup patch 3/8 of fscache-iter-3 v2

On Tue, Oct 5, 2021 at 8:52 AM David Howells <[email protected]> wrote:
>
> Dave Wysochanski <[email protected]> wrote:
>
> >
> > Handle failed return values of fscache_fallback_read_page() in
> > switch statement.
>
> After some discussion on IRC, the attached is probably better. Returning 1
> might result in 1 being returned through ->readpage(), which could be a
> problem.
>
> David
> ---
> diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> index 5b0e78742444..68e266a37675 100644
> --- a/fs/nfs/fscache.c
> +++ b/fs/nfs/fscache.c
> @@ -346,33 +346,18 @@ int __nfs_readpage_from_fscache(struct inode *inode, struct page *page)
>
> ret = fscache_fallback_read_page(nfs_i_fscache(inode), page);
> if (ret < 0) {
> - dfprintk(FSCACHE, "NFS: readpage_from_fscache: "
> - "fscache_fallback_read_page failed ret = %d\n", ret);
> - return ret;
> - }
> -
> - switch (ret) {
> - case 0: /* Read completed synchronously */
> - dfprintk(FSCACHE,
> - "NFS: readpage_from_fscache: read successful\n");
> - nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
> - SetPageUptodate(page);
> - return 0;
> -
> - case -ENOBUFS: /* inode not in cache */
> - case -ENODATA: /* page not in cache */
> nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
> dfprintk(FSCACHE,
> - "NFS: readpage_from_fscache %d\n", ret);
> - SetPageChecked(page);
> - return 1;
> -
> - default:
> - dfprintk(FSCACHE, "NFS: readpage_from_fscache %d\n", ret);
> - nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
> + "NFS: readpage_from_fscache failed %d\n", ret);
> SetPageChecked(page);
> + return ret;
> }
> - return ret;
> +
> + /* Read completed synchronously */
> + dfprintk(FSCACHE, "NFS: readpage_from_fscache: read successful\n");
> + nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
> + SetPageUptodate(page);
> + return 0;
> }
>
> /*
>

Yes this looks good.
Acked-by: Dave Wysochanski <[email protected]>