2014-09-02 17:30:43

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH v2 0/2] NFSD: Add v4.2 SEEK support

From: Anna Schumaker <[email protected]>

These patches add server support for the NFS v4.2 operation SEEK. The first
patch adds basic NFS v4.2 infrastructure that all future operations will rely
on. The second patch adds the SEEK operation.

Changes in v2:
- Remove CONFIG_NFSD_V4_2_SEEK.
- Change the call to preprocess_stateid_op() to only use RD_STATE instead of
RD_STATE | WR_STATE.

These patches and the corresponding client patch are avaliable in the [seek]
branch of:

git://git.linux-nfs.org/projects/anna/linux-nfs.git


Questions? Comments? Thoughts?

Anna


Anna Schumaker (2):
NFSD: Add generic v4.2 infrastructure
NFSD: Implement SEEK

fs/nfsd/nfs4proc.c | 43 +++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/xdr4.h | 14 ++++++++++++
include/linux/nfs4.h | 23 ++++++++++++++++++--
4 files changed, 138 insertions(+), 2 deletions(-)

--
2.1.0



2014-09-02 19:10:47

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] NFSD: Add v4.2 SEEK support

On Tue, Sep 02, 2014 at 01:30:36PM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> These patches add server support for the NFS v4.2 operation SEEK. The first
> patch adds basic NFS v4.2 infrastructure that all future operations will rely
> on. The second patch adds the SEEK operation.
>
> Changes in v2:
> - Remove CONFIG_NFSD_V4_2_SEEK.
> - Change the call to preprocess_stateid_op() to only use RD_STATE instead of
> RD_STATE | WR_STATE.
>
> These patches and the corresponding client patch are avaliable in the [seek]
> branch of:
>
> git://git.linux-nfs.org/projects/anna/linux-nfs.git
>
>
> Questions? Comments? Thoughts?

They look OK to me. I'll probably merge them once there's evidence
they're likely to be merged on the client side too.

Also: has anyone done wireshark support for this yet?

--b.

>
> Anna
>
>
> Anna Schumaker (2):
> NFSD: Add generic v4.2 infrastructure
> NFSD: Implement SEEK
>
> fs/nfsd/nfs4proc.c | 43 +++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4xdr.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/xdr4.h | 14 ++++++++++++
> include/linux/nfs4.h | 23 ++++++++++++++++++--
> 4 files changed, 138 insertions(+), 2 deletions(-)
>
> --
> 2.1.0
>
> --
> 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

2014-09-09 17:53:40

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] NFSD: Add generic v4.2 infrastructure

On Tue, Sep 02, 2014 at 01:30:37PM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> It's cleaner to introduce everything at once and have the server reply
> with "not supported" than it would be to introduce extra operations when
> implementing a specific one in the middle of the list.
>
> Signed-off-by: Anna Schumaker <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2014-09-02 17:30:44

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH v2 1/2] NFSD: Add generic v4.2 infrastructure

From: Anna Schumaker <[email protected]>

It's cleaner to introduce everything at once and have the server reply
with "not supported" than it would be to introduce extra operations when
implementing a specific one in the middle of the list.

Signed-off-by: Anna Schumaker <[email protected]>
---
fs/nfsd/nfs4xdr.c | 28 ++++++++++++++++++++++++++++
include/linux/nfs4.h | 18 ++++++++++++++++--
2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f9821ce..94dde7b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1593,6 +1593,20 @@ static nfsd4_dec nfsd4_dec_ops[] = {
[OP_WANT_DELEGATION] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_DESTROY_CLIENTID] = (nfsd4_dec)nfsd4_decode_destroy_clientid,
[OP_RECLAIM_COMPLETE] = (nfsd4_dec)nfsd4_decode_reclaim_complete,
+
+ /* new operations for NFSv4.2 */
+ [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_COPY] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_COPY_NOTIFY] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_IO_ADVISE] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_LAYOUTERROR] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_LAYOUTSTATS] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_OFFLOAD_CANCEL] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_OFFLOAD_STATUS] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_READ_PLUS] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_SEEK] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_WRITE_SAME] = (nfsd4_dec)nfsd4_decode_notsupp,
};

