LinuxLists.cc - [PATCH 1/1] NFS append COMMIT after synchronous COPY

2017-04-17 21:15:40

Subject: [PATCH 1/1] NFS append COMMIT after synchronous COPY

2017-04-18 15:04:02

Subject: Re: [PATCH 1/1] NFS append COMMIT after synchronous COPY

Hi Olga,

On 04/17/2017 05:15 PM, 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 <[email protected]>
> ---
> fs/nfs/internal.h | 1 -
> fs/nfs/nfs42proc.c | 6 ------
> fs/nfs/nfs42xdr.c | 15 +++++++++++++++
> fs/nfs/write.c | 30 ------------------------------
> 4 files changed, 15 insertions(+), 37 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..03f3c9e 100644
> --- a/fs/nfs/nfs42xdr.c
> +++ b/fs/nfs/nfs42xdr.c
> @@ -25,6 +25,8 @@
> NFS42_WRITE_RES_SIZE + \
> 1 /* cr_consecutive */ + \
> 1 /* cr_synchronous */)
> +#define decode_commit_maxsz (op_decode_hdr_maxsz + \
> + decode_verifier_maxsz)e

nfs4xdr.c includes this file (it's a weird setup, I know), so this and the corresponding encode size should already be declared and available to us. Can you add them to the NFS4_enc_copy_sz and NFS4_dec_copy_size values?

Thanks,
Anna

> #define encode_deallocate_maxsz (op_encode_hdr_maxsz + \
> encode_fallocate_maxsz)
> #define decode_deallocate_maxsz (op_decode_hdr_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
> */
>

2017-04-18 16:07:06

by Olga Kornievskaia

[permalink] [raw]

Subject: Re: [PATCH 1/1] NFS append COMMIT after synchronous COPY

