| Message ID | 20221017105212.77588-2-dwysocha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert NFS with fscache to the netfs API | expand |
On Mon, 2022-10-17 at 06:52 -0400, Dave Wysochanski wrote: > Rename readpage_async_filler to nfs_pageio_add_page to > better reflect what this function does (add a page to > the nfs_pageio_descriptor), and simplify arguments to > this function by removing struct nfs_readdesc. > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > Reviewed-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfs/read.c | 60 +++++++++++++++++++++++++------------------------ > -- > 1 file changed, 30 insertions(+), 30 deletions(-) > > diff --git a/fs/nfs/read.c b/fs/nfs/read.c > index 8ae2c8d1219d..525e82ea9a9e 100644 > --- a/fs/nfs/read.c > +++ b/fs/nfs/read.c > @@ -127,11 +127,6 @@ static void nfs_readpage_release(struct nfs_page > *req, int error) > nfs_release_request(req); > } > > -struct nfs_readdesc { > - struct nfs_pageio_descriptor pgio; > - struct nfs_open_context *ctx; > -}; > - > static void nfs_page_group_set_uptodate(struct nfs_page *req) > { > if (nfs_page_group_sync_on_bit(req, PG_UPTODATE)) > @@ -153,7 +148,8 @@ static void nfs_read_completion(struct > nfs_pgio_header *hdr) > > if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) { > /* note: regions of the page not covered by a > - * request are zeroed in > readpage_async_filler */ > + * request are zeroed in nfs_pageio_add_page > + */ > if (bytes > hdr->good_bytes) { > /* nothing in this request was good, > so zero > * the full extent of the request */ > @@ -281,8 +277,10 @@ static void nfs_readpage_result(struct rpc_task > *task, > nfs_readpage_retry(task, hdr); > } > > -static int > -readpage_async_filler(struct nfs_readdesc *desc, struct page *page) > +int > +nfs_pageio_add_page(struct nfs_pageio_descriptor *pgio, > + struct nfs_open_context *ctx, > + struct page *page) If we're going to rename this function, then let's not give it a name that suggests it belongs in pagelist.c. It's not a generic helper function, but is still very much specific to the pagecache read functionality. > { > struct inode *inode = page_file_mapping(page)->host; > unsigned int rsize = NFS_SERVER(inode)->rsize; > @@ -302,15 +300,15 @@ readpage_async_filler(struct nfs_readdesc > *desc, struct page *page) > goto out_unlock; > } > > - new = nfs_create_request(desc->ctx, page, 0, aligned_len); > + new = nfs_create_request(ctx, page, 0, aligned_len); > if (IS_ERR(new)) > goto out_error; > > if (len < PAGE_SIZE) > zero_user_segment(page, len, PAGE_SIZE); > - if (!nfs_pageio_add_request(&desc->pgio, new)) { > + if (!nfs_pageio_add_request(pgio, new)) { > nfs_list_remove_request(new); > - error = desc->pgio.pg_error; > + error = pgio->pg_error; > nfs_readpage_release(new, error); > goto out; > } > @@ -332,7 +330,8 @@ readpage_async_filler(struct nfs_readdesc *desc, > struct page *page) > int nfs_read_folio(struct file *file, struct folio *folio) > { > struct page *page = &folio->page; > - struct nfs_readdesc desc; > + struct nfs_pageio_descriptor pgio; > + struct nfs_open_context *ctx; > struct inode *inode = page_file_mapping(page)->host; > int ret; > > @@ -358,29 +357,29 @@ int nfs_read_folio(struct file *file, struct > folio *folio) > > if (file == NULL) { > ret = -EBADF; > - desc.ctx = nfs_find_open_context(inode, NULL, > FMODE_READ); > - if (desc.ctx == NULL) > + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); > + if (ctx == NULL) > goto out_unlock; > } else > - desc.ctx = > get_nfs_open_context(nfs_file_open_context(file)); > + ctx = > get_nfs_open_context(nfs_file_open_context(file)); > > - xchg(&desc.ctx->error, 0); > - nfs_pageio_init_read(&desc.pgio, inode, false, > + xchg(&ctx->error, 0); > + nfs_pageio_init_read(&pgio, inode, false, > &nfs_async_read_completion_ops); > > - ret = readpage_async_filler(&desc, page); > + ret = nfs_pageio_add_page(&pgio, ctx, page); > if (ret) > goto out; > > - nfs_pageio_complete_read(&desc.pgio); > - ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0; > + nfs_pageio_complete_read(&pgio); > + ret = pgio.pg_error < 0 ? pgio.pg_error : 0; > if (!ret) { > ret = wait_on_page_locked_killable(page); > if (!PageUptodate(page) && !ret) > - ret = xchg(&desc.ctx->error, 0); > + ret = xchg(&ctx->error, 0); > } > out: > - put_nfs_open_context(desc.ctx); > + put_nfs_open_context(ctx); > trace_nfs_aop_readpage_done(inode, page, ret); > return ret; > out_unlock: > @@ -391,9 +390,10 @@ int nfs_read_folio(struct file *file, struct > folio *folio) > > void nfs_readahead(struct readahead_control *ractl) > { > + struct nfs_pageio_descriptor pgio; > + struct nfs_open_context *ctx; > unsigned int nr_pages = readahead_count(ractl); > struct file *file = ractl->file; > - struct nfs_readdesc desc; > struct inode *inode = ractl->mapping->host; > struct page *page; > int ret; > @@ -407,25 +407,25 @@ void nfs_readahead(struct readahead_control > *ractl) > > if (file == NULL) { > ret = -EBADF; > - desc.ctx = nfs_find_open_context(inode, NULL, > FMODE_READ); > - if (desc.ctx == NULL) > + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); > + if (ctx == NULL) > goto out; > } else > - desc.ctx = > get_nfs_open_context(nfs_file_open_context(file)); > + ctx = > get_nfs_open_context(nfs_file_open_context(file)); > > - nfs_pageio_init_read(&desc.pgio, inode, false, > + nfs_pageio_init_read(&pgio, inode, false, > &nfs_async_read_completion_ops); > > while ((page = readahead_page(ractl)) != NULL) { > - ret = readpage_async_filler(&desc, page); > + ret = nfs_pageio_add_page(&pgio, ctx, page); > put_page(page); > if (ret) > break; > } > > - nfs_pageio_complete_read(&desc.pgio); > + nfs_pageio_complete_read(&pgio); > > - put_nfs_open_context(desc.ctx); > + put_nfs_open_context(ctx); > out: > trace_nfs_aop_readahead_done(inode, nr_pages, ret); > }
On Thu, Oct 27, 2022 at 2:07 PM Trond Myklebust <trondmy@kernel.org> wrote: > > On Mon, 2022-10-17 at 06:52 -0400, Dave Wysochanski wrote: > > Rename readpage_async_filler to nfs_pageio_add_page to > > better reflect what this function does (add a page to > > the nfs_pageio_descriptor), and simplify arguments to > > this function by removing struct nfs_readdesc. > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfs/read.c | 60 +++++++++++++++++++++++++------------------------ > > -- > > 1 file changed, 30 insertions(+), 30 deletions(-) > > > > diff --git a/fs/nfs/read.c b/fs/nfs/read.c > > index 8ae2c8d1219d..525e82ea9a9e 100644 > > --- a/fs/nfs/read.c > > +++ b/fs/nfs/read.c > > @@ -127,11 +127,6 @@ static void nfs_readpage_release(struct nfs_page > > *req, int error) > > nfs_release_request(req); > > } > > > > -struct nfs_readdesc { > > - struct nfs_pageio_descriptor pgio; > > - struct nfs_open_context *ctx; > > -}; > > - > > static void nfs_page_group_set_uptodate(struct nfs_page *req) > > { > > if (nfs_page_group_sync_on_bit(req, PG_UPTODATE)) > > @@ -153,7 +148,8 @@ static void nfs_read_completion(struct > > nfs_pgio_header *hdr) > > > > if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) { > > /* note: regions of the page not covered by a > > - * request are zeroed in > > readpage_async_filler */ > > + * request are zeroed in nfs_pageio_add_page > > + */ > > if (bytes > hdr->good_bytes) { > > /* nothing in this request was good, > > so zero > > * the full extent of the request */ > > @@ -281,8 +277,10 @@ static void nfs_readpage_result(struct rpc_task > > *task, > > nfs_readpage_retry(task, hdr); > > } > > > > -static int > > -readpage_async_filler(struct nfs_readdesc *desc, struct page *page) > > +int > > +nfs_pageio_add_page(struct nfs_pageio_descriptor *pgio, > > + struct nfs_open_context *ctx, > > + struct page *page) > > If we're going to rename this function, then let's not give it a name > that suggests it belongs in pagelist.c. It's not a generic helper > function, but is still very much specific to the pagecache read > functionality. > How about nfs_read_add_page()? > > { > > struct inode *inode = page_file_mapping(page)->host; > > unsigned int rsize = NFS_SERVER(inode)->rsize; > > @@ -302,15 +300,15 @@ readpage_async_filler(struct nfs_readdesc > > *desc, struct page *page) > > goto out_unlock; > > } > > > > - new = nfs_create_request(desc->ctx, page, 0, aligned_len); > > + new = nfs_create_request(ctx, page, 0, aligned_len); > > if (IS_ERR(new)) > > goto out_error; > > > > if (len < PAGE_SIZE) > > zero_user_segment(page, len, PAGE_SIZE); > > - if (!nfs_pageio_add_request(&desc->pgio, new)) { > > + if (!nfs_pageio_add_request(pgio, new)) { > > nfs_list_remove_request(new); > > - error = desc->pgio.pg_error; > > + error = pgio->pg_error; > > nfs_readpage_release(new, error); > > goto out; > > } > > @@ -332,7 +330,8 @@ readpage_async_filler(struct nfs_readdesc *desc, > > struct page *page) > > int nfs_read_folio(struct file *file, struct folio *folio) > > { > > struct page *page = &folio->page; > > - struct nfs_readdesc desc; > > + struct nfs_pageio_descriptor pgio; > > + struct nfs_open_context *ctx; > > struct inode *inode = page_file_mapping(page)->host; > > int ret; > > > > @@ -358,29 +357,29 @@ int nfs_read_folio(struct file *file, struct > > folio *folio) > > > > if (file == NULL) { > > ret = -EBADF; > > - desc.ctx = nfs_find_open_context(inode, NULL, > > FMODE_READ); > > - if (desc.ctx == NULL) > > + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); > > + if (ctx == NULL) > > goto out_unlock; > > } else > > - desc.ctx = > > get_nfs_open_context(nfs_file_open_context(file)); > > + ctx = > > get_nfs_open_context(nfs_file_open_context(file)); > > > > - xchg(&desc.ctx->error, 0); > > - nfs_pageio_init_read(&desc.pgio, inode, false, > > + xchg(&ctx->error, 0); > > + nfs_pageio_init_read(&pgio, inode, false, > > &nfs_async_read_completion_ops); > > > > - ret = readpage_async_filler(&desc, page); > > + ret = nfs_pageio_add_page(&pgio, ctx, page); > > if (ret) > > goto out; > > > > - nfs_pageio_complete_read(&desc.pgio); > > - ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0; > > + nfs_pageio_complete_read(&pgio); > > + ret = pgio.pg_error < 0 ? pgio.pg_error : 0; > > if (!ret) { > > ret = wait_on_page_locked_killable(page); > > if (!PageUptodate(page) && !ret) > > - ret = xchg(&desc.ctx->error, 0); > > + ret = xchg(&ctx->error, 0); > > } > > out: > > - put_nfs_open_context(desc.ctx); > > + put_nfs_open_context(ctx); > > trace_nfs_aop_readpage_done(inode, page, ret); > > return ret; > > out_unlock: > > @@ -391,9 +390,10 @@ int nfs_read_folio(struct file *file, struct > > folio *folio) > > > > void nfs_readahead(struct readahead_control *ractl) > > { > > + struct nfs_pageio_descriptor pgio; > > + struct nfs_open_context *ctx; > > unsigned int nr_pages = readahead_count(ractl); > > struct file *file = ractl->file; > > - struct nfs_readdesc desc; > > struct inode *inode = ractl->mapping->host; > > struct page *page; > > int ret; > > @@ -407,25 +407,25 @@ void nfs_readahead(struct readahead_control > > *ractl) > > > > if (file == NULL) { > > ret = -EBADF; > > - desc.ctx = nfs_find_open_context(inode, NULL, > > FMODE_READ); > > - if (desc.ctx == NULL) > > + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); > > + if (ctx == NULL) > > goto out; > > } else > > - desc.ctx = > > get_nfs_open_context(nfs_file_open_context(file)); > > + ctx = > > get_nfs_open_context(nfs_file_open_context(file)); > > > > - nfs_pageio_init_read(&desc.pgio, inode, false, > > + nfs_pageio_init_read(&pgio, inode, false, > > &nfs_async_read_completion_ops); > > > > while ((page = readahead_page(ractl)) != NULL) { > > - ret = readpage_async_filler(&desc, page); > > + ret = nfs_pageio_add_page(&pgio, ctx, page); > > put_page(page); > > if (ret) > > break; > > } > > > > - nfs_pageio_complete_read(&desc.pgio); > > + nfs_pageio_complete_read(&pgio); > > > > - put_nfs_open_context(desc.ctx); > > + put_nfs_open_context(ctx); > > out: > > trace_nfs_aop_readahead_done(inode, nr_pages, ret); > > } > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Fri, 2022-10-28 at 06:32 -0400, David Wysochanski wrote: > On Thu, Oct 27, 2022 at 2:07 PM Trond Myklebust <trondmy@kernel.org> > wrote: > > > > On Mon, 2022-10-17 at 06:52 -0400, Dave Wysochanski wrote: > > > Rename readpage_async_filler to nfs_pageio_add_page to > > > better reflect what this function does (add a page to > > > the nfs_pageio_descriptor), and simplify arguments to > > > this function by removing struct nfs_readdesc. > > > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfs/read.c | 60 +++++++++++++++++++++++++-------------------- > > > ---- > > > -- > > > 1 file changed, 30 insertions(+), 30 deletions(-) > > > > > > diff --git a/fs/nfs/read.c b/fs/nfs/read.c > > > index 8ae2c8d1219d..525e82ea9a9e 100644 > > > --- a/fs/nfs/read.c > > > +++ b/fs/nfs/read.c > > > @@ -127,11 +127,6 @@ static void nfs_readpage_release(struct > > > nfs_page > > > *req, int error) > > > nfs_release_request(req); > > > } > > > > > > -struct nfs_readdesc { > > > - struct nfs_pageio_descriptor pgio; > > > - struct nfs_open_context *ctx; > > > -}; > > > - > > > static void nfs_page_group_set_uptodate(struct nfs_page *req) > > > { > > > if (nfs_page_group_sync_on_bit(req, PG_UPTODATE)) > > > @@ -153,7 +148,8 @@ static void nfs_read_completion(struct > > > nfs_pgio_header *hdr) > > > > > > if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) { > > > /* note: regions of the page not covered > > > by a > > > - * request are zeroed in > > > readpage_async_filler */ > > > + * request are zeroed in > > > nfs_pageio_add_page > > > + */ > > > if (bytes > hdr->good_bytes) { > > > /* nothing in this request was > > > good, > > > so zero > > > * the full extent of the request > > > */ > > > @@ -281,8 +277,10 @@ static void nfs_readpage_result(struct > > > rpc_task > > > *task, > > > nfs_readpage_retry(task, hdr); > > > } > > > > > > -static int > > > -readpage_async_filler(struct nfs_readdesc *desc, struct page > > > *page) > > > +int > > > +nfs_pageio_add_page(struct nfs_pageio_descriptor *pgio, > > > + struct nfs_open_context *ctx, > > > + struct page *page) > > > > If we're going to rename this function, then let's not give it a > > name > > that suggests it belongs in pagelist.c. It's not a generic helper > > function, but is still very much specific to the pagecache read > > functionality. > > > > How about nfs_read_add_page()? > > That's better.
diff --git a/fs/nfs/read.c b/fs/nfs/read.c index 8ae2c8d1219d..525e82ea9a9e 100644 --- a/fs/nfs/read.c +++ b/fs/nfs/read.c @@ -127,11 +127,6 @@ static void nfs_readpage_release(struct nfs_page *req, int error) nfs_release_request(req); } -struct nfs_readdesc { - struct nfs_pageio_descriptor pgio; - struct nfs_open_context *ctx; -}; - static void nfs_page_group_set_uptodate(struct nfs_page *req) { if (nfs_page_group_sync_on_bit(req, PG_UPTODATE)) @@ -153,7 +148,8 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr) if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) { /* note: regions of the page not covered by a - * request are zeroed in readpage_async_filler */ + * request are zeroed in nfs_pageio_add_page + */ if (bytes > hdr->good_bytes) { /* nothing in this request was good, so zero * the full extent of the request */ @@ -281,8 +277,10 @@ static void nfs_readpage_result(struct rpc_task *task, nfs_readpage_retry(task, hdr); } -static int -readpage_async_filler(struct nfs_readdesc *desc, struct page *page) +int +nfs_pageio_add_page(struct nfs_pageio_descriptor *pgio, + struct nfs_open_context *ctx, + struct page *page) { struct inode *inode = page_file_mapping(page)->host; unsigned int rsize = NFS_SERVER(inode)->rsize; @@ -302,15 +300,15 @@ readpage_async_filler(struct nfs_readdesc *desc, struct page *page) goto out_unlock; } - new = nfs_create_request(desc->ctx, page, 0, aligned_len); + new = nfs_create_request(ctx, page, 0, aligned_len); if (IS_ERR(new)) goto out_error; if (len < PAGE_SIZE) zero_user_segment(page, len, PAGE_SIZE); - if (!nfs_pageio_add_request(&desc->pgio, new)) { + if (!nfs_pageio_add_request(pgio, new)) { nfs_list_remove_request(new); - error = desc->pgio.pg_error; + error = pgio->pg_error; nfs_readpage_release(new, error); goto out; } @@ -332,7 +330,8 @@ readpage_async_filler(struct nfs_readdesc *desc, struct page *page) int nfs_read_folio(struct file *file, struct folio *folio) { struct page *page = &folio->page; - struct nfs_readdesc desc; + struct nfs_pageio_descriptor pgio; + struct nfs_open_context *ctx; struct inode *inode = page_file_mapping(page)->host; int ret; @@ -358,29 +357,29 @@ int nfs_read_folio(struct file *file, struct folio *folio) if (file == NULL) { ret = -EBADF; - desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ); - if (desc.ctx == NULL) + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); + if (ctx == NULL) goto out_unlock; } else - desc.ctx = get_nfs_open_context(nfs_file_open_context(file)); + ctx = get_nfs_open_context(nfs_file_open_context(file)); - xchg(&desc.ctx->error, 0); - nfs_pageio_init_read(&desc.pgio, inode, false, + xchg(&ctx->error, 0); + nfs_pageio_init_read(&pgio, inode, false, &nfs_async_read_completion_ops); - ret = readpage_async_filler(&desc, page); + ret = nfs_pageio_add_page(&pgio, ctx, page); if (ret) goto out; - nfs_pageio_complete_read(&desc.pgio); - ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0; + nfs_pageio_complete_read(&pgio); + ret = pgio.pg_error < 0 ? pgio.pg_error : 0; if (!ret) { ret = wait_on_page_locked_killable(page); if (!PageUptodate(page) && !ret) - ret = xchg(&desc.ctx->error, 0); + ret = xchg(&ctx->error, 0); } out: - put_nfs_open_context(desc.ctx); + put_nfs_open_context(ctx); trace_nfs_aop_readpage_done(inode, page, ret); return ret; out_unlock: @@ -391,9 +390,10 @@ int nfs_read_folio(struct file *file, struct folio *folio) void nfs_readahead(struct readahead_control *ractl) { + struct nfs_pageio_descriptor pgio; + struct nfs_open_context *ctx; unsigned int nr_pages = readahead_count(ractl); struct file *file = ractl->file; - struct nfs_readdesc desc; struct inode *inode = ractl->mapping->host; struct page *page; int ret; @@ -407,25 +407,25 @@ void nfs_readahead(struct readahead_control *ractl) if (file == NULL) { ret = -EBADF; - desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ); - if (desc.ctx == NULL) + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); + if (ctx == NULL) goto out; } else - desc.ctx = get_nfs_open_context(nfs_file_open_context(file)); + ctx = get_nfs_open_context(nfs_file_open_context(file)); - nfs_pageio_init_read(&desc.pgio, inode, false, + nfs_pageio_init_read(&pgio, inode, false, &nfs_async_read_completion_ops); while ((page = readahead_page(ractl)) != NULL) { - ret = readpage_async_filler(&desc, page); + ret = nfs_pageio_add_page(&pgio, ctx, page); put_page(page); if (ret) break; } - nfs_pageio_complete_read(&desc.pgio); + nfs_pageio_complete_read(&pgio); - put_nfs_open_context(desc.ctx); + put_nfs_open_context(ctx); out: trace_nfs_aop_readahead_done(inode, nr_pages, ret); }