static inline bool
@@ -3822,6 +3836,20 @@ static nfsd4_enc nfsd4_enc_ops[] = {
[OP_WANT_DELEGATION] = (nfsd4_enc)nfsd4_encode_noop,
[OP_DESTROY_CLIENTID] = (nfsd4_enc)nfsd4_encode_noop,
[OP_RECLAIM_COMPLETE] = (nfsd4_enc)nfsd4_encode_noop,
+
+ /* NFSv4.2 operations */
+ [OP_ALLOCATE] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_COPY] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_COPY_NOTIFY] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_DEALLOCATE] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_IO_ADVISE] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_LAYOUTERROR] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_LAYOUTSTATS] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_OFFLOAD_CANCEL] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_OFFLOAD_STATUS] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_READ_PLUS] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_SEEK] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_WRITE_SAME] = (nfsd4_enc)nfsd4_encode_noop,
};

/*
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 79b2a0f..cf38224 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -110,6 +110,20 @@ enum nfs_opnum4 {
OP_DESTROY_CLIENTID = 57,
OP_RECLAIM_COMPLETE = 58,

+ /* nfs42 */
+ OP_ALLOCATE = 59,
+ OP_COPY = 60,
+ OP_COPY_NOTIFY = 61,
+ OP_DEALLOCATE = 62,
+ OP_IO_ADVISE = 63,
+ OP_LAYOUTERROR = 64,
+ OP_LAYOUTSTATS = 65,
+ OP_OFFLOAD_CANCEL = 66,
+ OP_OFFLOAD_STATUS = 67,
+ OP_READ_PLUS = 68,
+ OP_SEEK = 69,
+ OP_WRITE_SAME = 70,
+
OP_ILLEGAL = 10044,
};

@@ -117,10 +131,10 @@ enum nfs_opnum4 {
Needs to be updated if more operations are defined in future.*/

#define FIRST_NFS4_OP OP_ACCESS
-#define LAST_NFS4_OP OP_RECLAIM_COMPLETE
+#define LAST_NFS4_OP OP_WRITE_SAME
#define LAST_NFS40_OP OP_RELEASE_LOCKOWNER
#define LAST_NFS41_OP OP_RECLAIM_COMPLETE
-#define LAST_NFS42_OP OP_RECLAIM_COMPLETE
+#define LAST_NFS42_OP OP_WRITE_SAME

enum nfsstat4 {
NFS4_OK = 0,
--
2.1.0


2014-09-02 17:30:45

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH v2 2/2] NFSD: Implement SEEK

From: Anna Schumaker <[email protected]>

This patch adds server support for the NFS v4.2 operation SEEK, which
returns the position of the next hole or data segment in a file.

Signed-off-by: Anna Schumaker <[email protected]>
---
fs/nfsd/nfs4proc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 36 ++++++++++++++++++++++++++++++++++--
fs/nfsd/xdr4.h | 14 ++++++++++++++
include/linux/nfs4.h | 5 +++++
4 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5e0dc52..aed5b88 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1013,6 +1013,43 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return status;
}

+static __be32
+nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+ struct nfsd4_seek *seek)
+{
+ struct file *file;
+ __be32 status = nfs_ok;
+
+ status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
+ &seek->seek_stateid,
+ RD_STATE, &file);
+ if (status) {
+ dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
+ return status;
+ }
+
+ switch (seek->seek_whence) {
+ case NFS4_CONTENT_DATA:
+ seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_DATA);
+ break;
+ case NFS4_CONTENT_HOLE:
+ seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_HOLE);
+ break;
+ default:
+ status = nfserr_union_notsupp;
+ goto out;
+ }
+
+ if (seek->seek_pos < 0)
+ status = nfserrno(seek->seek_pos);
+ else if (seek->seek_pos >= i_size_read(file_inode(file)))
+ seek->seek_eof = true;
+
+out:
+ fput(file);
+ return status;
+}
+
/* This routine never returns NFS_OK! If there are no other errors, it
* will return NFSERR_SAME or NFSERR_NOT_SAME depending on whether the
* attributes matched. VERIFY is implemented by mapping NFSERR_SAME
@@ -1881,6 +1918,12 @@ static struct nfsd4_operation nfsd4_ops[] = {
.op_get_currentstateid = (stateid_getter)nfsd4_get_freestateid,
.op_rsize_bop = (nfsd4op_rsize)nfsd4_only_status_rsize,
},
+
+ /* NFSv4.2 operations */
+ [OP_SEEK] = {
+ .op_func = (nfsd4op_func)nfsd4_seek,
+ .op_name = "OP_SEEK",
+ },
};