> On Apr 18, 2017, at 11:03 AM, Anna Schumaker =
<[email protected]> wrote:
>=20
> Hi Olga,
>=20
> On 04/17/2017 05:15 PM, 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.
>>=20
>> It saves a round trip, since after the COPY, the client sends a =
COMMIT
>> anyway.
>>=20
>> Signed-off-by: Olga Kornievskaia <[email protected]>
>> ---
>> fs/nfs/internal.h | 1 -
>> fs/nfs/nfs42proc.c | 6 ------
>> fs/nfs/nfs42xdr.c | 15 +++++++++++++++
>> fs/nfs/write.c | 30 ------------------------------
>> 4 files changed, 15 insertions(+), 37 deletions(-)
>>=20
>> 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;
>>=20
>> - if (res->write_res.verifier.committed !=3D NFS_FILE_SYNC) {
>> - status =3D 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);
>>=20
>> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
>> index 6c72964..03f3c9e 100644
>> --- a/fs/nfs/nfs42xdr.c
>> +++ b/fs/nfs/nfs42xdr.c
>> @@ -25,6 +25,8 @@
>> NFS42_WRITE_RES_SIZE + \
>> 1 /* cr_consecutive */ + \
>> 1 /* cr_synchronous */)
>> +#define decode_commit_maxsz (op_decode_hdr_maxsz + \
>> + decode_verifier_maxsz)e
>=20
> nfs4xdr.c includes this file (it's a weird setup, I know), so this and =
the corresponding encode size should already be declared and available =
to us. Can you add them to the NFS4_enc_copy_sz and NFS4_dec_copy_size =
values?

Got it. Thanks. Another version commit up.

>=20
> Thanks,
> Anna
>=20
>> #define encode_deallocate_maxsz (op_encode_hdr_maxsz + \
>> encode_fallocate_maxsz)
>> #define decode_deallocate_maxsz (op_decode_hdr_maxsz)
>> @@ -222,6 +224,18 @@ static void nfs4_xdr_enc_allocate(struct =
rpc_rqst *req,
>> encode_nops(&hdr);
>> }
>>=20
>> +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 =3D reserve_space(xdr, 12);
>> + p =3D xdr_encode_hyper(p, args->dst_pos);
>> + *p =3D 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);
>> }
>>=20
>> 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;
>> }
>>=20
>> -int nfs_commit_file(struct file *file, struct nfs_write_verifier =
*verf)
>> -{
>> - struct inode *inode =3D file_inode(file);
>> - struct nfs_open_context *open;
>> - struct nfs_commit_info cinfo;
>> - struct nfs_page *req;
>> - int ret;
>> -
>> - open =3D get_nfs_open_context(nfs_file_open_context(file));
>> - req =3D nfs_create_request(open, NULL, NULL, 0, =
i_size_read(inode));
>> - if (IS_ERR(req)) {
>> - ret =3D 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 =3D nfs_commit_inode(inode, FLUSH_SYNC);
>> - if (ret > 0)
>> - ret =3D 0;
>> -
>> - nfs_free_request(req);
>> -out_put:
>> - put_nfs_open_context(open);
>> - return ret;
>> -}
>> -EXPORT_SYMBOL_GPL(nfs_commit_file);
>> -
>> /*
>> * COMMIT call returned
>> */
>>=20

2017-04-18 16:09:44

by Olga Kornievskaia

[permalink] [raw]

Subject: [PATCH 1/1] NFS append COMMIT after synchronous COPY

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 <[email protected]>
---
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_maxsz + \
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
*/
--
1.8.3.1

2017-05-08 20:31:30

by Trond Myklebust

[permalink] [raw]

Subject: Re: [PATCH 1/1] NFS append COMMIT after synchronous COPY

T24gVHVlLCAyMDE3LTA0LTE4IGF0IDEyOjA5IC0wNDAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90
ZToNCj4gSW5zdGVhZCBvZiBtZXNzaW5nIHdpdGggdGhlIGNvbW1pdCBwYXRoIHdoaWNoIGhhcyBi
ZWVuIGNhdXNpbmcNCj4gaXNzdWVzLA0KPiBhZGQgYSBDT01NSVQgb3AgYWZ0ZXIgdGhlIENPUFkg
YW5kIGFzayBmb3Igc3RhYmxlIGNvcGllcyBpbiB0aGUgZmlyc3QNCj4gc3BhY2UuDQo+IA0KPiBJ
dCBzYXZlcyBhIHJvdW5kIHRyaXAsIHNpbmNlIGFmdGVyIHRoZSBDT1BZLCB0aGUgY2xpZW50IHNl
bmRzIGENCj4gQ09NTUlUDQo+IGFueXdheS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IE9sZ2EgS29y
bmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPg0KPiAtLS0NCj4gwqBmcy9uZnMvaW50ZXJuYWwu
aMKgwqB8wqDCoDEgLQ0KPiDCoGZzL25mcy9uZnM0MnByb2MuYyB8wqDCoDYgLS0tLS0tDQo+IMKg
ZnMvbmZzL25mczQyeGRyLmPCoMKgfCAxOSArKysrKysrKysrKysrKysrKy0tDQo+IMKgZnMvbmZz
L3dyaXRlLmPCoMKgwqDCoMKgfCAzMCAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4g
wqA0IGZpbGVzIGNoYW5nZWQsIDE3IGluc2VydGlvbnMoKyksIDM5IGRlbGV0aW9ucygtKQ0KPiAN
Cj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9pbnRlcm5hbC5oIGIvZnMvbmZzL2ludGVybmFsLmgNCj4g
aW5kZXggN2IzOGZlZC4uMjk4OTQxOCAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL2ludGVybmFsLmgN
Cj4gKysrIGIvZnMvbmZzL2ludGVybmFsLmgNCj4gQEAgLTQ5NSw3ICs0OTUsNiBAQCB2b2lkIG5m
c19tYXJrX3JlcXVlc3RfY29tbWl0KHN0cnVjdCBuZnNfcGFnZQ0KPiAqcmVxLA0KPiDCoAkJCcKg
wqDCoMKgwqB1MzIgZHNfY29tbWl0X2lkeCk7DQo+IMKgaW50IG5mc193cml0ZV9uZWVkX2NvbW1p
dChzdHJ1Y3QgbmZzX3BnaW9faGVhZGVyICopOw0KPiDCoHZvaWQgbmZzX3dyaXRlYmFja191cGRh
dGVfaW5vZGUoc3RydWN0IG5mc19wZ2lvX2hlYWRlciAqaGRyKTsNCj4gLWludCBuZnNfY29tbWl0
X2ZpbGUoc3RydWN0IGZpbGUgKmZpbGUsIHN0cnVjdCBuZnNfd3JpdGVfdmVyaWZpZXINCj4gKnZl
cmYpOw0KPiDCoGludCBuZnNfZ2VuZXJpY19jb21taXRfbGlzdChzdHJ1Y3QgaW5vZGUgKmlub2Rl
LCBzdHJ1Y3QgbGlzdF9oZWFkDQo+ICpoZWFkLA0KPiDCoAkJCcKgwqDCoMKgaW50IGhvdywgc3Ry
dWN0IG5mc19jb21taXRfaW5mbyAqY2luZm8pOw0KPiDCoHZvaWQgbmZzX3JldHJ5X2NvbW1pdChz
dHJ1Y3QgbGlzdF9oZWFkICpwYWdlX2xpc3QsDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNDJw
cm9jLmMgYi9mcy9uZnMvbmZzNDJwcm9jLmMNCj4gaW5kZXggMWU0ODZjNy4uOWZlZTQ4ZCAxMDA2
NDQNCj4gLS0tIGEvZnMvbmZzL25mczQycHJvYy5jDQo+ICsrKyBiL2ZzL25mcy9uZnM0MnByb2Mu
Yw0KPiBAQCAtMTc0LDEyICsxNzQsNiBAQCBzdGF0aWMgc3NpemVfdCBfbmZzNDJfcHJvY19jb3B5
KHN0cnVjdCBmaWxlDQo+ICpzcmMsDQo+IMKgCWlmIChzdGF0dXMpDQo+IMKgCQlyZXR1cm4gc3Rh
dHVzOw0KPiDCoA0KPiAtCWlmIChyZXMtPndyaXRlX3Jlcy52ZXJpZmllci5jb21taXR0ZWQgIT0g
TkZTX0ZJTEVfU1lOQykgew0KPiAtCQlzdGF0dXMgPSBuZnNfY29tbWl0X2ZpbGUoZHN0LCAmcmVz
LQ0KPiA+d3JpdGVfcmVzLnZlcmlmaWVyLnZlcmlmaWVyKTsNCj4gLQkJaWYgKHN0YXR1cykNCj4g
LQkJCXJldHVybiBzdGF0dXM7DQo+IC0JfQ0KPiAtDQo+IMKgCXRydW5jYXRlX3BhZ2VjYWNoZV9y
YW5nZShkc3RfaW5vZGUsIHBvc19kc3QsDQo+IMKgCQkJCcKgcG9zX2RzdCArIHJlcy0+d3JpdGVf
cmVzLmNvdW50KTsNCj4gwqANCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0Mnhkci5jIGIvZnMv
bmZzL25mczQyeGRyLmMNCj4gaW5kZXggNmM3Mjk2NC4uOGJkZjFkNSAxMDA2NDQNCj4gLS0tIGEv
ZnMvbmZzL25mczQyeGRyLmMNCj4gKysrIGIvZnMvbmZzL25mczQyeGRyLmMNCj4gQEAgLTY2LDEy
ICs2NiwxNCBAQA0KPiDCoAkJCQkJwqBlbmNvZGVfcHV0ZmhfbWF4c3ogKyBcDQo+IMKgCQkJCQnC
oGVuY29kZV9zYXZlZmhfbWF4c3ogKyBcDQo+IMKgCQkJCQnCoGVuY29kZV9wdXRmaF9tYXhzeiAr
IFwNCj4gLQkJCQkJwqBlbmNvZGVfY29weV9tYXhzeikNCj4gKwkJCQkJwqBlbmNvZGVfY29weV9t
YXhzeiArIFwNCj4gKwkJCQkJwqBlbmNvZGVfY29tbWl0X21heHN6KQ0KPiDCoCNkZWZpbmUgTkZT
NF9kZWNfY29weV9zegkJKGNvbXBvdW5kX2RlY29kZV9oZHJfbWF4c3ogKw0KPiBcDQo+IMKgCQkJ
CQnCoGRlY29kZV9wdXRmaF9tYXhzeiArIFwNCj4gwqAJCQkJCcKgZGVjb2RlX3NhdmVmaF9tYXhz
eiArIFwNCj4gwqAJCQkJCcKgZGVjb2RlX3B1dGZoX21heHN6ICsgXA0KPiAtCQkJCQnCoGRlY29k
ZV9jb3B5X21heHN6KQ0KPiArCQkJCQnCoGRlY29kZV9jb3B5X21heHN6ICsgXA0KPiArCQkJCQnC
oGRlY29kZV9jb21taXRfbWF4c3opDQo+IMKgI2RlZmluZSBORlM0X2VuY19kZWFsbG9jYXRlX3N6
CQkoY29tcG91bmRfZW5jb2RlX2hkcl9tDQo+IGF4c3ogKyBcDQo+IMKgCQkJCQnCoGVuY29kZV9w
dXRmaF9tYXhzeiArIFwNCj4gwqAJCQkJCcKgZW5jb2RlX2RlYWxsb2NhdGVfbWF4c3ogKyBcDQo+
IEBAIC0yMjIsNiArMjI0LDE4IEBAIHN0YXRpYyB2b2lkIG5mczRfeGRyX2VuY19hbGxvY2F0ZShz
dHJ1Y3QNCj4gcnBjX3Jxc3QgKnJlcSwNCj4gwqAJZW5jb2RlX25vcHMoJmhkcik7DQo+IMKgfQ0K
PiDCoA0KPiArc3RhdGljIHZvaWQgZW5jb2RlX2NvcHlfY29tbWl0KHN0cnVjdCB4ZHJfc3RyZWFt
ICp4ZHIsDQo+ICsJCQnCoMKgc3RydWN0IG5mczQyX2NvcHlfYXJncyAqYXJncywNCj4gKwkJCcKg
wqBzdHJ1Y3QgY29tcG91bmRfaGRyICpoZHIpDQo+ICt7DQo+ICsJX19iZTMyICpwOw0KPiArDQo+
ICsJZW5jb2RlX29wX2hkcih4ZHIsIE9QX0NPTU1JVCwgZGVjb2RlX2NvbW1pdF9tYXhzeiwgaGRy
KTsNCj4gKwlwID0gcmVzZXJ2ZV9zcGFjZSh4ZHIsIDEyKTsNCj4gKwlwID0geGRyX2VuY29kZV9o
eXBlcihwLCBhcmdzLT5kc3RfcG9zKTsNCj4gKwkqcCA9IGNwdV90b19iZTMyKGFyZ3MtPmNvdW50
KTsNCj4gK30NCj4gKw0KPiDCoC8qDQo+IMKgICogRW5jb2RlIENPUFkgcmVxdWVzdA0KPiDCoCAq
Lw0KPiBAQCAtMjM5LDYgKzI1Myw3IEBAIHN0YXRpYyB2b2lkIG5mczRfeGRyX2VuY19jb3B5KHN0
cnVjdCBycGNfcnFzdA0KPiAqcmVxLA0KPiDCoAllbmNvZGVfc2F2ZWZoKHhkciwgJmhkcik7DQo+
IMKgCWVuY29kZV9wdXRmaCh4ZHIsIGFyZ3MtPmRzdF9maCwgJmhkcik7DQo+IMKgCWVuY29kZV9j
b3B5KHhkciwgYXJncywgJmhkcik7DQo+ICsJZW5jb2RlX2NvcHlfY29tbWl0KHhkciwgYXJncywg
Jmhkcik7DQo+IMKgCWVuY29kZV9ub3BzKCZoZHIpOw0KPiDCoH0NCj4gwqANCj4gZGlmZiAtLWdp
dCBhL2ZzL25mcy93cml0ZS5jIGIvZnMvbmZzL3dyaXRlLmMNCj4gaW5kZXggYWJiMmM4YS4uOGQ2
NWQ4OCAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL3dyaXRlLmMNCj4gKysrIGIvZnMvbmZzL3dyaXRl
LmMNCj4gQEAgLTE3MjAsMzYgKzE3MjAsNiBAQCB2b2lkIG5mc19yZXRyeV9jb21taXQoc3RydWN0
IGxpc3RfaGVhZA0KPiAqcGFnZV9saXN0LA0KPiDCoAlyZXR1cm4gLUVOT01FTTsNCj4gwqB9DQo+
IMKgDQo+IC1pbnQgbmZzX2NvbW1pdF9maWxlKHN0cnVjdCBmaWxlICpmaWxlLCBzdHJ1Y3QgbmZz
X3dyaXRlX3ZlcmlmaWVyDQo+ICp2ZXJmKQ0KPiAtew0KPiAtCXN0cnVjdCBpbm9kZSAqaW5vZGUg
PSBmaWxlX2lub2RlKGZpbGUpOw0KPiAtCXN0cnVjdCBuZnNfb3Blbl9jb250ZXh0ICpvcGVuOw0K
PiAtCXN0cnVjdCBuZnNfY29tbWl0X2luZm8gY2luZm87DQo+IC0Jc3RydWN0IG5mc19wYWdlICpy
ZXE7DQo+IC0JaW50IHJldDsNCj4gLQ0KPiAtCW9wZW4gPSBnZXRfbmZzX29wZW5fY29udGV4dChu
ZnNfZmlsZV9vcGVuX2NvbnRleHQoZmlsZSkpOw0KPiAtCXJlccKgwqA9IG5mc19jcmVhdGVfcmVx
dWVzdChvcGVuLCBOVUxMLCBOVUxMLCAwLA0KPiBpX3NpemVfcmVhZChpbm9kZSkpOw0KPiAtCWlm
IChJU19FUlIocmVxKSkgew0KPiAtCQlyZXQgPSBQVFJfRVJSKHJlcSk7DQo+IC0JCWdvdG8gb3V0
X3B1dDsNCj4gLQl9DQo+IC0NCj4gLQluZnNfaW5pdF9jaW5mb19mcm9tX2lub2RlKCZjaW5mbywg
aW5vZGUpOw0KPiAtDQo+IC0JbWVtY3B5KCZyZXEtPndiX3ZlcmYsIHZlcmYsIHNpemVvZihzdHJ1
Y3QNCj4gbmZzX3dyaXRlX3ZlcmlmaWVyKSk7DQo+IC0JbmZzX3JlcXVlc3RfYWRkX2NvbW1pdF9s
aXN0KHJlcSwgJmNpbmZvKTsNCj4gLQlyZXQgPSBuZnNfY29tbWl0X2lub2RlKGlub2RlLCBGTFVT
SF9TWU5DKTsNCj4gLQlpZiAocmV0ID4gMCkNCj4gLQkJcmV0ID0gMDsNCj4gLQ0KPiAtCW5mc19m
cmVlX3JlcXVlc3QocmVxKTsNCj4gLW91dF9wdXQ6DQo+IC0JcHV0X25mc19vcGVuX2NvbnRleHQo
b3Blbik7DQo+IC0JcmV0dXJuIHJldDsNCj4gLX0NCj4gLUVYUE9SVF9TWU1CT0xfR1BMKG5mc19j
b21taXRfZmlsZSk7DQo+IC0NCj4gwqAvKg0KPiDCoCAqIENPTU1JVCBjYWxsIHJldHVybmVkDQo+
IMKgICovDQoNClNob3VsZG4ndCB3ZSBkZWNvZGUgdGhlIHJlcGx5IGZyb20gdGhlIHNlcnZlciBz
byB0aGF0IHdlIGNhbiByZXR1cm4gYW4NCmVycm9yIG1lc3NhZ2Ugb3IgcmV0cnkgaWYgdGhlIGNv
bW1pdCBmYWlsZWQgYW5kL29yIHRoZSB2ZXJpZmllcnMgZG9uJ3QNCm1hdGNoPyBPdGhlcndpc2Ug
d2Ugd29uJ3Qgbm90aWNlIGlmIHRoZSBmbHVzaCB0byBkaXNrIGZhaWxlZC4NCg0KLS0gDQpUcm9u
ZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgUHJpbWFyeURhdGENCnRy
b25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

2017-05-08 20:48:53

by Olga Kornievskaia

[permalink] [raw]

Subject: Re: [PATCH 1/1] NFS append COMMIT after synchronous COPY

On Mon, May 8, 2017 at 4:31 PM, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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
> [email protected]

2017-05-08 22:02:27

by Olga Kornievskaia

[permalink] [raw]

Subject: [PATCH v3 1/1] NFS append COMMIT after synchronous COPY

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 <[email protected]>
---
fs/nfs/internal.h | 1 -
fs/nfs/nfs42proc.c | 21 +++++++++++++++------
fs/nfs/nfs42xdr.c | 22 ++++++++++++++++++++--
fs/nfs/write.c | 30 ------------------------------
include/linux/nfs_xdr.h | 1 +
5 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 31b26cf..e9b4c33 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 87f5b7b..929d09a 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -167,23 +167,29 @@ static ssize_t _nfs42_proc_copy(struct file *src,
if (status)
return status;

+ res->commit_res.verf = kzalloc(sizeof(struct nfs_writeverf), GFP_NOFS);
+ if (!res->commit_res.verf)
+ return -ENOMEM;
status = nfs4_call_sync(server->client, server, &msg,
&args->seq_args, &res->seq_res, 0);
if (status == -ENOTSUPP)
server->caps &= ~NFS_CAP_COPY;
if (status)
- return status;
+ goto out;

- if (res->write_res.verifier.committed != NFS_FILE_SYNC) {
- status = nfs_commit_file(dst, &res->write_res.verifier.verifier);
- if (status)
- return status;
+ if (!nfs_write_verifier_cmp(&res->write_res.verifier.verifier,
+ &res->commit_res.verf->verifier)) {
+ status = -EAGAIN;
+ goto out;
}

truncate_pagecache_range(dst_inode, pos_dst,
pos_dst + res->write_res.count);

- return res->write_res.count;
+ status = res->write_res.count;
+out:
+ kfree(res->commit_res.verf);
+ return status;
}

ssize_t nfs42_proc_copy(struct file *src, loff_t pos_src,
@@ -240,6 +246,9 @@ ssize_t nfs42_proc_copy(struct file *src, loff_t pos_src,
if (err == -ENOTSUPP) {
err = -EOPNOTSUPP;
break;
+ } if (err == -EAGAIN) {
+ dst_exception.retry = 1;
+ continue;
}

err2 = nfs4_handle_exception(server, err, &src_exception);
diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 6c72964..528362f 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_maxsz + \
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);
}

@@ -481,6 +496,9 @@ static int nfs4_xdr_dec_copy(struct rpc_rqst *rqstp,
if (status)
goto out;
status = decode_copy(xdr, res);
+ if (status)
+ goto out;
+ status = decode_commit(xdr, &res->commit_res);
out:
return status;
}
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 59e21cc..85bfa41 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1742,36 +1742,6 @@ void nfs_retry_commit(struct list_head *page_list,
data->mds_ops, how, 0);
}

-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
*/
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 677c6b9..b28c834 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1383,6 +1383,7 @@ struct nfs42_copy_res {
struct nfs42_write_res write_res;
bool consecutive;
bool synchronous;
+ struct nfs_commitres commit_res;
};

struct nfs42_seek_args {
--
1.8.3.1