If we're using NFSv4.1, then we have the ability to let the server know
whether or not we believe that returning a delegation as part of our OPEN
request would be useful.
The feature needs to be used with care, since the client sending the request
doesn't necessarily know how other clients are using that file, and how
they may be affected by the delegation.
For this reason, our initial use of the feature will be to let the server
know when the client believes that handing out a delegation would not be
useful.
The first application for this function is when opening the file using
O_DIRECT.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 30 +++++++++++++++++++
fs/nfs/nfs4xdr.c | 79 +++++++++++++++++++++++++++++++++----------------
include/linux/nfs_xdr.h | 2 ++
3 files changed, 85 insertions(+), 26 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6e1c9b2d92c5..cd4295d84d54 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -940,6 +940,31 @@ static bool nfs4_clear_cap_atomic_open_v1(struct nfs_server *server,
return true;
}
+static u32
+nfs4_map_atomic_open_share(struct nfs_server *server,
+ fmode_t fmode, int openflags)
+{
+ u32 res = 0;
+
+ switch (fmode & (FMODE_READ | FMODE_WRITE)) {
+ case FMODE_READ:
+ res = NFS4_SHARE_ACCESS_READ;
+ break;
+ case FMODE_WRITE:
+ res = NFS4_SHARE_ACCESS_WRITE;
+ break;
+ case FMODE_READ|FMODE_WRITE:
+ res = NFS4_SHARE_ACCESS_BOTH;
+ }
+ if (!(server->caps & NFS_CAP_ATOMIC_OPEN_V1))
+ goto out;
+ /* Want no delegation if we're using O_DIRECT */
+ if (openflags & O_DIRECT)
+ res |= NFS4_SHARE_WANT_NO_DELEG;
+out:
+ return res;
+}
+
static enum open_claim_type4
nfs4_map_atomic_open_claim(struct nfs_server *server,
enum open_claim_type4 claim)
@@ -1002,6 +1027,8 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct dentry *dentry,
atomic_inc(&sp->so_count);
p->o_arg.open_flags = flags;
p->o_arg.fmode = fmode & (FMODE_READ|FMODE_WRITE);
+ p->o_arg.share_access = nfs4_map_atomic_open_share(server,
+ fmode, flags);
/* don't put an ACCESS op in OPEN compound if O_EXCL, because ACCESS
* will return permission denied for all bits until close */
if (!(flags & O_EXCL)) {
@@ -2695,6 +2722,9 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
goto out_wait;
}
}
+ calldata->arg.share_access =
+ nfs4_map_atomic_open_share(NFS_SERVER(inode),
+ calldata->arg.fmode, 0);
nfs_fattr_init(calldata->res.fattr);
calldata->timestamp = jiffies;
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index a2329d69502b..e23a0a664e12 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1351,24 +1351,12 @@ static void encode_lookup(struct xdr_stream *xdr, const struct qstr *name, struc
encode_string(xdr, name->len, name->name);
}
-static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode)
+static void encode_share_access(struct xdr_stream *xdr, u32 share_access)
{
__be32 *p;
p = reserve_space(xdr, 8);
- switch (fmode & (FMODE_READ|FMODE_WRITE)) {
- case FMODE_READ:
- *p++ = cpu_to_be32(NFS4_SHARE_ACCESS_READ);
- break;
- case FMODE_WRITE:
- *p++ = cpu_to_be32(NFS4_SHARE_ACCESS_WRITE);
- break;
- case FMODE_READ|FMODE_WRITE:
- *p++ = cpu_to_be32(NFS4_SHARE_ACCESS_BOTH);
- break;
- default:
- *p++ = cpu_to_be32(0);
- }
+ *p++ = cpu_to_be32(share_access);
*p = cpu_to_be32(0); /* for linux, share_deny = 0 always */
}
@@ -1380,7 +1368,7 @@ static inline void encode_openhdr(struct xdr_stream *xdr, const struct nfs_opena
* owner 4 = 32
*/
encode_nfs4_seqid(xdr, arg->seqid);
- encode_share_access(xdr, arg->fmode);
+ encode_share_access(xdr, arg->share_access);
p = reserve_space(xdr, 36);
p = xdr_encode_hyper(p, arg->clientid);
*p++ = cpu_to_be32(24);
@@ -1535,7 +1523,7 @@ static void encode_open_downgrade(struct xdr_stream *xdr, const struct nfs_close
encode_op_hdr(xdr, OP_OPEN_DOWNGRADE, decode_open_downgrade_maxsz, hdr);
encode_nfs4_stateid(xdr, &arg->stateid);
encode_nfs4_seqid(xdr, arg->seqid);
- encode_share_access(xdr, arg->fmode);
+ encode_share_access(xdr, arg->share_access);
}
static void
@@ -4935,20 +4923,13 @@ out_overflow:
return -EIO;
}
-static int decode_delegation(struct xdr_stream *xdr, struct nfs_openres *res)
+static int decode_rw_delegation(struct xdr_stream *xdr,
+ uint32_t delegation_type,
+ struct nfs_openres *res)
{
__be32 *p;
- uint32_t delegation_type;
int status;
- p = xdr_inline_decode(xdr, 4);
- if (unlikely(!p))
- goto out_overflow;
- delegation_type = be32_to_cpup(p);
- if (delegation_type == NFS4_OPEN_DELEGATE_NONE) {
- res->delegation_type = 0;
- return 0;
- }
status = decode_stateid(xdr, &res->delegation);
if (unlikely(status))
return status;
@@ -4972,6 +4953,52 @@ out_overflow:
return -EIO;
}
+static int decode_no_delegation(struct xdr_stream *xdr, struct nfs_openres *res)
+{
+ __be32 *p;
+ uint32_t why_no_delegation;
+
+ p = xdr_inline_decode(xdr, 4);
+ if (unlikely(!p))
+ goto out_overflow;
+ why_no_delegation = be32_to_cpup(p);
+ switch (why_no_delegation) {
+ case WND4_CONTENTION:
+ case WND4_RESOURCE:
+ xdr_inline_decode(xdr, 4);
+ /* Ignore for now */
+ }
+ return 0;
+out_overflow:
+ print_overflow_msg(__func__, xdr);
+ return -EIO;
+}
+
+static int decode_delegation(struct xdr_stream *xdr, struct nfs_openres *res)
+{
+ __be32 *p;
+ uint32_t delegation_type;
+
+ p = xdr_inline_decode(xdr, 4);
+ if (unlikely(!p))
+ goto out_overflow;
+ delegation_type = be32_to_cpup(p);
+ res->delegation_type = 0;
+ switch (delegation_type) {
+ case NFS4_OPEN_DELEGATE_NONE:
+ return 0;
+ case NFS4_OPEN_DELEGATE_READ:
+ case NFS4_OPEN_DELEGATE_WRITE:
+ return decode_rw_delegation(xdr, delegation_type, res);
+ case NFS4_OPEN_DELEGATE_NONE_EXT:
+ return decode_no_delegation(xdr, res);
+ }
+ return -EIO;
+out_overflow:
+ print_overflow_msg(__func__, xdr);
+ return -EIO;
+}
+
static int decode_open(struct xdr_stream *xdr, struct nfs_openres *res)
{
__be32 *p;
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 81401125ab2d..2c35e2affa6f 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -326,6 +326,7 @@ struct nfs_openargs {
struct nfs_seqid * seqid;
int open_flags;
fmode_t fmode;
+ u32 share_access;
u32 access;
__u64 clientid;
struct stateowner_id id;
@@ -393,6 +394,7 @@ struct nfs_closeargs {
nfs4_stateid stateid;
struct nfs_seqid * seqid;
fmode_t fmode;
+ u32 share_access;
const u32 * bitmask;
};
--
2.1.0
If we already hold a delegation, there should be no reason for the
server to issue it to us again. Unfortunately, there appear to be
servers out there that engage in this practice. While it is often
harmless to do so, there is one case where this creates a problem
and that is when the client is in the process of returning that
delegation.
This patch uses the NFSv4.1 NFS4_SHARE_WANT_NO_DELEG flag to inform
the server not to return a delegation in these cases.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cd4295d84d54..fb41624bafc9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -942,8 +942,10 @@ static bool nfs4_clear_cap_atomic_open_v1(struct nfs_server *server,
static u32
nfs4_map_atomic_open_share(struct nfs_server *server,
+ struct inode *inode,
fmode_t fmode, int openflags)
{
+ struct nfs_delegation *delegation;
u32 res = 0;
switch (fmode & (FMODE_READ | FMODE_WRITE)) {
@@ -959,8 +961,25 @@ nfs4_map_atomic_open_share(struct nfs_server *server,
if (!(server->caps & NFS_CAP_ATOMIC_OPEN_V1))
goto out;
/* Want no delegation if we're using O_DIRECT */
- if (openflags & O_DIRECT)
+ if (openflags & O_DIRECT) {
res |= NFS4_SHARE_WANT_NO_DELEG;
+ goto out;
+ }
+ if (inode == NULL)
+ goto out;
+ rcu_read_lock();
+ delegation = rcu_dereference(NFS_I(inode)->delegation);
+ /*
+ * If we have a delegation, either ask for an upgrade or ask for
+ * no delegation
+ */
+ if (delegation) {
+ if ((fmode & FMODE_WRITE) && !(delegation->type & FMODE_WRITE))
+ res |= NFS4_SHARE_WANT_WRITE_DELEG;
+ else
+ res |= NFS4_SHARE_WANT_NO_DELEG;
+ }
+ rcu_read_unlock();
out:
return res;
}
@@ -1028,7 +1047,7 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct dentry *dentry,
p->o_arg.open_flags = flags;
p->o_arg.fmode = fmode & (FMODE_READ|FMODE_WRITE);
p->o_arg.share_access = nfs4_map_atomic_open_share(server,
- fmode, flags);
+ dentry->d_inode, fmode, flags);
/* don't put an ACCESS op in OPEN compound if O_EXCL, because ACCESS
* will return permission denied for all bits until close */
if (!(flags & O_EXCL)) {
@@ -2724,7 +2743,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
}
calldata->arg.share_access =
nfs4_map_atomic_open_share(NFS_SERVER(inode),
- calldata->arg.fmode, 0);
+ NULL, calldata->arg.fmode, 0);
nfs_fattr_init(calldata->res.fattr);
calldata->timestamp = jiffies;
--
2.1.0
DQo+IE9uIEZlYiAzLCAyMDE1LCBhdCA0OjU5IFBNLCBUcm9uZCBNeWtsZWJ1c3QgPHRyb25kLm15
a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+IHdyb3RlOg0KPiANCj4gSWYgd2UgYWxyZWFkeSBob2xk
IGEgZGVsZWdhdGlvbiwgdGhlcmUgc2hvdWxkIGJlIG5vIHJlYXNvbiBmb3IgdGhlDQo+IHNlcnZl
ciB0byBpc3N1ZSBpdCB0byB1cyBhZ2Fpbi4gVW5mb3J0dW5hdGVseSwgdGhlcmUgYXBwZWFyIHRv
IGJlDQo+IHNlcnZlcnMgb3V0IHRoZXJlIHRoYXQgZW5nYWdlIGluIHRoaXMgcHJhY3RpY2UuIFdo
aWxlIGl0IGlzIG9mdGVuDQo+IGhhcm1sZXNzIHRvIGRvIHNvLCB0aGVyZSBpcyBvbmUgY2FzZSB3
aGVyZSB0aGlzIGNyZWF0ZXMgYSBwcm9ibGVtDQo+IGFuZCB0aGF0IGlzIHdoZW4gdGhlIGNsaWVu
dCBpcyBpbiB0aGUgcHJvY2VzcyBvZiByZXR1cm5pbmcgdGhhdA0KPiBkZWxlZ2F0aW9uLg0KPiBU
aGlzIHBhdGNoIHVzZXMgdGhlIE5GU3Y0LjEgTkZTNF9TSEFSRV9XQU5UX05PX0RFTEVHIGZsYWcg
dG8gaW5mb3JtDQo+IHRoZSBzZXJ2ZXIgbm90IHRvIHJldHVybiBhIGRlbGVnYXRpb24gaW4gdGhl
c2UgY2FzZXMuDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPHRyb25kLm15
a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+DQo+IC0tLQ0KPiBmcy9uZnMvbmZzNHByb2MuYyB8IDI1
ICsrKysrKysrKysrKysrKysrKysrKystLS0NCj4gMSBmaWxlIGNoYW5nZWQsIDIyIGluc2VydGlv
bnMoKyksIDMgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9j
LmMgYi9mcy9uZnMvbmZzNHByb2MuYw0KPiBpbmRleCBjZDQyOTVkODRkNTQuLmZiNDE2MjRiYWZj
OSAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCj4gKysrIGIvZnMvbmZzL25mczRw
cm9jLmMNCj4gQEAgLTk0Miw4ICs5NDIsMTAgQEAgc3RhdGljIGJvb2wgbmZzNF9jbGVhcl9jYXBf
YXRvbWljX29wZW5fdjEoc3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZlciwNCj4gDQo+IHN0YXRpYyB1
MzINCj4gbmZzNF9tYXBfYXRvbWljX29wZW5fc2hhcmUoc3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZl
ciwNCj4gKwkJc3RydWN0IGlub2RlICppbm9kZSwNCj4gCQlmbW9kZV90IGZtb2RlLCBpbnQgb3Bl
bmZsYWdzKQ0KPiB7DQo+ICsJc3RydWN0IG5mc19kZWxlZ2F0aW9uICpkZWxlZ2F0aW9uOw0KPiAJ
dTMyIHJlcyA9IDA7DQo+IA0KPiAJc3dpdGNoIChmbW9kZSAmIChGTU9ERV9SRUFEIHwgRk1PREVf
V1JJVEUpKSB7DQo+IEBAIC05NTksOCArOTYxLDI1IEBAIG5mczRfbWFwX2F0b21pY19vcGVuX3No
YXJlKHN0cnVjdCBuZnNfc2VydmVyICpzZXJ2ZXIsDQo+IAlpZiAoIShzZXJ2ZXItPmNhcHMgJiBO
RlNfQ0FQX0FUT01JQ19PUEVOX1YxKSkNCj4gCQlnb3RvIG91dDsNCj4gCS8qIFdhbnQgbm8gZGVs
ZWdhdGlvbiBpZiB3ZSdyZSB1c2luZyBPX0RJUkVDVCAqLw0KPiAtCWlmIChvcGVuZmxhZ3MgJiBP
X0RJUkVDVCkNCj4gKwlpZiAob3BlbmZsYWdzICYgT19ESVJFQ1QpIHsNCj4gCQlyZXMgfD0gTkZT
NF9TSEFSRV9XQU5UX05PX0RFTEVHOw0KPiArCQlnb3RvIG91dDsNCj4gKwl9DQo+ICsJaWYgKGlu
b2RlID09IE5VTEwpDQo+ICsJCWdvdG8gb3V0Ow0KPiArCXJjdV9yZWFkX2xvY2soKTsNCj4gKwlk
ZWxlZ2F0aW9uID0gcmN1X2RlcmVmZXJlbmNlKE5GU19JKGlub2RlKS0+ZGVsZWdhdGlvbik7DQo+
ICsJLyoNCj4gKwkgKiBJZiB3ZSBoYXZlIGEgZGVsZWdhdGlvbiwgZWl0aGVyIGFzayBmb3IgYW4g
dXBncmFkZSBvciBhc2sgZm9yDQo+ICsJICogbm8gZGVsZWdhdGlvbg0KPiArCSAqLw0KPiArCWlm
IChkZWxlZ2F0aW9uKSB7DQo+ICsJCWlmICgoZm1vZGUgJiBGTU9ERV9XUklURSkgJiYgIShkZWxl
Z2F0aW9uLT50eXBlICYgRk1PREVfV1JJVEUpKQ0KPiArCQkJcmVzIHw9IE5GUzRfU0hBUkVfV0FO
VF9XUklURV9ERUxFRzsNCj4gKwkJZWxzZQ0KPiArCQkJcmVzIHw9IE5GUzRfU0hBUkVfV0FOVF9O
T19ERUxFRzsNCj4gKwl9DQo+ICsJcmN1X3JlYWRfdW5sb2NrKCk7DQo+IG91dDoNCj4gCXJldHVy
biByZXM7DQo+IH0NCj4gQEAgLTEwMjgsNyArMTA0Nyw3IEBAIHN0YXRpYyBzdHJ1Y3QgbmZzNF9v
cGVuZGF0YSAqbmZzNF9vcGVuZGF0YV9hbGxvYyhzdHJ1Y3QgZGVudHJ5ICpkZW50cnksDQo+IAlw
LT5vX2FyZy5vcGVuX2ZsYWdzID0gZmxhZ3M7DQo+IAlwLT5vX2FyZy5mbW9kZSA9IGZtb2RlICYg
KEZNT0RFX1JFQUR8Rk1PREVfV1JJVEUpOw0KPiAJcC0+b19hcmcuc2hhcmVfYWNjZXNzID0gbmZz
NF9tYXBfYXRvbWljX29wZW5fc2hhcmUoc2VydmVyLA0KPiAtCQkJZm1vZGUsIGZsYWdzKTsNCj4g
KwkJCWRlbnRyeS0+ZF9pbm9kZSwgZm1vZGUsIGZsYWdzKTsNCj4gCS8qIGRvbid0IHB1dCBhbiBB
Q0NFU1Mgb3AgaW4gT1BFTiBjb21wb3VuZCBpZiBPX0VYQ0wsIGJlY2F1c2UgQUNDRVNTDQo+IAkg
KiB3aWxsIHJldHVybiBwZXJtaXNzaW9uIGRlbmllZCBmb3IgYWxsIGJpdHMgdW50aWwgY2xvc2Ug
Ki8NCj4gCWlmICghKGZsYWdzICYgT19FWENMKSkgew0KPiBAQCAtMjcyNCw3ICsyNzQzLDcgQEAg
c3RhdGljIHZvaWQgbmZzNF9jbG9zZV9wcmVwYXJlKHN0cnVjdCBycGNfdGFzayAqdGFzaywgdm9p
ZCAqZGF0YSkNCj4gCX0NCj4gCWNhbGxkYXRhLT5hcmcuc2hhcmVfYWNjZXNzID0NCj4gCQluZnM0
X21hcF9hdG9taWNfb3Blbl9zaGFyZShORlNfU0VSVkVSKGlub2RlKSwNCj4gLQkJCQljYWxsZGF0
YS0+YXJnLmZtb2RlLCAwKTsNCj4gKwkJCQlOVUxMLCBjYWxsZGF0YS0+YXJnLmZtb2RlLCAwKTsN
Cj4gDQo+IAluZnNfZmF0dHJfaW5pdChjYWxsZGF0YS0+cmVzLmZhdHRyKTsNCj4gCWNhbGxkYXRh
LT50aW1lc3RhbXAgPSBqaWZmaWVzOw0KPiAtLSANCj4gMi4xLjANCj4gDQoNCg0KSGkgVHJvbmQs
DQoNCkRvIHlvdSBzZWUgdGhpcyBvZiBoZWxwaW5nIHdpdGggdGhlIHJhY2U/IFdlIHdvbuKAmXQg
ZmluZCBhIGRlbGVnYXRpb24gb24gdGhlIHJhY2luZyBvcGVuIGFzIGl0IGhhcyBiZWVuIGRldGFj
aGVkIGZyb20gdGhlIGlub2RlLiBGb3IgdGhlIG90aGVyIHBhdGNoLCBhcmVu4oCZdCB3ZSBhbHJl
YWR5IGNoZWNraW5nIGlmIHdlIGNhbiBkbyBhIGxvY2FsIG9wZW4gYnkgY2hlY2tpbmcgZm9yIHRo
ZSBkZWxlZ2F0aW9uIChjYW5fb3Blbl9kZWxlZ2F0ZWQoKSk/DQoNClRoYW5rcy4NCg0K
On Tue, Feb 3, 2015 at 5:47 PM, Kornievskaia, Olga
<[email protected]> wrote:
>
>> On Feb 3, 2015, at 4:59 PM, Trond Myklebust <[email protected]> wrote:
>>
>> If we already hold a delegation, there should be no reason for the
>> server to issue it to us again. Unfortunately, there appear to be
>> servers out there that engage in this practice. While it is often
>> harmless to do so, there is one case where this creates a problem
>> and that is when the client is in the process of returning that
>> delegation.
>> This patch uses the NFSv4.1 NFS4_SHARE_WANT_NO_DELEG flag to inform
>> the server not to return a delegation in these cases.
>>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index cd4295d84d54..fb41624bafc9 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -942,8 +942,10 @@ static bool nfs4_clear_cap_atomic_open_v1(struct nfs_server *server,
>>
>> static u32
>> nfs4_map_atomic_open_share(struct nfs_server *server,
>> + struct inode *inode,
>> fmode_t fmode, int openflags)
>> {
>> + struct nfs_delegation *delegation;
>> u32 res = 0;
>>
>> switch (fmode & (FMODE_READ | FMODE_WRITE)) {
>> @@ -959,8 +961,25 @@ nfs4_map_atomic_open_share(struct nfs_server *server,
>> if (!(server->caps & NFS_CAP_ATOMIC_OPEN_V1))
>> goto out;
>> /* Want no delegation if we're using O_DIRECT */
>> - if (openflags & O_DIRECT)
>> + if (openflags & O_DIRECT) {
>> res |= NFS4_SHARE_WANT_NO_DELEG;
>> + goto out;
>> + }
>> + if (inode == NULL)
>> + goto out;
>> + rcu_read_lock();
>> + delegation = rcu_dereference(NFS_I(inode)->delegation);
>> + /*
>> + * If we have a delegation, either ask for an upgrade or ask for
>> + * no delegation
>> + */
>> + if (delegation) {
>> + if ((fmode & FMODE_WRITE) && !(delegation->type & FMODE_WRITE))
>> + res |= NFS4_SHARE_WANT_WRITE_DELEG;
>> + else
>> + res |= NFS4_SHARE_WANT_NO_DELEG;
>> + }
>> + rcu_read_unlock();
>> out:
>> return res;
>> }
>> @@ -1028,7 +1047,7 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct dentry *dentry,
>> p->o_arg.open_flags = flags;
>> p->o_arg.fmode = fmode & (FMODE_READ|FMODE_WRITE);
>> p->o_arg.share_access = nfs4_map_atomic_open_share(server,
>> - fmode, flags);
>> + dentry->d_inode, fmode, flags);
>> /* don't put an ACCESS op in OPEN compound if O_EXCL, because ACCESS
>> * will return permission denied for all bits until close */
>> if (!(flags & O_EXCL)) {
>> @@ -2724,7 +2743,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
>> }
>> calldata->arg.share_access =
>> nfs4_map_atomic_open_share(NFS_SERVER(inode),
>> - calldata->arg.fmode, 0);
>> + NULL, calldata->arg.fmode, 0);
>>
>> nfs_fattr_init(calldata->res.fattr);
>> calldata->timestamp = jiffies;
>> --
>> 2.1.0
>>
>
>
> Hi Trond,
>
> Do you see this of helping with the race? We won’t find a delegation on the racing open as it has been detached from the inode. For the other patch, aren’t we already checking if we can do a local open by checking for the delegation (can_open_delegated())?
>
Argh. You're 100% right in that it won't completely close the race.
However it narrows the race to the delegreturn itself, whereas
currently we also have a race while in the process of reclaiming opens
+ locks.
As for patch 1/2, I believe that O_DIRECT opens are still a valid case.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Tue, Feb 03, 2015 at 04:59:44PM -0500, Trond Myklebust wrote:
> If we already hold a delegation, there should be no reason for the
> server to issue it to us again. Unfortunately, there appear to be
> servers out there that engage in this practice. While it is often
> harmless to do so, there is one case where this creates a problem
> and that is when the client is in the process of returning that
> delegation.
> This patch uses the NFSv4.1 NFS4_SHARE_WANT_NO_DELEG flag to inform
> the server not to return a delegation in these cases.
Shouldn't this be patch 1 as it's the actual bug fix that might need
backporting?
On Thu, Feb 5, 2015 at 9:07 AM, Christoph Hellwig <[email protected]> wrote:
> On Tue, Feb 03, 2015 at 04:59:44PM -0500, Trond Myklebust wrote:
>> If we already hold a delegation, there should be no reason for the
>> server to issue it to us again. Unfortunately, there appear to be
>> servers out there that engage in this practice. While it is often
>> harmless to do so, there is one case where this creates a problem
>> and that is when the client is in the process of returning that
>> delegation.
>> This patch uses the NFSv4.1 NFS4_SHARE_WANT_NO_DELEG flag to inform
>> the server not to return a delegation in these cases.
>
> Shouldn't this be patch 1 as it's the actual bug fix that might need
> backporting?
I'm dropping it instead. It doesn't completely close the race left
open by the lack of clarity in the protocol, and so it is
counterproductive; it will leave sloppy server vendors thinking the
problem is solved when it isn't.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Tue, Feb 03, 2015 at 04:59:43PM -0500, Trond Myklebust wrote:
> If we're using NFSv4.1, then we have the ability to let the server know
> whether or not we believe that returning a delegation as part of our OPEN
> request would be useful.
> The feature needs to be used with care, since the client sending the request
> doesn't necessarily know how other clients are using that file, and how
> they may be affected by the delegation.
> For this reason, our initial use of the feature will be to let the server
> know when the client believes that handing out a delegation would not be
> useful.
> The first application for this function is when opening the file using
> O_DIRECT.
I can't see why O_DIRECT openers would not want to use a delegation, can you
explain your rationale?
On Wed, Feb 4, 2015 at 3:16 AM, Christoph Hellwig <[email protected]> wrote:
> On Tue, Feb 03, 2015 at 04:59:43PM -0500, Trond Myklebust wrote:
>> If we're using NFSv4.1, then we have the ability to let the server know
>> whether or not we believe that returning a delegation as part of our OPEN
>> request would be useful.
>> The feature needs to be used with care, since the client sending the request
>> doesn't necessarily know how other clients are using that file, and how
>> they may be affected by the delegation.
>> For this reason, our initial use of the feature will be to let the server
>> know when the client believes that handing out a delegation would not be
>> useful.
>> The first application for this function is when opening the file using
>> O_DIRECT.
>
> I can't see why O_DIRECT openers would not want to use a delegation, can you
> explain your rationale?
>
Delegations are all about allowing the NFS client to cache data
aggressively, and notifying when it is no longer safe to do so. That
is clearly not of interest to an application using O_DIRECT, since it
is by definition managing the data cache (if there is one) instead of
the NFS client. We don't share delegation state with userspace and
even if we did, there are no existing applications out there that are
capable (or even interested) of taking advantage of it.
You can argue that the client could still use the delegation to cache
metadata and open/lock state, but most of the users of O_DIRECT of
which I'm aware tend to be data intensive, and not very metadata/state
intensive. So why burden both the server and the with that extra state
management?
Cheers
Trond
On Wed, Feb 04, 2015 at 07:32:43AM -0500, Trond Myklebust wrote:
> Delegations are all about allowing the NFS client to cache data
> aggressively, and notifying when it is no longer safe to do so. That
> is clearly not of interest to an application using O_DIRECT, since it
> is by definition managing the data cache (if there is one) instead of
> the NFS client. We don't share delegation state with userspace and
> even if we did, there are no existing applications out there that are
> capable (or even interested) of taking advantage of it.
>
> You can argue that the client could still use the delegation to cache
> metadata and open/lock state, but most of the users of O_DIRECT of
> which I'm aware tend to be data intensive, and not very metadata/state
> intensive. So why burden both the server and the with that extra state
> management?
How does the delegation hurt us in this case? That needs to go into
the patch description, and into a comment near the code.