2014-03-18 11:28:25

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

We only use one static ops vector or grab it from the layout driver,
so clean up the whole code path to make that more obvious.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfs/direct.c | 4 ++--
fs/nfs/nfs3proc.c | 1 -
fs/nfs/nfs4proc.c | 1 -
fs/nfs/pnfs.c | 14 --------------
fs/nfs/pnfs.h | 8 --------
fs/nfs/proc.c | 1 -
fs/nfs/write.c | 20 ++++++++++++--------
include/linux/nfs_xdr.h | 2 --
8 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index b8797ae..bd9d524 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -564,7 +564,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
dreq->count = 0;
get_dreq(dreq);

- NFS_PROTO(dreq->inode)->write_pageio_init(&desc, dreq->inode, FLUSH_STABLE,
+ nfs_pageio_init_write(&desc, dreq->inode, FLUSH_STABLE,
&nfs_direct_write_completion_ops);
desc.pg_dreq = dreq;

@@ -874,7 +874,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
size_t requested_bytes = 0;
unsigned long seg;

- NFS_PROTO(inode)->write_pageio_init(&desc, inode, FLUSH_COND_STABLE,
+ nfs_pageio_init_write(&desc, inode, FLUSH_COND_STABLE,
&nfs_direct_write_completion_ops);
desc.pg_dreq = dreq;
get_dreq(dreq);
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index aa9bc97..5ae9abf 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -986,7 +986,6 @@ const struct nfs_rpc_ops nfs_v3_clientops = {
.read_rpc_prepare = nfs3_proc_read_rpc_prepare,
.read_done = nfs3_read_done,
.write_setup = nfs3_proc_write_setup,
- .write_pageio_init = nfs_pageio_init_write,
.write_rpc_prepare = nfs3_proc_write_rpc_prepare,
.write_done = nfs3_write_done,
.commit_setup = nfs3_proc_commit_setup,
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8672323..7759b5aa 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8466,7 +8466,6 @@ const struct nfs_rpc_ops nfs_v4_clientops = {
.read_rpc_prepare = nfs4_proc_read_rpc_prepare,
.read_done = nfs4_read_done,
.write_setup = nfs4_proc_write_setup,
- .write_pageio_init = pnfs_pageio_init_write,
.write_rpc_prepare = nfs4_proc_write_rpc_prepare,
.write_done = nfs4_write_done,
.commit_setup = nfs4_proc_commit_setup,
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index cb53d45..d8be82d 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1447,20 +1447,6 @@ pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, struct inode *inode,
nfs_pageio_init(pgio, inode, ld->pg_read_ops, compl_ops, server->rsize, 0);
}

-void
-pnfs_pageio_init_write(struct nfs_pageio_descriptor *pgio, struct inode *inode,
- int ioflags,
- const struct nfs_pgio_completion_ops *compl_ops)
-{
- struct nfs_server *server = NFS_SERVER(inode);
- struct pnfs_layoutdriver_type *ld = server->pnfs_curr_ld;
-
- if (ld == NULL)
- nfs_pageio_init_write(pgio, inode, ioflags, compl_ops);
- else
- nfs_pageio_init(pgio, inode, ld->pg_write_ops, compl_ops, server->wsize, ioflags);
-}
-
bool
pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
struct nfs_page *req)
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 0237939..e9ac8fb 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -182,8 +182,6 @@ void pnfs_put_lseg(struct pnfs_layout_segment *lseg);

void pnfs_pageio_init_read(struct nfs_pageio_descriptor *, struct inode *,
const struct nfs_pgio_completion_ops *);
-void pnfs_pageio_init_write(struct nfs_pageio_descriptor *, struct inode *,
- int, const struct nfs_pgio_completion_ops *);

void set_pnfs_layoutdriver(struct nfs_server *, const struct nfs_fh *, u32);
void unset_pnfs_layoutdriver(struct nfs_server *);
@@ -467,12 +465,6 @@ static inline void pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, str
nfs_pageio_init_read(pgio, inode, compl_ops);
}

-static inline void pnfs_pageio_init_write(struct nfs_pageio_descriptor *pgio, struct inode *inode, int ioflags,
- const struct nfs_pgio_completion_ops *compl_ops)
-{
- nfs_pageio_init_write(pgio, inode, ioflags, compl_ops);
-}
-
static inline int
pnfs_commit_list(struct inode *inode, struct list_head *mds_pages, int how,
struct nfs_commit_info *cinfo)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index fddbba2..6ea2bed 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -764,7 +764,6 @@ const struct nfs_rpc_ops nfs_v2_clientops = {
.read_rpc_prepare = nfs_proc_read_rpc_prepare,
.read_done = nfs_read_done,
.write_setup = nfs_proc_write_setup,
- .write_pageio_init = nfs_pageio_init_write,
.write_rpc_prepare = nfs_proc_write_rpc_prepare,
.write_done = nfs_write_done,
.commit_setup = nfs_proc_commit_setup,
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 9a3b6a4..8afed78 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -354,10 +354,8 @@ static int nfs_writepage_locked(struct page *page, struct writeback_control *wbc
struct nfs_pageio_descriptor pgio;
int err;

- NFS_PROTO(page_file_mapping(page)->host)->write_pageio_init(&pgio,
- page->mapping->host,
- wb_priority(wbc),
- &nfs_async_write_completion_ops);
+ nfs_pageio_init_write(&pgio, page->mapping->host, wb_priority(wbc),
+ &nfs_async_write_completion_ops);
err = nfs_do_writepage(page, wbc, &pgio);
nfs_pageio_complete(&pgio);
if (err < 0)
@@ -400,7 +398,8 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)

nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGES);

- NFS_PROTO(inode)->write_pageio_init(&pgio, inode, wb_priority(wbc), &nfs_async_write_completion_ops);
+ nfs_pageio_init_write(&pgio, inode, wb_priority(wbc),
+ &nfs_async_write_completion_ops);
err = write_cache_pages(mapping, wbc, nfs_writepages_callback, &pgio);
nfs_pageio_complete(&pgio);

@@ -1285,10 +1284,15 @@ void nfs_pageio_init_write(struct nfs_pageio_descriptor *pgio,
struct inode *inode, int ioflags,
const struct nfs_pgio_completion_ops *compl_ops)
{
- nfs_pageio_init(pgio, inode, &nfs_pageio_write_ops, compl_ops,
- NFS_SERVER(inode)->wsize, ioflags);
+ struct nfs_server *server = NFS_SERVER(inode);
+ const struct nfs_pageio_ops *pg_ops = &nfs_pageio_write_ops;
+
+#if IS_ENABLED(CONFIG_NFS_V4)
+ if (server->pnfs_curr_ld)
+ pg_ops = server->pnfs_curr_ld->pg_write_ops;
+#endif
+ nfs_pageio_init(pgio, inode, pg_ops, compl_ops, server->wsize, ioflags);
}
-EXPORT_SYMBOL_GPL(nfs_pageio_init_write);

void nfs_pageio_reset_write_mds(struct nfs_pageio_descriptor *pgio)
{
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index b2fb167..ed12f03 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1467,8 +1467,6 @@ struct nfs_rpc_ops {
int (*read_rpc_prepare)(struct rpc_task *, struct nfs_read_data *);
int (*read_done) (struct rpc_task *, struct nfs_read_data *);
void (*write_setup) (struct nfs_write_data *, struct rpc_message *);
- void (*write_pageio_init)(struct nfs_pageio_descriptor *, struct inode *, int,
- const struct nfs_pgio_completion_ops *);
int (*write_rpc_prepare)(struct rpc_task *, struct nfs_write_data *);
int (*write_done) (struct rpc_task *, struct nfs_write_data *);
void (*commit_setup) (struct nfs_commit_data *, struct rpc_message *);
--
1.7.10.4



2014-03-18 18:04:32

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

On 03/18/2014 10:09 AM, Christoph Hellwig wrote:
>>> + struct nfs_server *server = NFS_SERVER(inode);
>>> + const struct nfs_pageio_ops *pg_ops = &nfs_pageio_write_ops;
>>> +
>>> +#if IS_ENABLED(CONFIG_NFS_V4)
>>> + if (server->pnfs_curr_ld)
>>> + pg_ops = server->pnfs_curr_ld->pg_write_ops;
>>> +#endif
>>> + nfs_pageio_init(pgio, inode, pg_ops, compl_ops, server->wsize, ioflags);
>>> }
>> I thought it was bad style to put a #ifdef in the middle of a function, which is why we have so many noop functions in the rest of the code. Should this be done with a select_compl_ops() function instead?
> I don't think it's much of a problem for a small and readable function
> with a single ifdef. And we'd need one helper for read and write each,
> which would make the code much harder to read. Still better than what's
> there currently, though.
>
> The other option would be to simply always have the pnfs_curr_ld field
> in the server structure and remove the ifdef entirely.

Okay. If we keep the ifdef, then it should probably check for CONFIG_NFS_V4_1 and not CONFIG_NFS_V4

2014-03-19 07:41:36

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

On Tue, Mar 18, 2014 at 02:04:28PM -0400, Anna Schumaker wrote:
> > The other option would be to simply always have the pnfs_curr_ld field
> > in the server structure and remove the ifdef entirely.
>
> Okay. If we keep the ifdef, then it should probably check for CONFIG_NFS_V4_1 and not CONFIG_NFS_V4

The field in the header is under CONFIG_NFS_V4, so I tried to make it
consistent.


2014-03-18 12:49:47

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

Hey Christoph,

On 03/18/2014 07:28 AM, Christoph Hellwig wrote:
> We only use one static ops vector or grab it from the layout driver,
> so clean up the whole code path to make that more obvious.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/nfs/direct.c | 4 ++--
> fs/nfs/nfs3proc.c | 1 -
> fs/nfs/nfs4proc.c | 1 -
> fs/nfs/pnfs.c | 14 --------------
> fs/nfs/pnfs.h | 8 --------
> fs/nfs/proc.c | 1 -
> fs/nfs/write.c | 20 ++++++++++++--------
> include/linux/nfs_xdr.h | 2 --
> 8 files changed, 14 insertions(+), 37 deletions(-)
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index b8797ae..bd9d524 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -564,7 +564,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
> dreq->count = 0;
> get_dreq(dreq);
>
> - NFS_PROTO(dreq->inode)->write_pageio_init(&desc, dreq->inode, FLUSH_STABLE,
> + nfs_pageio_init_write(&desc, dreq->inode, FLUSH_STABLE,
> &nfs_direct_write_completion_ops);
> desc.pg_dreq = dreq;
>
> @@ -874,7 +874,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
> size_t requested_bytes = 0;
> unsigned long seg;
>
> - NFS_PROTO(inode)->write_pageio_init(&desc, inode, FLUSH_COND_STABLE,
> + nfs_pageio_init_write(&desc, inode, FLUSH_COND_STABLE,
> &nfs_direct_write_completion_ops);
> desc.pg_dreq = dreq;
> get_dreq(dreq);
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index aa9bc97..5ae9abf 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -986,7 +986,6 @@ const struct nfs_rpc_ops nfs_v3_clientops = {
> .read_rpc_prepare = nfs3_proc_read_rpc_prepare,
> .read_done = nfs3_read_done,
> .write_setup = nfs3_proc_write_setup,
> - .write_pageio_init = nfs_pageio_init_write,
> .write_rpc_prepare = nfs3_proc_write_rpc_prepare,
> .write_done = nfs3_write_done,
> .commit_setup = nfs3_proc_commit_setup,
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8672323..7759b5aa 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -8466,7 +8466,6 @@ const struct nfs_rpc_ops nfs_v4_clientops = {
> .read_rpc_prepare = nfs4_proc_read_rpc_prepare,
> .read_done = nfs4_read_done,
> .write_setup = nfs4_proc_write_setup,
> - .write_pageio_init = pnfs_pageio_init_write,
> .write_rpc_prepare = nfs4_proc_write_rpc_prepare,
> .write_done = nfs4_write_done,
> .commit_setup = nfs4_proc_commit_setup,
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index cb53d45..d8be82d 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -1447,20 +1447,6 @@ pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, struct inode *inode,
> nfs_pageio_init(pgio, inode, ld->pg_read_ops, compl_ops, server->rsize, 0);
> }
>
> -void
> -pnfs_pageio_init_write(struct nfs_pageio_descriptor *pgio, struct inode *inode,
> - int ioflags,
> - const struct nfs_pgio_completion_ops *compl_ops)
> -{
> - struct nfs_server *server = NFS_SERVER(inode);
> - struct pnfs_layoutdriver_type *ld = server->pnfs_curr_ld;
> -
> - if (ld == NULL)
> - nfs_pageio_init_write(pgio, inode, ioflags, compl_ops);
> - else
> - nfs_pageio_init(pgio, inode, ld->pg_write_ops, compl_ops, server->wsize, ioflags);
> -}
> -
> bool
> pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
> struct nfs_page *req)
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index 0237939..e9ac8fb 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -182,8 +182,6 @@ void pnfs_put_lseg(struct pnfs_layout_segment *lseg);
>
> void pnfs_pageio_init_read(struct nfs_pageio_descriptor *, struct inode *,
> const struct nfs_pgio_completion_ops *);
> -void pnfs_pageio_init_write(struct nfs_pageio_descriptor *, struct inode *,
> - int, const struct nfs_pgio_completion_ops *);
>
> void set_pnfs_layoutdriver(struct nfs_server *, const struct nfs_fh *, u32);
> void unset_pnfs_layoutdriver(struct nfs_server *);
> @@ -467,12 +465,6 @@ static inline void pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, str
> nfs_pageio_init_read(pgio, inode, compl_ops);
> }
>
> -static inline void pnfs_pageio_init_write(struct nfs_pageio_descriptor *pgio, struct inode *inode, int ioflags,
> - const struct nfs_pgio_completion_ops *compl_ops)
> -{
> - nfs_pageio_init_write(pgio, inode, ioflags, compl_ops);
> -}
> -
> static inline int
> pnfs_commit_list(struct inode *inode, struct list_head *mds_pages, int how,
> struct nfs_commit_info *cinfo)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index fddbba2..6ea2bed 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -764,7 +764,6 @@ const struct nfs_rpc_ops nfs_v2_clientops = {
> .read_rpc_prepare = nfs_proc_read_rpc_prepare,
> .read_done = nfs_read_done,
> .write_setup = nfs_proc_write_setup,
> - .write_pageio_init = nfs_pageio_init_write,
> .write_rpc_prepare = nfs_proc_write_rpc_prepare,
> .write_done = nfs_write_done,
> .commit_setup = nfs_proc_commit_setup,
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 9a3b6a4..8afed78 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -354,10 +354,8 @@ static int nfs_writepage_locked(struct page *page, struct writeback_control *wbc
> struct nfs_pageio_descriptor pgio;
> int err;
>
> - NFS_PROTO(page_file_mapping(page)->host)->write_pageio_init(&pgio,
> - page->mapping->host,
> - wb_priority(wbc),
> - &nfs_async_write_completion_ops);
> + nfs_pageio_init_write(&pgio, page->mapping->host, wb_priority(wbc),
> + &nfs_async_write_completion_ops);
> err = nfs_do_writepage(page, wbc, &pgio);
> nfs_pageio_complete(&pgio);
> if (err < 0)
> @@ -400,7 +398,8 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
>
> nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGES);
>
> - NFS_PROTO(inode)->write_pageio_init(&pgio, inode, wb_priority(wbc), &nfs_async_write_completion_ops);
> + nfs_pageio_init_write(&pgio, inode, wb_priority(wbc),
> + &nfs_async_write_completion_ops);
> err = write_cache_pages(mapping, wbc, nfs_writepages_callback, &pgio);
> nfs_pageio_complete(&pgio);
>
> @@ -1285,10 +1284,15 @@ void nfs_pageio_init_write(struct nfs_pageio_descriptor *pgio,
> struct inode *inode, int ioflags,
> const struct nfs_pgio_completion_ops *compl_ops)
> {
> - nfs_pageio_init(pgio, inode, &nfs_pageio_write_ops, compl_ops,
> - NFS_SERVER(inode)->wsize, ioflags);
> + struct nfs_server *server = NFS_SERVER(inode);
> + const struct nfs_pageio_ops *pg_ops = &nfs_pageio_write_ops;
> +
> +#if IS_ENABLED(CONFIG_NFS_V4)
> + if (server->pnfs_curr_ld)
> + pg_ops = server->pnfs_curr_ld->pg_write_ops;
> +#endif
> + nfs_pageio_init(pgio, inode, pg_ops, compl_ops, server->wsize, ioflags);
> }

I thought it was bad style to put a #ifdef in the middle of a function, which is why we have so many noop functions in the rest of the code. Should this be done with a select_compl_ops() function instead?

Anna

> -EXPORT_SYMBOL_GPL(nfs_pageio_init_write);
>
> void nfs_pageio_reset_write_mds(struct nfs_pageio_descriptor *pgio)
> {
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index b2fb167..ed12f03 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1467,8 +1467,6 @@ struct nfs_rpc_ops {
> int (*read_rpc_prepare)(struct rpc_task *, struct nfs_read_data *);
> int (*read_done) (struct rpc_task *, struct nfs_read_data *);
> void (*write_setup) (struct nfs_write_data *, struct rpc_message *);
> - void (*write_pageio_init)(struct nfs_pageio_descriptor *, struct inode *, int,
> - const struct nfs_pgio_completion_ops *);
> int (*write_rpc_prepare)(struct rpc_task *, struct nfs_write_data *);
> int (*write_done) (struct rpc_task *, struct nfs_write_data *);
> void (*commit_setup) (struct nfs_commit_data *, struct rpc_message *);


2014-03-18 14:09:43

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

> > + struct nfs_server *server = NFS_SERVER(inode);
> > + const struct nfs_pageio_ops *pg_ops = &nfs_pageio_write_ops;
> > +
> > +#if IS_ENABLED(CONFIG_NFS_V4)
> > + if (server->pnfs_curr_ld)
> > + pg_ops = server->pnfs_curr_ld->pg_write_ops;
> > +#endif
> > + nfs_pageio_init(pgio, inode, pg_ops, compl_ops, server->wsize, ioflags);
> > }
>
> I thought it was bad style to put a #ifdef in the middle of a function, which is why we have so many noop functions in the rest of the code. Should this be done with a select_compl_ops() function instead?

I don't think it's much of a problem for a small and readable function
with a single ifdef. And we'd need one helper for read and write each,
which would make the code much harder to read. Still better than what's
there currently, though.

The other option would be to simply always have the pnfs_curr_ld field
in the server structure and remove the ifdef entirely.

2014-03-19 13:11:39

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

On 03/19/2014 03:41 AM, Christoph Hellwig wrote:
> On Tue, Mar 18, 2014 at 02:04:28PM -0400, Anna Schumaker wrote:
>>> The other option would be to simply always have the pnfs_curr_ld field
>>> in the server structure and remove the ifdef entirely.
>> Okay. If we keep the ifdef, then it should probably check for CONFIG_NFS_V4_1 and not CONFIG_NFS_V4
> The field in the header is under CONFIG_NFS_V4, so I tried to make it
> consistent.
>
struct pnfs_layoutdriver_type comes from fs/nfs/pnfs.h under a CONFIG_NFS_V4_1 check, so you can't access pg_write_ops or pg_read_ops without v4.1 enabled.


2014-03-19 15:40:49

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfs: remove ->write_pageio_init from rpc ops

On Wed, Mar 19, 2014 at 09:11:35AM -0400, Anna Schumaker wrote:
> On 03/19/2014 03:41 AM, Christoph Hellwig wrote:
> > On Tue, Mar 18, 2014 at 02:04:28PM -0400, Anna Schumaker wrote:
> >>> The other option would be to simply always have the pnfs_curr_ld field
> >>> in the server structure and remove the ifdef entirely.
> >> Okay. If we keep the ifdef, then it should probably check for CONFIG_NFS_V4_1 and not CONFIG_NFS_V4
> > The field in the header is under CONFIG_NFS_V4, so I tried to make it
> > consistent.
> >
> struct pnfs_layoutdriver_type comes from fs/nfs/pnfs.h under a CONFIG_NFS_V4_1 check, so you can't access pg_write_ops or pg_read_ops without v4.1 enabled.

You're right - the field exists for plain v4, but we can't dereference
it unless 4.1 support is enable. I'll send an update.

2014-03-18 11:28:45

by Christoph Hellwig

[permalink] [raw]
Subject: [PATHC 2/2] nfs: remove ->read_pageio_init from rpc ops

We only use one static ops vector or grab it from the layout driver,
so clean up the whole code path to make that more obvious.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfs/direct.c | 3 +--
fs/nfs/nfs3proc.c | 1 -
fs/nfs/nfs4proc.c | 1 -
fs/nfs/pnfs.c | 13 -------------
fs/nfs/pnfs.h | 9 ---------
fs/nfs/proc.c | 1 -
fs/nfs/read.c | 16 +++++++++++-----
include/linux/nfs_xdr.h | 2 --
8 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index bd9d524..bb6fcb8 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -424,8 +424,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
size_t requested_bytes = 0;
unsigned long seg;

- NFS_PROTO(dreq->inode)->read_pageio_init(&desc, dreq->inode,
- &nfs_direct_read_completion_ops);
+ nfs_pageio_init_read(&desc, dreq->inode, &nfs_direct_read_completion_ops);
get_dreq(dreq);
desc.pg_dreq = dreq;
atomic_inc(&inode->i_dio_count);
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 5ae9abf..feb2049 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -982,7 +982,6 @@ const struct nfs_rpc_ops nfs_v3_clientops = {
.pathconf = nfs3_proc_pathconf,
.decode_dirent = nfs3_decode_dirent,
.read_setup = nfs3_proc_read_setup,
- .read_pageio_init = nfs_pageio_init_read,
.read_rpc_prepare = nfs3_proc_read_rpc_prepare,
.read_done = nfs3_read_done,
.write_setup = nfs3_proc_write_setup,
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7759b5aa..9a8eb6a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8462,7 +8462,6 @@ const struct nfs_rpc_ops nfs_v4_clientops = {
.set_capabilities = nfs4_server_capabilities,
.decode_dirent = nfs4_decode_dirent,
.read_setup = nfs4_proc_read_setup,
- .read_pageio_init = pnfs_pageio_init_read,
.read_rpc_prepare = nfs4_proc_read_rpc_prepare,
.read_done = nfs4_read_done,
.write_setup = nfs4_proc_write_setup,
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d8be82d..c965f7d 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1434,19 +1434,6 @@ pnfs_generic_pg_init_write(struct nfs_pageio_descriptor *pgio,
}
EXPORT_SYMBOL_GPL(pnfs_generic_pg_init_write);

-void
-pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, struct inode *inode,
- const struct nfs_pgio_completion_ops *compl_ops)
-{
- struct nfs_server *server = NFS_SERVER(inode);
- struct pnfs_layoutdriver_type *ld = server->pnfs_curr_ld;
-
- if (ld == NULL)
- nfs_pageio_init_read(pgio, inode, compl_ops);
- else
- nfs_pageio_init(pgio, inode, ld->pg_read_ops, compl_ops, server->rsize, 0);
-}
-
bool
pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
struct nfs_page *req)
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index e9ac8fb..94a9a18 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -180,9 +180,6 @@ extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);
void pnfs_get_layout_hdr(struct pnfs_layout_hdr *lo);
void pnfs_put_lseg(struct pnfs_layout_segment *lseg);

-void pnfs_pageio_init_read(struct nfs_pageio_descriptor *, struct inode *,
- const struct nfs_pgio_completion_ops *);
-
void set_pnfs_layoutdriver(struct nfs_server *, const struct nfs_fh *, u32);
void unset_pnfs_layoutdriver(struct nfs_server *);
void pnfs_generic_pg_init_read(struct nfs_pageio_descriptor *, struct nfs_page *);
@@ -459,12 +456,6 @@ static inline void unset_pnfs_layoutdriver(struct nfs_server *s)
{
}

-static inline void pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, struct inode *inode,
- const struct nfs_pgio_completion_ops *compl_ops)
-{
- nfs_pageio_init_read(pgio, inode, compl_ops);
-}
-
static inline int
pnfs_commit_list(struct inode *inode, struct list_head *mds_pages, int how,
struct nfs_commit_info *cinfo)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 6ea2bed..7676973 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -760,7 +760,6 @@ const struct nfs_rpc_ops nfs_v2_clientops = {
.pathconf = nfs_proc_pathconf,
.decode_dirent = nfs2_decode_dirent,
.read_setup = nfs_proc_read_setup,
- .read_pageio_init = nfs_pageio_init_read,
.read_rpc_prepare = nfs_proc_read_rpc_prepare,
.read_done = nfs_read_done,
.write_setup = nfs_proc_write_setup,
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 411aedd..190fb63 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -24,6 +24,7 @@
#include "internal.h"
#include "iostat.h"
#include "fscache.h"
+#include "pnfs.h"

#define NFSDBG_FACILITY NFSDBG_PAGECACHE

@@ -117,10 +118,15 @@ void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
struct inode *inode,
const struct nfs_pgio_completion_ops *compl_ops)
{
- nfs_pageio_init(pgio, inode, &nfs_pageio_read_ops, compl_ops,
- NFS_SERVER(inode)->rsize, 0);
+ struct nfs_server *server = NFS_SERVER(inode);
+ const struct nfs_pageio_ops *pg_ops = &nfs_pageio_read_ops;
+
+#if IS_ENABLED(CONFIG_NFS_V4)
+ if (server->pnfs_curr_ld)
+ pg_ops = server->pnfs_curr_ld->pg_read_ops;
+#endif
+ nfs_pageio_init(pgio, inode, pg_ops, compl_ops, server->rsize, 0);
}
-EXPORT_SYMBOL_GPL(nfs_pageio_init_read);

void nfs_pageio_reset_read_mds(struct nfs_pageio_descriptor *pgio)
{
@@ -147,7 +153,7 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode,
if (len < PAGE_CACHE_SIZE)
zero_user_segment(page, len, PAGE_CACHE_SIZE);

- NFS_PROTO(inode)->read_pageio_init(&pgio, inode, &nfs_async_read_completion_ops);
+ nfs_pageio_init_read(&pgio, inode, &nfs_async_read_completion_ops);
nfs_pageio_add_request(&pgio, new);
nfs_pageio_complete(&pgio);
NFS_I(inode)->read_io += pgio.pg_bytes_written;
@@ -654,7 +660,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
if (ret == 0)
goto read_complete; /* all pages were read */

- NFS_PROTO(inode)->read_pageio_init(&pgio, inode, &nfs_async_read_completion_ops);
+ nfs_pageio_init_read(&pgio, inode, &nfs_async_read_completion_ops);

ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc);

diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index ed12f03..0146485 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1462,8 +1462,6 @@ struct nfs_rpc_ops {
int (*set_capabilities)(struct nfs_server *, struct nfs_fh *);
int (*decode_dirent)(struct xdr_stream *, struct nfs_entry *, int);
void (*read_setup) (struct nfs_read_data *, struct rpc_message *);
- void (*read_pageio_init)(struct nfs_pageio_descriptor *, struct inode *,
- const struct nfs_pgio_completion_ops *);
int (*read_rpc_prepare)(struct rpc_task *, struct nfs_read_data *);
int (*read_done) (struct rpc_task *, struct nfs_read_data *);
void (*write_setup) (struct nfs_write_data *, struct rpc_message *);
--
1.7.10.4