From: Bryan Schumaker <[email protected]>
When I initially wrote it, I didn't understand how lists worked so I
wrote something that didn't use them. I think making a list of stateids
to test is a more straightforward implementation, especially compared to
especially compared to decoding stateids while simultaneously encoding
a reply to the client.
Signed-off-by: Bryan Schumaker <[email protected]>
---
fs/nfsd/nfs4state.c | 9 ++++++-
fs/nfsd/nfs4xdr.c | 70 +++++++++++++++++---------------------------------
fs/nfsd/xdr4.h | 9 +++++-
3 files changed, 39 insertions(+), 49 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e8c98f0..c59a3f6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3400,7 +3400,14 @@ __be32
nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd4_test_stateid *test_stateid)
{
- /* real work is done during encoding */
+ struct nfsd4_test_stateid_id *stateid;
+ struct nfs4_client *cl = cstate->session->se_client;
+
+ nfs4_lock_state();
+ list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
+ stateid->ts_id_status = nfs4_validate_stateid(cl, &stateid->ts_id_stateid);
+ nfs4_unlock_state();
+
return nfs_ok;
}
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 0ec5a1b..67fe3d7 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -133,22 +133,6 @@ xdr_error: \
} \
} while (0)
-static void save_buf(struct nfsd4_compoundargs *argp, struct nfsd4_saved_compoundargs *savep)
-{
- savep->p = argp->p;
- savep->end = argp->end;
- savep->pagelen = argp->pagelen;
- savep->pagelist = argp->pagelist;
-}
-
-static void restore_buf(struct nfsd4_compoundargs *argp, struct nfsd4_saved_compoundargs *savep)
-{
- argp->p = savep->p;
- argp->end = savep->end;
- argp->pagelen = savep->pagelen;
- argp->pagelist = savep->pagelist;
-}
-
static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32 nbytes)
{
/* We want more bytes than seem to be available.
@@ -1385,31 +1369,36 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
static __be32
nfsd4_decode_test_stateid(struct nfsd4_compoundargs *argp, struct nfsd4_test_stateid *test_stateid)
{
- unsigned int nbytes;
- stateid_t si;
int i;
- __be32 *p;
- __be32 status;
+ __be32 *p, status;
+ struct nfsd4_test_stateid_id *stateid;
READ_BUF(4);
test_stateid->ts_num_ids = ntohl(*p++);
- nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
- if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
- goto xdr_error;
-
- test_stateid->ts_saved_args = argp;
- save_buf(argp, &test_stateid->ts_savedp);
+ INIT_LIST_HEAD(&test_stateid->ts_stateid_list);
for (i = 0; i < test_stateid->ts_num_ids; i++) {
- status = nfsd4_decode_stateid(argp, &si);
+ stateid = kmalloc(sizeof(struct nfsd4_test_stateid_id), GFP_KERNEL);
+ if (!stateid)
+ goto out_free_stateids;
+
+ INIT_LIST_HEAD(&stateid->ts_id_list);
+ list_add_tail(&stateid->ts_id_list, &test_stateid->ts_stateid_list);
+
+ status = nfsd4_decode_stateid(argp, &stateid->ts_id_stateid);
if (status)
- return status;
+ goto out_free_stateids;
}
status = 0;
out:
return status;
+out_free_stateids:
+ list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list) {
+ list_del(&stateid->ts_id_list);
+ kfree(stateid);
+ }
xdr_error:
dprintk("NFSD: xdr error (%s:%d)\n", __FILE__, __LINE__);
status = nfserr_bad_xdr;
@@ -3391,30 +3380,19 @@ __be32
nfsd4_encode_test_stateid(struct nfsd4_compoundres *resp, int nfserr,
struct nfsd4_test_stateid *test_stateid)
{
- struct nfsd4_compoundargs *argp;
- struct nfs4_client *cl = resp->cstate.session->se_client;
- stateid_t si;
+ struct nfsd4_test_stateid_id *stateid, *next;
__be32 *p;
- int i;
- int valid;
- restore_buf(test_stateid->ts_saved_args, &test_stateid->ts_savedp);
- argp = test_stateid->ts_saved_args;
-
- RESERVE_SPACE(4);
+ RESERVE_SPACE(4 + (4 * test_stateid->ts_num_ids));
*p++ = htonl(test_stateid->ts_num_ids);
- resp->p = p;
- nfs4_lock_state();
- for (i = 0; i < test_stateid->ts_num_ids; i++) {
- nfsd4_decode_stateid(argp, &si);
- valid = nfs4_validate_stateid(cl, &si);
- RESERVE_SPACE(4);
- *p++ = htonl(valid);
- resp->p = p;
+ list_for_each_entry_safe(stateid, next, &test_stateid->ts_stateid_list, ts_id_list) {
+ *p++ = htonl(stateid->ts_id_status);
+ list_del(&stateid->ts_id_list);
+ kfree(stateid);
}
- nfs4_unlock_state();
+ ADJUST_ARGS();
return nfserr;
}
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 2364747..0a667e9 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -343,10 +343,15 @@ struct nfsd4_saved_compoundargs {
struct page **pagelist;
};
+struct nfsd4_test_stateid_id {
+ __be32 ts_id_status;
+ stateid_t ts_id_stateid;
+ struct list_head ts_id_list;
+};
+
struct nfsd4_test_stateid {
__be32 ts_num_ids;
- struct nfsd4_compoundargs *ts_saved_args;
- struct nfsd4_saved_compoundargs ts_savedp;
+ struct list_head ts_stateid_list;
};
struct nfsd4_free_stateid {
--
1.7.8.4
On Thu Jan 26 16:25:54 2012, J. Bruce Fields wrote:
> On Wed, Jan 25, 2012 at 11:48:15AM -0500, [email protected] wrote:
>> From: Bryan Schumaker <[email protected]>
>>
>> When I initially wrote it, I didn't understand how lists worked so I
>> wrote something that didn't use them. I think making a list of stateids
>> to test is a more straightforward implementation, especially compared to
>> especially compared to decoding stateids while simultaneously encoding
>> a reply to the client.
>
> Thanks, yes, I'll be much happier with something like this. The only
> thing I'm worried about:
>
>> static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32 nbytes)
>> {
>> /* We want more bytes than seem to be available.
>> @@ -1385,31 +1369,36 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
>> static __be32
>> nfsd4_decode_test_stateid(struct nfsd4_compoundargs *argp, struct nfsd4_test_stateid *test_stateid)
>> {
>> - unsigned int nbytes;
>> - stateid_t si;
>> int i;
>> - __be32 *p;
>> - __be32 status;
>> + __be32 *p, status;
>> + struct nfsd4_test_stateid_id *stateid;
>>
>> READ_BUF(4);
>> test_stateid->ts_num_ids = ntohl(*p++);
>>
>> - nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
>> - if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
>> - goto xdr_error;
>> -
>> - test_stateid->ts_saved_args = argp;
>> - save_buf(argp, &test_stateid->ts_savedp);
>> + INIT_LIST_HEAD(&test_stateid->ts_stateid_list);
>>
>> for (i = 0; i < test_stateid->ts_num_ids; i++) {
>> - status = nfsd4_decode_stateid(argp, &si);
>> + stateid = kmalloc(sizeof(struct nfsd4_test_stateid_id), GFP_KERNEL);
>
> Hm, are you sure this will always get freed in all cases?
>
> Off the top of my head: the compound gets decode in one fell swoop. The
> we process and encode each op one at a time. So, for example, if we
> fail later in the decoding, or if processing fails on an earlier op than
> this one, then we're not going to hit the free in the encoder:
>
>> nfsd4_encode_test_stateid(struct nfsd4_compoundres *resp, int nfserr,
>> struct nfsd4_test_stateid *test_stateid)
>> {
> ...
>> + list_for_each_entry_safe(stateid, next, &test_stateid->ts_stateid_list, ts_id_list) {
>> + *p++ = htonl(stateid->ts_id_status);
>> + list_del(&stateid->ts_id_list);
>> + kfree(stateid);
>
> Hence the defer_free/release_compoundargs stuff. If you grep through
> nfs4xdr.c for defer_free I think you'll see how it works.
I'll take a look, thanks! I didn't know about the defer_free option,
but using it makes sense here. I'll fix up the patches and send a new
version.
- Bryan
>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 25, 2012 at 11:48:15AM -0500, [email protected] wrote:
> From: Bryan Schumaker <[email protected]>
>
> When I initially wrote it, I didn't understand how lists worked so I
> wrote something that didn't use them. I think making a list of stateids
> to test is a more straightforward implementation, especially compared to
> especially compared to decoding stateids while simultaneously encoding
> a reply to the client.
Thanks, yes, I'll be much happier with something like this. The only
thing I'm worried about:
> static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32 nbytes)
> {
> /* We want more bytes than seem to be available.
> @@ -1385,31 +1369,36 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
> static __be32
> nfsd4_decode_test_stateid(struct nfsd4_compoundargs *argp, struct nfsd4_test_stateid *test_stateid)
> {
> - unsigned int nbytes;
> - stateid_t si;
> int i;
> - __be32 *p;
> - __be32 status;
> + __be32 *p, status;
> + struct nfsd4_test_stateid_id *stateid;
>
> READ_BUF(4);
> test_stateid->ts_num_ids = ntohl(*p++);
>
> - nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
> - if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
> - goto xdr_error;
> -
> - test_stateid->ts_saved_args = argp;
> - save_buf(argp, &test_stateid->ts_savedp);
> + INIT_LIST_HEAD(&test_stateid->ts_stateid_list);
>
> for (i = 0; i < test_stateid->ts_num_ids; i++) {
> - status = nfsd4_decode_stateid(argp, &si);
> + stateid = kmalloc(sizeof(struct nfsd4_test_stateid_id), GFP_KERNEL);
Hm, are you sure this will always get freed in all cases?
Off the top of my head: the compound gets decode in one fell swoop. The
we process and encode each op one at a time. So, for example, if we
fail later in the decoding, or if processing fails on an earlier op than
this one, then we're not going to hit the free in the encoder:
> nfsd4_encode_test_stateid(struct nfsd4_compoundres *resp, int nfserr,
> struct nfsd4_test_stateid *test_stateid)
> {
...
> + list_for_each_entry_safe(stateid, next, &test_stateid->ts_stateid_list, ts_id_list) {
> + *p++ = htonl(stateid->ts_id_status);
> + list_del(&stateid->ts_id_list);
> + kfree(stateid);
Hence the defer_free/release_compoundargs stuff. If you grep through
nfs4xdr.c for defer_free I think you'll see how it works.
--b.