int nfsd4_max_reply(struct svc_rqst *rqstp, struct nfsd4_op *op)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 94dde7b..57b3973 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1521,6 +1521,22 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
}

static __be32
+nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
+{
+ DECODE_HEAD;
+
+ status = nfsd4_decode_stateid(argp, &seek->seek_stateid);
+ if (status)
+ return status;
+
+ READ_BUF(12);
+ p = xdr_decode_hyper(p, &seek->seek_offset);
+ seek->seek_whence = be32_to_cpup(p);
+
+ DECODE_TAIL;
+}
+
+static __be32
nfsd4_decode_noop(struct nfsd4_compoundargs *argp, void *p)
{
return nfs_ok;
@@ -1605,7 +1621,7 @@ static nfsd4_dec nfsd4_dec_ops[] = {
[OP_OFFLOAD_CANCEL] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_OFFLOAD_STATUS] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_READ_PLUS] = (nfsd4_dec)nfsd4_decode_notsupp,
- [OP_SEEK] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_SEEK] = (nfsd4_dec)nfsd4_decode_seek,
[OP_WRITE_SAME] = (nfsd4_dec)nfsd4_decode_notsupp,
};

@@ -3765,6 +3781,22 @@ nfsd4_encode_test_stateid(struct nfsd4_compoundres *resp, __be32 nfserr,
}

static __be32
+nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
+ struct nfsd4_seek *seek)
+{
+ __be32 *p;
+
+ if (nfserr)
+ return nfserr;
+
+ p = xdr_reserve_space(&resp->xdr, 12);
+ *p++ = cpu_to_be32(seek->seek_eof);
+ p = xdr_encode_hyper(p, seek->seek_pos);
+
+ return nfserr;
+}
+
+static __be32
nfsd4_encode_noop(struct nfsd4_compoundres *resp, __be32 nfserr, void *p)
{
return nfserr;
@@ -3848,7 +3880,7 @@ static nfsd4_enc nfsd4_enc_ops[] = {
[OP_OFFLOAD_CANCEL] = (nfsd4_enc)nfsd4_encode_noop,
[OP_OFFLOAD_STATUS] = (nfsd4_enc)nfsd4_encode_noop,
[OP_READ_PLUS] = (nfsd4_enc)nfsd4_encode_noop,
- [OP_SEEK] = (nfsd4_enc)nfsd4_encode_noop,
+ [OP_SEEK] = (nfsd4_enc)nfsd4_encode_seek,
[OP_WRITE_SAME] = (nfsd4_enc)nfsd4_encode_noop,
};

diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 465e779..5720e94 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -428,6 +428,17 @@ struct nfsd4_reclaim_complete {
u32 rca_one_fs;
};

+struct nfsd4_seek {
+ /* request */
+ stateid_t seek_stateid;
+ loff_t seek_offset;
+ u32 seek_whence;
+
+ /* response */
+ u32 seek_eof;
+ loff_t seek_pos;
+};
+
struct nfsd4_op {
int opnum;
__be32 status;
@@ -473,6 +484,9 @@ struct nfsd4_op {
struct nfsd4_reclaim_complete reclaim_complete;
struct nfsd4_test_stateid test_stateid;
struct nfsd4_free_stateid free_stateid;
+
+ /* NFSv4.2 */
+ struct nfsd4_seek seek;
} u;
struct nfs4_replay * replay;
};
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index cf38224..026b0c0 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -550,4 +550,9 @@ struct nfs4_deviceid {
char data[NFS4_DEVICEID4_SIZE];
};

+enum data_content4 {
+ NFS4_CONTENT_DATA = 0,
+ NFS4_CONTENT_HOLE = 1,
+};
+
#endif
--
2.1.0


2014-09-09 19:17:51

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] NFSD: Implement SEEK

