On Mon, May 8, 2017 at 4:31 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
> On Tue, 2017-04-18 at 12:09 -0400, Olga Kornievskaia wrote:
>> Instead of messing with the commit path which has been causing
>> issues,
>> add a COMMIT op after the COPY and ask for stable copies in the first
>> space.
>>
>> It saves a round trip, since after the COPY, the client sends a
>> COMMIT
>> anyway.
>>
>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
>> ---
>> fs/nfs/internal.h | 1 -
>> fs/nfs/nfs42proc.c | 6 ------
>> fs/nfs/nfs42xdr.c | 19 +++++++++++++++++--
>> fs/nfs/write.c | 30 ------------------------------
>> 4 files changed, 17 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>> index 7b38fed..2989418 100644
>> --- a/fs/nfs/internal.h
>> +++ b/fs/nfs/internal.h
>> @@ -495,7 +495,6 @@ void nfs_mark_request_commit(struct nfs_page
>> *req,
>> u32 ds_commit_idx);
>> int nfs_write_need_commit(struct nfs_pgio_header *);
>> void nfs_writeback_update_inode(struct nfs_pgio_header *hdr);
>> -int nfs_commit_file(struct file *file, struct nfs_write_verifier
>> *verf);
>> int nfs_generic_commit_list(struct inode *inode, struct list_head
>> *head,
>> int how, struct nfs_commit_info *cinfo);
>> void nfs_retry_commit(struct list_head *page_list,
>> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
>> index 1e486c7..9fee48d 100644
>> --- a/fs/nfs/nfs42proc.c
>> +++ b/fs/nfs/nfs42proc.c
>> @@ -174,12 +174,6 @@ static ssize_t _nfs42_proc_copy(struct file
>> *src,
>> if (status)
>> return status;
>>
>> - if (res->write_res.verifier.committed != NFS_FILE_SYNC) {
>> - status = nfs_commit_file(dst, &res-
>> >write_res.verifier.verifier);
>> - if (status)
>> - return status;
>> - }
>> -
>> truncate_pagecache_range(dst_inode, pos_dst,
>> pos_dst + res->write_res.count);
>>
>> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
>> index 6c72964..8bdf1d5 100644
>> --- a/fs/nfs/nfs42xdr.c
>> +++ b/fs/nfs/nfs42xdr.c
>> @@ -66,12 +66,14 @@
>> encode_putfh_maxsz + \
>> encode_savefh_maxsz + \
>> encode_putfh_maxsz + \
>> - encode_copy_maxsz)
>> + encode_copy_maxsz + \
>> + encode_commit_maxsz)
>> #define NFS4_dec_copy_sz (compound_decode_hdr_maxsz +
>> \
>> decode_putfh_maxsz + \
>> decode_savefh_maxsz + \
>> decode_putfh_maxsz + \
>> - decode_copy_maxsz)
>> + decode_copy_maxsz + \
>> + decode_commit_maxsz)
>> #define NFS4_enc_deallocate_sz (compound_encode_hdr_m
>> axsz + \
>> encode_putfh_maxsz + \
>> encode_deallocate_maxsz + \
>> @@ -222,6 +224,18 @@ static void nfs4_xdr_enc_allocate(struct
>> rpc_rqst *req,
>> encode_nops(&hdr);
>> }
>>
>> +static void encode_copy_commit(struct xdr_stream *xdr,
>> + struct nfs42_copy_args *args,
>> + struct compound_hdr *hdr)
>> +{
>> + __be32 *p;
>> +
>> + encode_op_hdr(xdr, OP_COMMIT, decode_commit_maxsz, hdr);
>> + p = reserve_space(xdr, 12);
>> + p = xdr_encode_hyper(p, args->dst_pos);
>> + *p = cpu_to_be32(args->count);
>> +}
>> +
>> /*
>> * Encode COPY request
>> */
>> @@ -239,6 +253,7 @@ static void nfs4_xdr_enc_copy(struct rpc_rqst
>> *req,
>> encode_savefh(xdr, &hdr);
>> encode_putfh(xdr, args->dst_fh, &hdr);
>> encode_copy(xdr, args, &hdr);
>> + encode_copy_commit(xdr, args, &hdr);
>> encode_nops(&hdr);
>> }
>>
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index abb2c8a..8d65d88 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -1720,36 +1720,6 @@ void nfs_retry_commit(struct list_head
>> *page_list,
>> return -ENOMEM;
>> }
>>
>> -int nfs_commit_file(struct file *file, struct nfs_write_verifier
>> *verf)
>> -{
>> - struct inode *inode = file_inode(file);
>> - struct nfs_open_context *open;
>> - struct nfs_commit_info cinfo;
>> - struct nfs_page *req;
>> - int ret;
>> -
>> - open = get_nfs_open_context(nfs_file_open_context(file));
>> - req = nfs_create_request(open, NULL, NULL, 0,
>> i_size_read(inode));
>> - if (IS_ERR(req)) {
>> - ret = PTR_ERR(req);
>> - goto out_put;
>> - }
>> -
>> - nfs_init_cinfo_from_inode(&cinfo, inode);
>> -
>> - memcpy(&req->wb_verf, verf, sizeof(struct
>> nfs_write_verifier));
>> - nfs_request_add_commit_list(req, &cinfo);
>> - ret = nfs_commit_inode(inode, FLUSH_SYNC);
>> - if (ret > 0)
>> - ret = 0;
>> -
>> - nfs_free_request(req);
>> -out_put:
>> - put_nfs_open_context(open);
>> - return ret;
>> -}
>> -EXPORT_SYMBOL_GPL(nfs_commit_file);
>> -
>> /*
>> * COMMIT call returned
>> */
>
> Shouldn't we decode the reply from the server so that we can return an
> error message or retry if the commit failed and/or the verifiers don't
> match? Otherwise we won't notice if the flush to disk failed.
Good point. Let me see what I can come up with.
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@xxxxxxxxxxxxxxx
--
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
