Re: [PATCH 3/4] NFSD: Add WRITE_PLUS support for hole punches

[Anna Schumaker <bjschuma@netapp.com>]

On Mon 28 Oct 2013 05:40:30 PM EDT, J. Bruce Fields wrote:
> On Mon, Oct 28, 2013 at 10:57:25AM -0400, Anna Schumaker wrote:
>> This patch only implements DATA_CONTENT_HOLE support, but I tried to
>> write it so other arms of the discriminated union can be added easily
>> later.
>
> And there's an NFS4ERR_UNION_NOTSUPP to handle that case, OK.  (Though
> the spec language is a little ambiguous:
>
> 	If the client asks for a hole and the server does not support
> 	that arm of the discriminated union, but does support one or
> 	more additional arms, it can signal to the client that it
> 	supports the operation, but not the arm with
> 	NFS4ERR_UNION_NOTSUPP.
>
> So it's unclear whether we can return this in the case the client is
> *not* asking for a hole.  Are we sure the NFS4_CONTENT_DATA case is
> optional?  The language in 5.3 ("Instead a client should utlize
> READ_PLUS and WRITE_PLUS") also suggests the CONTENT_DATA case might be
> meant to be mandatory.)
>
>> This patch is missing support for decoding multiple requests on the same
>> file.  The way it's written now only the last range provided by the
>> client will be written to.
>
> But that doesn't seem to be a limitation anticipated by the spec, so I
> guess we need to fix that.
>
> (Why is there an array, anyway?  Wouldn't it work just as well to send
> multiple RPC's, or a single RPC with multiple WRITE_PLUS ops?)
>
>>
>> Signed-off-by: Anna Schumaker <bjschuma@netapp.com>
>> ---
>>  fs/nfsd/nfs4proc.c   | 45 ++++++++++++++++++++++++++++++++++++++
>>  fs/nfsd/nfs4xdr.c    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  fs/nfsd/vfs.c        | 14 ++++++++++++
>>  fs/nfsd/vfs.h        |  1 +
>>  fs/nfsd/xdr4.h       | 24 ++++++++++++++++++++
>>  fs/open.c            |  1 +
>>  include/linux/nfs4.h |  8 ++++++-
>>  7 files changed, 152 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 419572f..3210c6f 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1028,6 +1028,42 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  	return status;
>>  }
>>
>> +static __be32
>> +nfsd4_write_plus_hole(struct file *file, struct nfsd4_write_plus *writeplus,
>> +		      struct net *net)
>> +{
>> +	__be32 status;
>> +
>> +	status = nfsd4_vfs_fallocate(file, writeplus->wp_allocated,
>> +				writeplus->wp_offset, writeplus->wp_length);
>> +	if (status == nfs_ok) {
>> +		writeplus->wp_res.wr_stid = NULL;
>> +		writeplus->wp_res.wr_bytes_written = writeplus->wp_length;
>> +		writeplus->wp_res.wr_stable_how = NFS_FILE_SYNC;
>
> Do we need to sync?

I did the sync in nfsd4_vfs_fallocate (below), but I can move it if 
that would make more sense.
>
>> +		gen_boot_verifier(&writeplus->wp_res.wr_verifier, net);
>> +	}
>> +
>> +	return status;
>
> Nit, but I like the usual idiom better in most cases:
>
> 	if (status)
> 		return status;
> 	normal case here...
> 	return 0;

Sure, I'll change that.

>
> --b.
>
>> +}
>> +
>> +static __be32
>> +nfsd4_write_plus(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> +		struct nfsd4_write_plus *writeplus)
>> +{
>> +	struct file *file;
>> +	__be32 status;
>> +	struct net *net = SVC_NET(rqstp);
>> +
>> +	status = nfs4_preprocess_stateid_op(net, cstate, &writeplus->wp_stateid,
>> +					    WR_STATE, &file);
>> +	if (status)
>> +		return status;
>> +
>> +	if (writeplus->wp_data_content == NFS4_CONTENT_HOLE)
>> +		return nfsd4_write_plus_hole(file, writeplus, net);
>> +	return nfserr_union_notsupp;
>> +}
>> +
>>  /* 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
>> @@ -1840,6 +1876,15 @@ 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_WRITE_PLUS] = {
>> +		.op_func = (nfsd4op_func)nfsd4_write_plus,
>> +		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
>> +		.op_name = "OP_WRITE_PLUS",
>> +		.op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
>> +		.op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
>> +	},
>>  };
>>
>>  #ifdef NFSD_DEBUG
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 60f5a1f..1e4e9af 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1485,6 +1485,32 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>>  }
>>
>>  static __be32
>> +nfsd4_decode_write_plus(struct nfsd4_compoundargs *argp,
>> +			struct nfsd4_write_plus *writeplus)
>> +{
>> +	DECODE_HEAD;
>> +	unsigned int i;
>> +
>> +	status = nfsd4_decode_stateid(argp, &writeplus->wp_stateid);
>> +	if (status)
>> +		return status;
>> +
>> +	READ_BUF(8);
>> +	READ32(writeplus->wp_stable_how);
>> +	READ32(writeplus->wp_data_length);
>> +
>> +	for (i = 0; i < writeplus->wp_data_length; i++) {
>> +		READ_BUF(24);
>> +		READ32(writeplus->wp_data_content);
>> +		READ64(writeplus->wp_offset);
>> +		READ64(writeplus->wp_length);
>> +		READ32(writeplus->wp_allocated);
>> +	}
>> +
>> +	DECODE_TAIL;
>> +}
>> +
>> +static __be32
>>  nfsd4_decode_noop(struct nfsd4_compoundargs *argp, void *p)
>>  {
>>  	return nfs_ok;
>> @@ -1665,7 +1691,7 @@ static nfsd4_dec nfsd42_dec_ops[] = {
>>  	[OP_COPY_NOTIFY]	= (nfsd4_dec)nfsd4_decode_notsupp,
>>  	[OP_OFFLOAD_REVOKE]	= (nfsd4_dec)nfsd4_decode_notsupp,
>>  	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_notsupp,
>> -	[OP_WRITE_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
>> +	[OP_WRITE_PLUS]		= (nfsd4_dec)nfsd4_decode_write_plus,
>>  	[OP_READ_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
>>  	[OP_SEEK]		= (nfsd4_dec)nfsd4_decode_notsupp,
>>  	[OP_IO_ADVISE]		= (nfsd4_dec)nfsd4_decode_notsupp,
>> @@ -3591,6 +3617,38 @@ nfsd4_encode_test_stateid(struct nfsd4_compoundres *resp, __be32 nfserr,
>>  	return nfserr;
>>  }
>>
>> +static void
>> +nfsd42_encode_write_res(struct nfsd4_compoundres *resp, struct nfsd42_write_res *write)
>> +{
>> +	__be32 *p;
>> +
>> +	RESERVE_SPACE(4);
>> +
>> +	if (write->wr_stid == NULL) {
>> +		WRITE32(0);
>> +		ADJUST_ARGS();
>> +	} else {
>> +		WRITE32(1);
>> +		ADJUST_ARGS();
>> +		nfsd4_encode_stateid(resp, &write->wr_stid->sc_stateid);
>> +	}
>> +
>> +	RESERVE_SPACE(12 + NFS4_VERIFIER_SIZE);
>> +	WRITE64(write->wr_bytes_written);
>> +	WRITE32(write->wr_stable_how);
>> +	WRITEMEM(write->wr_verifier.data, NFS4_VERIFIER_SIZE);
>> +	ADJUST_ARGS();
>> +}
>> +
>> +static __be32
>> +nfsd4_encode_write_plus(struct nfsd4_compoundres *resp, __be32 nfserr,
>> +		        struct nfsd4_write_plus *writeplus)
>> +{
>> +	if (!nfserr)
>> +		nfsd42_encode_write_res(resp, &writeplus->wp_res);
>> +	return nfserr;
>> +}
>> +
>>  static __be32
>>  nfsd4_encode_noop(struct nfsd4_compoundres *resp, __be32 nfserr, void *p)
>>  {
>> @@ -3670,7 +3728,7 @@ static nfsd4_enc nfsd4_enc_ops[] = {
>>  	[OP_COPY_NOTIFY]	= (nfsd4_enc)nfsd4_encode_noop,
>>  	[OP_OFFLOAD_REVOKE]	= (nfsd4_enc)nfsd4_encode_noop,
>>  	[OP_OFFLOAD_STATUS]	= (nfsd4_enc)nfsd4_encode_noop,
>> -	[OP_WRITE_PLUS]		= (nfsd4_enc)nfsd4_encode_noop,
>> +	[OP_WRITE_PLUS]		= (nfsd4_enc)nfsd4_encode_write_plus,
>>  	[OP_READ_PLUS]		= (nfsd4_enc)nfsd4_encode_noop,
>>  	[OP_SEEK]		= (nfsd4_enc)nfsd4_encode_noop,
>>  	[OP_IO_ADVISE]		= (nfsd4_enc)nfsd4_encode_noop,
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index c827acb..7fec087 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/file.h>
>>  #include <linux/splice.h>
>> +#include <linux/falloc.h>
>>  #include <linux/fcntl.h>
>>  #include <linux/namei.h>
>>  #include <linux/delay.h>
>> @@ -649,6 +650,19 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>  }
>>  #endif
>>
>> +__be32 nfsd4_vfs_fallocate(struct file *file, bool allocated, loff_t offset, loff_t len)
>> +{
>> +	int error, mode = 0;
>> +
>> +	if (allocated == false)
>> +		mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
>> +
>> +	error = do_fallocate(file, mode, offset, len);
>> +	if (error == 0)
>> +		error = vfs_fsync_range(file, offset, offset + len, 0);
>> +	return nfserrno(error);
>> +}
>> +
>>  #endif /* defined(CONFIG_NFSD_V4) */
>>
>>  #ifdef CONFIG_NFSD_V3
>> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
>> index a4be2e3..187eb95 100644
>> --- a/fs/nfsd/vfs.h
>> +++ b/fs/nfsd/vfs.h
>> @@ -57,6 +57,7 @@ __be32          nfsd4_set_nfs4_acl(struct svc_rqst *, struct svc_fh *,
>>  int             nfsd4_get_nfs4_acl(struct svc_rqst *, struct dentry *, struct nfs4_acl **);
>>  __be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
>>  		    struct xdr_netobj *);
>> +__be32		nfsd4_vfs_fallocate(struct file *, bool, loff_t, loff_t);
>>  #endif /* CONFIG_NFSD_V4 */
>>  __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
>>  				char *name, int len, struct iattr *attrs,
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index b3ed644..aaef9ab 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -430,6 +430,27 @@ struct nfsd4_reclaim_complete {
>>  	u32 rca_one_fs;
>>  };
>>
>> +struct nfsd42_write_res {
>> +	struct nfs4_stid	*wr_stid;
>> +	u64			wr_bytes_written;
>> +	u32			wr_stable_how;
>> +	nfs4_verifier		wr_verifier;
>> +};
>> +
>> +struct nfsd4_write_plus {
>> +	/* request */
>> +	stateid_t	wp_stateid;
>> +	u32		wp_stable_how;
>> +	u32		wp_data_length;
>> +	u32		wp_data_content;
>> +	u64		wp_offset;
>> +	u64		wp_length;
>> +	u32		wp_allocated;
>> +
>> +	/* response */
>> +	struct nfsd42_write_res	wp_res;
>> +};
>> +
>>  struct nfsd4_op {
>>  	int					opnum;
>>  	__be32					status;
>> @@ -475,6 +496,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_write_plus		write_plus;
>>  	} u;
>>  	struct nfs4_replay *			replay;
>>  };
>> diff --git a/fs/open.c b/fs/open.c
>> index d420331..09db2d5 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -278,6 +278,7 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>  	sb_end_write(inode->i_sb);
>>  	return ret;
>>  }
>> +EXPORT_SYMBOL_GPL(do_fallocate);
>>
>>  SYSCALL_DEFINE4(fallocate, int, fd, int, mode, loff_t, offset, loff_t, len)
>>  {
>> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
>> index 2bc5217..55ed991 100644
>> --- a/include/linux/nfs4.h
>> +++ b/include/linux/nfs4.h
>> @@ -128,7 +128,7 @@ 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_PLUS
>>
>>  enum nfsstat4 {
>>  	NFS4_OK = 0,
>> @@ -552,4 +552,10 @@ struct nfs4_deviceid {
>>  	char data[NFS4_DEVICEID4_SIZE];
>>  };
>>
>> +enum data_content4 {
>> +	NFS4_CONTENT_DATA		= 0,
>> +	NFS4_CONTENT_APP_DATA_HOLE	= 1,
>> +	NFS4_CONTENT_HOLE		= 2,
>> +};
>> +
>>  #endif
>> --
>> 1.8.4.1
>>