On 09/09/2014 01:56 PM, Christoph Hellwig wrote:
>> + switch (seek->seek_whence) {
>> + case NFS4_CONTENT_DATA:
>> + seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_DATA);
>> + break;
>> + case NFS4_CONTENT_HOLE:
>> + seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_HOLE);
>> + break;
>> + default:
>> + status = nfserr_union_notsupp;
>> + goto out;
>> + }
>
> nipick: maybe just assign pos in the switch, and have a single
> vfs_llseek call?
>
> Also this might want a comment that vfs_llseek is changing file->f_pos,
> but nothing in NFSD should ever rely on file->f_pos.

Sure.

>
>> static __be32
>> +nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
>> + struct nfsd4_seek *seek)
>> +{
>> + __be32 *p;
>> +
>> + if (nfserr)
>> + return nfserr;
>> +
>> + p = xdr_reserve_space(&resp->xdr, 12);
>
> nipick: can you replace the 12 by a "4 + 8"? I think having one
> literal for each field later encoded helps reading the XDR code a lot.
> And although it might not matter for a trivial encoder like this it
> set standards for future copy and paste code.

I can change that, too. Thanks for looking!

Anna
>


2014-09-22 17:53:36

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] NFSD: Add v4.2 SEEK support

On Tue, Sep 02, 2014 at 03:10:43PM -0400, J. Bruce Fields wrote:
> On Tue, Sep 02, 2014 at 01:30:36PM -0400, [email protected] wrote:
> > From: Anna Schumaker <[email protected]>
> >
> > These patches add server support for the NFS v4.2 operation SEEK. The first
> > patch adds basic NFS v4.2 infrastructure that all future operations will rely
> > on. The second patch adds the SEEK operation.
> >
> > Changes in v2:
> > - Remove CONFIG_NFSD_V4_2_SEEK.
> > - Change the call to preprocess_stateid_op() to only use RD_STATE instead of
> > RD_STATE | WR_STATE.
> >
> > These patches and the corresponding client patch are avaliable in the [seek]
> > branch of:
> >
> > git://git.linux-nfs.org/projects/anna/linux-nfs.git
> >
> >
> > Questions? Comments? Thoughts?
>
> They look OK to me. I'll probably merge them once there's evidence
> they're likely to be merged on the client side too.
>
> Also: has anyone done wireshark support for this yet?

Are you sending a new version of these?

(And ditto for the allocate patches.)

As far as I can tell review comments were mostly nits.

--b.

2014-09-09 17:56:08

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] NFSD: Implement SEEK

> + switch (seek->seek_whence) {
> + case NFS4_CONTENT_DATA:
> + seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_DATA);
> + break;
> + case NFS4_CONTENT_HOLE:
> + seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_HOLE);
> + break;
> + default:
> + status = nfserr_union_notsupp;
> + goto out;
> + }

nipick: maybe just assign pos in the switch, and have a single
vfs_llseek call?

Also this might want a comment that vfs_llseek is changing file->f_pos,
but nothing in NFSD should ever rely on file->f_pos.

> static __be32
> +nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
> + struct nfsd4_seek *seek)
> +{
> + __be32 *p;
> +
> + if (nfserr)
> + return nfserr;
> +
> + p = xdr_reserve_space(&resp->xdr, 12);

nipick: can you replace the 12 by a "4 + 8"? I think having one
literal for each field later encoded helps reading the XDR code a lot.
And although it might not matter for a trivial encoder like this it
set standards for future copy and paste code.