This one scares me.
On Sat, Mar 28, 2009 at 11:32:32AM +0300, Benny Halevy wrote:
> From: Andy Adamson <andros@xxxxxxxxxx>
>
> Cache all the result pages, including the rpc header in rq_respages[0],
> for a request in the slot table cache entry.
>
> Cache the statp pointer from nfsd_dispatch which points into rq_respages[0]
> just past the rpc header. When setting a cache entry, calculate and save the
> length of the nfs data minus the rpc header for rq_respages[0].
>
> When replaying a cache entry, replace the cached rpc header with the
> replayed request rpc result header, unless there is not enough room in the
> cached results first page. In that case, use the cached rpc header.
>
> The sessions fore channel maxresponse size cached is set to NFSD_PAGES_PER_SLOT
> * PAGE_SIZE. For compounds we are cacheing with operations such as READDIR
> that use the xdr_buf->pages to hold data, we choose to cache the extra page of
> data rather than copying data from xdr_buf->pages into the xdr_buf->head page.
>
> [nfsd41: limit cache to maxresponsesize_cached]
> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> [nfsd41: mv nfsd4_set_statp under CONFIG_NFSD_V4_1]
> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 142 ++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfssvc.c | 4 +
> include/linux/nfsd/cache.h | 5 ++
> include/linux/nfsd/state.h | 13 ++++
> include/linux/nfsd/xdr4.h | 4 +
> 5 files changed, 168 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 10eb67b..f0ce639 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -860,6 +860,148 @@ out_err:
> }
>
> #if defined(CONFIG_NFSD_V4_1)
> +void
> +nfsd4_set_statp(struct svc_rqst *rqstp, __be32 *statp)
> +{
> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> +
> + resp->cstate.statp = statp;
> +}
> +
> +/*
> + * Dereference the result pages.
> + */
> +static void
> +nfsd4_release_respages(struct page **respages, short resused)
> +{
> + int page_no;
> +
> + dprintk("--> %s\n", __func__);
> + for (page_no = 0; page_no < resused; page_no++) {
> + if (!respages[page_no])
> + continue;
> + put_page(respages[page_no]);
> + respages[page_no] = NULL;
> + }
> +}
> +
> +static void
> +nfsd4_move_pages(struct page **topages, struct page **frompages, short count)
s/move/copy/; we're not removing anything from the source.
> +{
> + int page_no;
As a general matter of style, I'd rather any loop variable in a function
this short and simple be named "i". "j" if you need another....
> +
> + for (page_no = 0; page_no < count; page_no++) {
> + topages[page_no] = frompages[page_no];
> + if (!topages[page_no])
> + continue;
> + get_page(topages[page_no]);
> + }
> +}
> +
> +/*
> + * Cache the reply pages up to NFSD_PAGES_PER_SLOT + 1, clearing the previous
> + * pages. We add a page to NFSD_PAGES_PER_SLOT for the case where the total
> + * length of the XDR response is less than se_fmaxresp_cached
> + * (NFSD_PAGES_PER_SLOT * PAGE_SIZE) but the xdr_buf pages is used for a
> + * of the reply (e.g. readdir).
That comment isn't very clear.
Is one page really sufficient? Consider, for example, a 2-byte read
which spans a page boundary:
first page: rpc header, compound header, putfh reply, etc.
second page: 1st byte of read data
third page: 2nd byte of read data
fourth page: 2 bytes of padding, rest of reply.
That's for a reply of total length less than a page.
> + *
> + * Store the base and length of the rq_req.head[0] page
> + * of the NFSv4.1 data, just past the rpc header.
> + */
> +void
> +nfsd4_set_cache_entry(struct nfsd4_compoundres *resp)
I find "set" a little vague. How about "store"?
> +{
> + struct nfsd4_cache_entry *entry = &resp->cstate.slot->sl_cache_entry;
> + struct svc_rqst *rqstp = resp->rqstp;
> + struct kvec *resv = &rqstp->rq_res.head[0];
> +
> + dprintk("--> %s entry %p\n", __func__, entry);
> +
> + /* Don't cache a failed OP_SEQUENCE */
> + if (resp->opcnt == 1 && resp->cstate.status)
> + return;
> + nfsd4_release_respages(entry->ce_respages, entry->ce_resused);
> + entry->ce_resused = rqstp->rq_resused;
> + if (entry->ce_resused > NFSD_PAGES_PER_SLOT + 1)
> + entry->ce_resused = NFSD_PAGES_PER_SLOT + 1;
> + nfsd4_move_pages(entry->ce_respages, rqstp->rq_respages,
> + entry->ce_resused);
> + entry->ce_status = resp->cstate.status;
Don't we need to track rq_res.page_base, page_len, etc.? Try testing
replays of small unaligned reads.
> + entry->ce_datav.iov_base = resp->cstate.statp;
> + entry->ce_datav.iov_len = resv->iov_len - ((char *)resp->cstate.statp -
> + (char *)page_address(rqstp->rq_respages[0]));
> + entry->ce_opcnt = resp->opcnt;
Why do we need to save and restore the number of operations?
In general--I'd rather functions and data structures got introduced in
the same patch as their users; they're harder to judge on their own.
> + /* Current request rpc header length*/
> + entry->ce_rpchdrlen = (char *)resp->cstate.statp -
> + (char *)page_address(rqstp->rq_respages[0]);
I don't believe we need to save ce_rpchdrlen.
> +}
> +
> +/*
> + * Copy the cached NFSv4.1 reply skipping the cached rpc header into the
> + * replay result res.head[0] past the rpc header to end up with replay
> + * rpc header and cached NFSv4.1 reply.
This comment could be clearer; how about just:
We keep the rpc header, but take the nfs reply from the reply
cache.
?
> + */
> +static int
> +nfsd41_copy_replay_data(struct nfsd4_compoundres *resp,
> + struct nfsd4_cache_entry *entry)
> +{
> + struct svc_rqst *rqstp = resp->rqstp;
> + struct kvec *resv = &resp->rqstp->rq_res.head[0];
> + int len;
> +
> + /* Current request rpc header length*/
> + len = (char *)resp->cstate.statp -
> + (char *)page_address(rqstp->rq_respages[0]);
Could write just resv->iov_base for for the second term there, I
beleive.
> + if (entry->ce_datav.iov_len + len > PAGE_SIZE) {
This should depend on NFSD_MAX_PAGES_PER_SLOT, or something--we
shouldn't be hard-wiring the assumption that the maximum cached reply
size is PAGE_SIZE.
> + dprintk("%s v41 cached reply too large (%Zd).\n", __func__,
> + entry->ce_datav.iov_len);
> + return 0;
> + }
> + /* copy the cached reply nfsd data past the current rpc header */
> + memcpy((char *)resv->iov_base + len, entry->ce_datav.iov_base,
That first argument could just be resp->cstate.statp.
> + entry->ce_datav.iov_len);
> + resv->iov_len = len + entry->ce_datav.iov_len;
> + return 1;
> +}
> +
> +/*
> + * Keep the first page of the replay. Copy the NFSv4.1 data from the first
> + * cached page. Replace any futher replay pages from the cache.
> + */
> +__be32
> +nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp)
> +{
> + struct nfsd4_cache_entry *entry = &resp->cstate.slot->sl_cache_entry;
> + __be32 status;
> +
> + dprintk("--> %s entry %p\n", __func__, entry);
> +
> +
> + if (!nfsd41_copy_replay_data(resp, entry)) {
> + /*
> + * Not enough room to use the replay rpc header, send the
> + * cached header. Release all the allocated result pages.
> + */
No, we can't do this. The protocol requires that we use the rpc header
from the replay.
--b.
> + svc_free_res_pages(resp->rqstp);
> + nfsd4_move_pages(resp->rqstp->rq_respages, entry->ce_respages,
> + entry->ce_resused);
> + } else {
> + /* Release all but the first allocated result page */
> +
> + resp->rqstp->rq_resused--;
> + svc_free_res_pages(resp->rqstp);
> +
> + nfsd4_move_pages(&resp->rqstp->rq_respages[1],
> + &entry->ce_respages[1],
> + entry->ce_resused - 1);
> + }
> +
> + resp->rqstp->rq_resused = entry->ce_resused;
> + status = entry->ce_status;
> +
> + return status;
> +}
> +
> /*
> * Set the exchange_id flags returned by the server.
> */
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index ef0a368..b5168d1 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -515,6 +515,10 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
> + rqstp->rq_res.head[0].iov_len;
> rqstp->rq_res.head[0].iov_len += sizeof(__be32);
>
> + /* NFSv4.1 DRC requires statp */
> + if (rqstp->rq_vers == 4)
> + nfsd4_set_statp(rqstp, statp);
> +
> /* Now call the procedure handler, and encode NFS status. */
> nfserr = proc->pc_func(rqstp, rqstp->rq_argp, rqstp->rq_resp);
> nfserr = map_new_errors(rqstp->rq_vers, nfserr);
> diff --git a/include/linux/nfsd/cache.h b/include/linux/nfsd/cache.h
> index 04b355c..57a83c7 100644
> --- a/include/linux/nfsd/cache.h
> +++ b/include/linux/nfsd/cache.h
> @@ -75,5 +75,10 @@ int nfsd_reply_cache_init(void);
> void nfsd_reply_cache_shutdown(void);
> int nfsd_cache_lookup(struct svc_rqst *, int);
> void nfsd_cache_update(struct svc_rqst *, int, __be32 *);
> +#ifdef CONFIG_NFSD_V4_1
> +void nfsd4_set_statp(struct svc_rqst *rqstp, __be32 *statp);
> +#else /* CONFIG_NFSD_V4_1 */
> +static inline void nfsd4_set_statp(struct svc_rqst *rqstp, __be32 *statp) {}
> +#endif /* CONFIG_NFSD_V4_1 */
>
> #endif /* NFSCACHE_H */
> diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
> index feab6ec..8ca6a82 100644
> --- a/include/linux/nfsd/state.h
> +++ b/include/linux/nfsd/state.h
> @@ -99,10 +99,23 @@ struct nfs4_callback {
> struct rpc_clnt * cb_client;
> };
>
> +/* Maximum number of pages per slot cache entry */
> +#define NFSD_PAGES_PER_SLOT 1
> +
> +struct nfsd4_cache_entry {
> + __be32 ce_status;
> + struct kvec ce_datav; /* encoded NFSv4.1 data in rq_res.head[0] */
> + struct page *ce_respages[NFSD_PAGES_PER_SLOT + 1];
> + short ce_resused;
> + int ce_opcnt;
> + int ce_rpchdrlen;
> +};
> +
> struct nfsd4_slot {
> bool sl_inuse;
> struct nfsd4_session *sl_session;
> u32 sl_seqid;
> + struct nfsd4_cache_entry sl_cache_entry;
> };
>
> struct nfsd4_session {
> diff --git a/include/linux/nfsd/xdr4.h b/include/linux/nfsd/xdr4.h
> index 9e4d8db..cde8947 100644
> --- a/include/linux/nfsd/xdr4.h
> +++ b/include/linux/nfsd/xdr4.h
> @@ -50,6 +50,8 @@ struct nfsd4_compound_state {
> struct nfs4_stateowner *replay_owner;
> /* For sessions DRC */
> struct nfsd4_slot *slot;
> + __be32 *statp;
> + u32 status;
> };
>
> struct nfsd4_change_info {
> @@ -490,6 +492,8 @@ extern __be32 nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> struct nfsd4_compound_state *,
> struct nfsd4_setclientid_confirm *setclientid_confirm);
> #if defined(CONFIG_NFSD_V4_1)
> +extern void nfsd4_set_cache_entry(struct nfsd4_compoundres *resp);
> +extern __be32 nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp);
> extern __be32 nfsd4_exchange_id(struct svc_rqst *rqstp,
> struct nfsd4_compound_state *,
> struct nfsd4_exchange_id *);
> --
> 1.6.2.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
