On 03/08/2012 12:57 PM, J. Bruce Fields wrote:
> On Thu, Mar 08, 2012 at 08:50:14PM +0000, Myklebust, Trond wrote:
>> On Thu, 2012-03-08 at 15:42 -0500, J. Bruce Fields wrote:
>>> On Thu, Mar 08, 2012 at 03:23:34PM -0500, Olga Kornievskaia wrote:
>>>> On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
>>>> <[email protected]> wrote:
>>>>> On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
>>>>>> wouldn't it be better for you to proactively return a read delegation
>>>>>> then unnecessarily erroring?
>>>>>
>>>>> If nobody else holds a delegation, then the NFS client is actually
>>>>> allowed to keep its read delegation while writing to the file. It does
>>>>> admittedly need to request an OPEN stateid for write in that case...
>>>>> (See section 10.4 of RFC3530bis draft 16)
>>>>
>>>> If we both agree that there has to be a request for an open stateid for
>>>> a write, then instead of returning the read delegation if the client receives
>>>> err_openmode (when it send the request with read delegation stateid
>>>> as you said per 3560bis), can't the client resend the setattr with the open
>>>> stateid? The ordering of the stateid usage is a "should" and not a "must".
>>>>
>>>> In rfc5661, it really doesn't make sense to ever send a setattr with
>>>> a read delegation stateid. According to 9.1.2, the server "MUST" return
>>>> err_open_mode" error in that case.
>>>>
>>>> I gather you are in this case dealing with 4.0 delegations. But I wonder
>>>> if you'll do something else for 4.1 delegation then?
>>>
>>> 3530bis has the same language ("...must verify that the access mode
>>> allows writing and return an NFS4ERR_OPENMODE error if it does not").
>>
>> OK, so we shouldn't send the delegation stateid either for v4 or v4.1.
>> However should we pre-emptively return the delegation? I've been
>> assuming not.
>
> The server's only legal option is to recall it, so it seems odd not to
> pre-emptively return--
Also from the client that sent the setattr? I, as Trond, understood that
the read delegation must be recalled from all clients but the one
doing the setattr/write.
other wise what does it mean:
"allowed to keep its read delegation while writing to the file"
I think the server should filter out it's global recall to dis-include
the caller.
Though I agree that the client must get a writable stateid for
setattr, and should not use it's read-delegation-stateid
> but as you say there's nothing to prevent the
> server from then handing one right back, possibly before you get a
> chance to send the setattr.
>
The above recall filtering will solve that. I know that in layout recalls
we have such facility, and we actually use it in RAID5 exofs.
> (And the linux server might well do that--maybe it should have some
> heuristic not to hand out a delegation that was just returned--not so
> much for this case as just because a return is a sign that the
> delegation isn't useful to that client.)
>
Thanks
Boaz
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Miklos,
The base cause of the test failure was that the client was holding
onto a read delegation while trying to truncate the file. Whereas
some servers allow the client to do this (provided that nobody else
holds a read delegation), the Linux server does not, and returns
an error.
The fix is therefore to return the delegation if the server rejects
our attempt (see patch 2/2).
I've included patch 1/2 since it too was already queued for the
[email protected] queue, and so 2/2 has a dependency...
Thanks for the bug report!
Cheers
Trond
Trond Myklebust (2):
NFS: Properly handle the case where the delegation is revoked
NFSv4: Return the delegation if the server returns NFS4ERR_OPENMODE
fs/nfs/delegation.c | 11 +++++++++++
fs/nfs/delegation.h | 1 +
fs/nfs/nfs4_fs.h | 3 +++
fs/nfs/nfs4proc.c | 31 ++++++++++++++++++++++++++++---
fs/nfs/nfs4state.c | 29 +++++++++++++++++++++++++++--
5 files changed, 70 insertions(+), 5 deletions(-)
--
1.7.7.6
wouldn't it be better for you to proactively return a read delegation
then unnecessarily erroring?
i also don't understand how this error occurs. doing a setattr in this
case you must have used a non-special stateid. the server would only
return an err_openmode if you sent the setattr with a read delegation
stateid. i guess my question is what stateid would you use that from
client's perspective represent a write-type state id but yet a server
would flag as having wrong access type?
also i'm curious why would a server, instead of returning
err_openmode, would not first recall your read delegation?
On Wed, Mar 7, 2012 at 5:40 PM, Trond Myklebust
<[email protected]> wrote:
> If a setattr() fails because of an NFS4ERR_OPENMODE error, it is
> probably due to us holding a read delegation. Ensure that the
> recovery routines return that delegation in this case.
>
> Reported-by: Miklos Szeredi <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> Cc: [email protected]
> ---
> ?fs/nfs/nfs4_fs.h ?| ? ?1 +
> ?fs/nfs/nfs4proc.c | ? 13 ++++++++++++-
> ?2 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 5c9b20b..8ccaf24 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -193,6 +193,7 @@ struct nfs4_exception {
> ? ? ? ?long timeout;
> ? ? ? ?int retry;
> ? ? ? ?struct nfs4_state *state;
> + ? ? ? struct inode *inode;
> ?};
>
> ?struct nfs4_state_recovery_ops {
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 3d26bab..bfcaa03 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -259,18 +259,28 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
> ?{
> ? ? ? ?struct nfs_client *clp = server->nfs_client;
> ? ? ? ?struct nfs4_state *state = exception->state;
> + ? ? ? struct inode *inode = exception->inode;
> ? ? ? ?int ret = errorcode;
>
> ? ? ? ?exception->retry = 0;
> ? ? ? ?switch(errorcode) {
> ? ? ? ? ? ? ? ?case 0:
> ? ? ? ? ? ? ? ? ? ? ? ?return 0;
> + ? ? ? ? ? ? ? case -NFS4ERR_OPENMODE:
> + ? ? ? ? ? ? ? ? ? ? ? if (nfs_have_delegation(inode, FMODE_READ)) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? nfs_inode_return_delegation(inode);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? exception->retry = 1;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 0;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? if (state == NULL)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? ? ? ? ? nfs4_schedule_stateid_recovery(server, state);
> + ? ? ? ? ? ? ? ? ? ? ? goto wait_on_recovery;
> ? ? ? ? ? ? ? ?case -NFS4ERR_DELEG_REVOKED:
> ? ? ? ? ? ? ? ?case -NFS4ERR_ADMIN_REVOKED:
> ? ? ? ? ? ? ? ?case -NFS4ERR_BAD_STATEID:
> ? ? ? ? ? ? ? ? ? ? ? ?if (state != NULL)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?nfs_remove_bad_delegation(state->inode);
> - ? ? ? ? ? ? ? case -NFS4ERR_OPENMODE:
> ? ? ? ? ? ? ? ? ? ? ? ?if (state == NULL)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ? ? ? ? ?nfs4_schedule_stateid_recovery(server, state);
> @@ -1908,6 +1918,7 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
> ? ? ? ?struct nfs_server *server = NFS_SERVER(inode);
> ? ? ? ?struct nfs4_exception exception = {
> ? ? ? ? ? ? ? ?.state = state,
> + ? ? ? ? ? ? ? .inode = inode,
> ? ? ? ?};
> ? ? ? ?int err;
> ? ? ? ?do {
> --
> 1.7.7.6
>
> --
> 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
If we know that the delegation stateid is bad or revoked, we need to
remove that delegation as soon as possible, and then mark all the
stateids that relied on that delegation for recovery. We cannot use
the delegation as part of the recovery process.
Also note that NFSv4.1 uses a different error code (NFS4ERR_DELEG_REVOKED)
to indicate that the delegation was revoked.
Finally, ensure that setlk() and setattr() can both recover safely from
a revoked delegation.
Signed-off-by: Trond Myklebust <[email protected]>
Cc: [email protected]
---
fs/nfs/delegation.c | 11 +++++++++++
fs/nfs/delegation.h | 1 +
fs/nfs/nfs4_fs.h | 2 ++
fs/nfs/nfs4proc.c | 18 ++++++++++++++++--
fs/nfs/nfs4state.c | 29 +++++++++++++++++++++++++++--
5 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 7f26540..ac889af 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -466,6 +466,17 @@ static void nfs_delegation_run_state_manager(struct nfs_client *clp)
nfs4_schedule_state_manager(clp);
}
+void nfs_remove_bad_delegation(struct inode *inode)
+{
+ struct nfs_delegation *delegation;
+
+ delegation = nfs_detach_delegation(NFS_I(inode), NFS_SERVER(inode));
+ if (delegation) {
+ nfs_inode_find_state_and_recover(inode, &delegation->stateid);
+ nfs_free_delegation(delegation);
+ }
+}
+
/**
* nfs_expire_all_delegation_types
* @clp: client to process
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index d9322e4..691a796 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -45,6 +45,7 @@ void nfs_expire_unreferenced_delegations(struct nfs_client *clp);
void nfs_handle_cb_pathdown(struct nfs_client *clp);
int nfs_client_return_marked_delegations(struct nfs_client *clp);
int nfs_delegations_present(struct nfs_client *clp);
+void nfs_remove_bad_delegation(struct inode *inode);
void nfs_delegation_mark_reclaim(struct nfs_client *clp);
void nfs_delegation_reap_unclaimed(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 4d7d0ae..5c9b20b 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -327,6 +327,8 @@ extern void nfs4_put_open_state(struct nfs4_state *);
extern void nfs4_close_state(struct nfs4_state *, fmode_t);
extern void nfs4_close_sync(struct nfs4_state *, fmode_t);
extern void nfs4_state_set_mode_locked(struct nfs4_state *, fmode_t);
+extern void nfs_inode_find_state_and_recover(struct inode *inode,
+ const nfs4_stateid *stateid);
extern void nfs4_schedule_lease_recovery(struct nfs_client *);
extern void nfs4_schedule_state_manager(struct nfs_client *);
extern void nfs4_schedule_path_down_recovery(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ec9f6ef..3d26bab 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -265,8 +265,11 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
switch(errorcode) {
case 0:
return 0;
+ case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
+ if (state != NULL)
+ nfs_remove_bad_delegation(state->inode);
case -NFS4ERR_OPENMODE:
if (state == NULL)
break;
@@ -1319,8 +1322,11 @@ int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state
* The show must go on: exit, but mark the
* stateid as needing recovery.
*/
+ case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
+ nfs_inode_find_state_and_recover(state->inode,
+ stateid);
nfs4_schedule_stateid_recovery(server, state);
case -EKEYEXPIRED:
/*
@@ -1900,7 +1906,9 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(inode);
- struct nfs4_exception exception = { };
+ struct nfs4_exception exception = {
+ .state = state,
+ };
int err;
do {
err = nfs4_handle_exception(server,
@@ -3714,8 +3722,11 @@ nfs4_async_handle_error(struct rpc_task *task, const struct nfs_server *server,
if (task->tk_status >= 0)
return 0;
switch(task->tk_status) {
+ case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
+ if (state != NULL)
+ nfs_remove_bad_delegation(state->inode);
case -NFS4ERR_OPENMODE:
if (state == NULL)
break;
@@ -4533,7 +4544,9 @@ out:
static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
{
- struct nfs4_exception exception = { };
+ struct nfs4_exception exception = {
+ .state = state,
+ };
int err;
do {
@@ -4626,6 +4639,7 @@ int nfs4_lock_delegation_recall(struct nfs4_state *state, struct file_lock *fl)
* The show must go on: exit, but mark the
* stateid as needing recovery.
*/
+ case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
case -NFS4ERR_OPENMODE:
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 4539203..0e60df1 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1132,12 +1132,37 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
{
struct nfs_client *clp = server->nfs_client;
- if (test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
- nfs_async_inode_return_delegation(state->inode, &state->stateid);
nfs4_state_mark_reclaim_nograce(clp, state);
nfs4_schedule_state_manager(clp);
}
+void nfs_inode_find_state_and_recover(struct inode *inode,
+ const nfs4_stateid *stateid)
+{
+ struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
+ struct nfs_inode *nfsi = NFS_I(inode);
+ struct nfs_open_context *ctx;
+ struct nfs4_state *state;
+ bool found = false;
+
+ spin_lock(&inode->i_lock);
+ list_for_each_entry(ctx, &nfsi->open_files, list) {
+ state = ctx->state;
+ if (state == NULL)
+ continue;
+ if (!test_bit(NFS_DELEGATED_STATE, &state->flags))
+ continue;
+ if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
+ continue;
+ nfs4_state_mark_reclaim_nograce(clp, state);
+ found = true;
+ }
+ spin_unlock(&inode->i_lock);
+ if (found)
+ nfs4_schedule_state_manager(clp);
+}
+
+
static int nfs4_reclaim_locks(struct nfs4_state *state, const struct nfs4_state_recovery_ops *ops)
{
struct inode *inode = state->inode;
--
1.7.7.6
On Thu, Mar 08, 2012 at 09:02:43PM +0000, Myklebust, Trond wrote:
> On Thu, 2012-03-08 at 15:57 -0500, J. Bruce Fields wrote:
> > On Thu, Mar 08, 2012 at 08:50:14PM +0000, Myklebust, Trond wrote:
> > > On Thu, 2012-03-08 at 15:42 -0500, J. Bruce Fields wrote:
> > > > On Thu, Mar 08, 2012 at 03:23:34PM -0500, Olga Kornievskaia wrote:
> > > > > On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
> > > > > <[email protected]> wrote:
> > > > > > On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
> > > > > >> wouldn't it be better for you to proactively return a read delegation
> > > > > >> then unnecessarily erroring?
> > > > > >
> > > > > > If nobody else holds a delegation, then the NFS client is actually
> > > > > > allowed to keep its read delegation while writing to the file. It does
> > > > > > admittedly need to request an OPEN stateid for write in that case...
> > > > > > (See section 10.4 of RFC3530bis draft 16)
> > > > >
> > > > > If we both agree that there has to be a request for an open stateid for
> > > > > a write, then instead of returning the read delegation if the client receives
> > > > > err_openmode (when it send the request with read delegation stateid
> > > > > as you said per 3560bis), can't the client resend the setattr with the open
> > > > > stateid? The ordering of the stateid usage is a "should" and not a "must".
> > > > >
> > > > > In rfc5661, it really doesn't make sense to ever send a setattr with
> > > > > a read delegation stateid. According to 9.1.2, the server "MUST" return
> > > > > err_open_mode" error in that case.
> > > > >
> > > > > I gather you are in this case dealing with 4.0 delegations. But I wonder
> > > > > if you'll do something else for 4.1 delegation then?
> > > >
> > > > 3530bis has the same language ("...must verify that the access mode
> > > > allows writing and return an NFS4ERR_OPENMODE error if it does not").
> > >
> > > OK, so we shouldn't send the delegation stateid either for v4 or v4.1.
> > > However should we pre-emptively return the delegation? I've been
> > > assuming not.
> >
> > The server's only legal option is to recall it, so it seems odd not to
> > pre-emptively return--but as you say there's nothing to prevent the
> > server from then handing one right back, possibly before you get a
> > chance to send the setattr.
>
> Why would it need to recall it if this is the only client holding a
> delegation? I agree that for the case of NFSv4 once we don't send the
> stateid, the server has no way to know that this is the same client, but
> the NFSv4.1 server doesn't have that limitation.
>
> > (And the linux server might well do that--maybe it should have some
> > heuristic not to hand out a delegation that was just returned--not so
> > much for this case as just because a return is a sign that the
> > delegation isn't useful to that client.)
>
> Umm... An NFSv4.1 client could simply request a delegation. As I said
> earlier, we don't keep tabs on what other processes are doing.
You're right, I wasn't thinking about the 4.1 case: so in the 4.1 case,
the server does know which client the setattr is coming from, and has
the option not to recall. And in 4.1 you have the option of asking for
no delegation.
So your question was whether to preemptively return in the 4.0 case?
--b.
On Thu, Mar 08, 2012 at 08:50:14PM +0000, Myklebust, Trond wrote:
> On Thu, 2012-03-08 at 15:42 -0500, J. Bruce Fields wrote:
> > On Thu, Mar 08, 2012 at 03:23:34PM -0500, Olga Kornievskaia wrote:
> > > On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
> > > <[email protected]> wrote:
> > > > On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
> > > >> wouldn't it be better for you to proactively return a read delegation
> > > >> then unnecessarily erroring?
> > > >
> > > > If nobody else holds a delegation, then the NFS client is actually
> > > > allowed to keep its read delegation while writing to the file. It does
> > > > admittedly need to request an OPEN stateid for write in that case...
> > > > (See section 10.4 of RFC3530bis draft 16)
> > >
> > > If we both agree that there has to be a request for an open stateid for
> > > a write, then instead of returning the read delegation if the client receives
> > > err_openmode (when it send the request with read delegation stateid
> > > as you said per 3560bis), can't the client resend the setattr with the open
> > > stateid? The ordering of the stateid usage is a "should" and not a "must".
> > >
> > > In rfc5661, it really doesn't make sense to ever send a setattr with
> > > a read delegation stateid. According to 9.1.2, the server "MUST" return
> > > err_open_mode" error in that case.
> > >
> > > I gather you are in this case dealing with 4.0 delegations. But I wonder
> > > if you'll do something else for 4.1 delegation then?
> >
> > 3530bis has the same language ("...must verify that the access mode
> > allows writing and return an NFS4ERR_OPENMODE error if it does not").
>
> OK, so we shouldn't send the delegation stateid either for v4 or v4.1.
> However should we pre-emptively return the delegation? I've been
> assuming not.
The server's only legal option is to recall it, so it seems odd not to
pre-emptively return--but as you say there's nothing to prevent the
server from then handing one right back, possibly before you get a
chance to send the setattr.
(And the linux server might well do that--maybe it should have some
heuristic not to hand out a delegation that was just returned--not so
much for this case as just because a return is a sign that the
delegation isn't useful to that client.)
--b.
T24gVGh1LCAyMDEyLTAzLTA4IGF0IDEyOjUyIC0wNTAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90
ZToNCj4gd291bGRuJ3QgaXQgYmUgYmV0dGVyIGZvciB5b3UgdG8gcHJvYWN0aXZlbHkgcmV0dXJu
IGEgcmVhZCBkZWxlZ2F0aW9uDQo+IHRoZW4gdW5uZWNlc3NhcmlseSBlcnJvcmluZz8NCg0KSWYg
bm9ib2R5IGVsc2UgaG9sZHMgYSBkZWxlZ2F0aW9uLCB0aGVuIHRoZSBORlMgY2xpZW50IGlzIGFj
dHVhbGx5DQphbGxvd2VkIHRvIGtlZXAgaXRzIHJlYWQgZGVsZWdhdGlvbiB3aGlsZSB3cml0aW5n
IHRvIHRoZSBmaWxlLiBJdCBkb2VzDQphZG1pdHRlZGx5IG5lZWQgdG8gcmVxdWVzdCBhbiBPUEVO
IHN0YXRlaWQgZm9yIHdyaXRlIGluIHRoYXQgY2FzZS4uLg0KKFNlZSBzZWN0aW9uIDEwLjQgb2Yg
UkZDMzUzMGJpcyBkcmFmdCAxNikNCg0KVGhhdCBzYWlkLCBpbiBlaXRoZXIgY2FzZSB3ZSBkbyBu
ZWVkIHRvIGRlYWwgd2l0aCB0aGUgZmFjdCB0aGF0IGEgbmV3DQpkZWxlZ2F0aW9uIG1heSBiZSBn
cmFudGVkIGFmdGVyIHdlIHJldHVybiB0aGUgb2xkIG9uZS4gVGhlcmUgaXMgbm8NCmxvY2tpbmcg
YXJvdW5kIHRoZSBzZXRhdHRyIGNhbGwgdG8gcHJldmVudCB0aGlzLg0KDQo+IGkgYWxzbyBkb24n
dCB1bmRlcnN0YW5kIGhvdyB0aGlzIGVycm9yIG9jY3Vycy4gZG9pbmcgYSBzZXRhdHRyIGluIHRo
aXMNCj4gY2FzZSB5b3UgbXVzdCBoYXZlIHVzZWQgYSBub24tc3BlY2lhbCBzdGF0ZWlkLiB0aGUg
c2VydmVyIHdvdWxkIG9ubHkNCj4gcmV0dXJuIGFuIGVycl9vcGVubW9kZSBpZiB5b3Ugc2VudCB0
aGUgc2V0YXR0ciB3aXRoIGEgcmVhZCBkZWxlZ2F0aW9uDQo+IHN0YXRlaWQuIGkgZ3Vlc3MgbXkg
cXVlc3Rpb24gaXMgd2hhdCBzdGF0ZWlkIHdvdWxkIHlvdSB1c2UgdGhhdCBmcm9tDQo+IGNsaWVu
dCdzIHBlcnNwZWN0aXZlIHJlcHJlc2VudCBhIHdyaXRlLXR5cGUgc3RhdGUgaWQgYnV0IHlldCBh
IHNlcnZlcg0KPiB3b3VsZCBmbGFnIGFzIGhhdmluZyB3cm9uZyBhY2Nlc3MgdHlwZT8NCg0KVGhl
IHJlYWQgZGVsZWdhdGlvbiBzdGF0ZWlkIGlzIGJlaW5nIHNlbnQgYXMgcGVyIHRoZSBwcmVzY3Jp
cHRpb24gaW4NCnNlY3Rpb24gOS4xLjMuNiBvZiBSRkMzNTMwYmlzLg0KDQo+IGFsc28gaSdtIGN1
cmlvdXMgd2h5IHdvdWxkIGEgc2VydmVyLCBpbnN0ZWFkIG9mIHJldHVybmluZw0KPiBlcnJfb3Bl
bm1vZGUsIHdvdWxkIG5vdCBmaXJzdCByZWNhbGwgeW91ciByZWFkIGRlbGVnYXRpb24/DQoNCkl0
IGNvdWxkLCBidXQgd2h5IGRvIHNvIHdoZW4gaXQgY2FuIGp1c3QgcmV0dXJuIGFuIGVycm9yIHZh
bHVlPyBUaGUNCnByZXNlbmNlIG9mIHRoZSBkZWxlZ2F0aW9uIHN0YXRlaWQgaW4gdGhlIFNFVEFU
VFIgcmVxdWVzdCBhbGxvd3MgaXQgdG8NCmNvbW11bmljYXRlIGRpcmVjdGx5IHRvIHRoZSBjbGll
bnQgd2hhdCB0aGUgcHJvYmxlbSBpcyB3aXRob3V0IHRoZSBuZWVkDQpmb3IgYW55IGNhbGxiYWNr
cy4NCg0KQ2hlZXJzDQogIFRyb25kDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMg
Y2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0K
d3d3Lm5ldGFwcC5jb20NCg0K
On Thu, Mar 8, 2012 at 3:50 PM, Myklebust, Trond
<[email protected]> wrote:
> On Thu, 2012-03-08 at 15:42 -0500, J. Bruce Fields wrote:
>> On Thu, Mar 08, 2012 at 03:23:34PM -0500, Olga Kornievskaia wrote:
>> > On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
>> > <[email protected]> wrote:
>> > > On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
>> > >> wouldn't it be better for you to proactively return a read delegation
>> > >> then unnecessarily erroring?
>> > >
>> > > If nobody else holds a delegation, then the NFS client is actually
>> > > allowed to keep its read delegation while writing to the file. It does
>> > > admittedly need to request an OPEN stateid for write in that case...
>> > > (See section 10.4 of RFC3530bis draft 16)
>> >
>> > If we both agree that there has to be a request for an open stateid for
>> > a write, then instead of returning the read delegation if the client receives
>> > err_openmode (when it send the request with read delegation stateid
>> > as you said per 3560bis), can't the client resend the setattr with the open
>> > stateid? The ordering of the stateid usage is a "should" and not a "must".
>> >
>> > In rfc5661, it really doesn't make sense to ever send a setattr with
>> > a read delegation stateid. According to 9.1.2, the server "MUST" return
>> > err_open_mode" error in that case.
>> >
>> > I gather you are in this case dealing with 4.0 delegations. But I wonder
>> > if you'll do something else for 4.1 delegation then?
>>
>> 3530bis has the same language ("...must verify that the access mode
>> allows writing and return an NFS4ERR_OPENMODE error if it does not").
>
> OK, so we shouldn't send the delegation stateid either for v4 or v4.1.
> However should we pre-emptively return the delegation? I've been
> assuming not.
It would be nice not to pre-emptively return delegations but for that
we need server implementors to get on board with the idea.
T24gVGh1LCAyMDEyLTAzLTA4IGF0IDE1OjU3IC0wNTAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIFRodSwgTWFyIDA4LCAyMDEyIGF0IDA4OjUwOjE0UE0gKzAwMDAsIE15a2xlYnVzdCwg
VHJvbmQgd3JvdGU6DQo+ID4gT24gVGh1LCAyMDEyLTAzLTA4IGF0IDE1OjQyIC0wNTAwLCBKLiBC
cnVjZSBGaWVsZHMgd3JvdGU6DQo+ID4gPiBPbiBUaHUsIE1hciAwOCwgMjAxMiBhdCAwMzoyMzoz
NFBNIC0wNTAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiA+ID4gT24gVGh1LCBNYXIg
OCwgMjAxMiBhdCAxOjE1IFBNLCBNeWtsZWJ1c3QsIFRyb25kDQo+ID4gPiA+IDxUcm9uZC5NeWts
ZWJ1c3RAbmV0YXBwLmNvbT4gd3JvdGU6DQo+ID4gPiA+ID4gT24gVGh1LCAyMDEyLTAzLTA4IGF0
IDEyOjUyIC0wNTAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiA+ID4gPj4gd291bGRu
J3QgaXQgYmUgYmV0dGVyIGZvciB5b3UgdG8gcHJvYWN0aXZlbHkgcmV0dXJuIGEgcmVhZCBkZWxl
Z2F0aW9uDQo+ID4gPiA+ID4+IHRoZW4gdW5uZWNlc3NhcmlseSBlcnJvcmluZz8NCj4gPiA+ID4g
Pg0KPiA+ID4gPiA+IElmIG5vYm9keSBlbHNlIGhvbGRzIGEgZGVsZWdhdGlvbiwgdGhlbiB0aGUg
TkZTIGNsaWVudCBpcyBhY3R1YWxseQ0KPiA+ID4gPiA+IGFsbG93ZWQgdG8ga2VlcCBpdHMgcmVh
ZCBkZWxlZ2F0aW9uIHdoaWxlIHdyaXRpbmcgdG8gdGhlIGZpbGUuIEl0IGRvZXMNCj4gPiA+ID4g
PiBhZG1pdHRlZGx5IG5lZWQgdG8gcmVxdWVzdCBhbiBPUEVOIHN0YXRlaWQgZm9yIHdyaXRlIGlu
IHRoYXQgY2FzZS4uLg0KPiA+ID4gPiA+IChTZWUgc2VjdGlvbiAxMC40IG9mIFJGQzM1MzBiaXMg
ZHJhZnQgMTYpDQo+ID4gPiA+IA0KPiA+ID4gPiBJZiB3ZSBib3RoIGFncmVlIHRoYXQgdGhlcmUg
aGFzIHRvIGJlIGEgcmVxdWVzdCBmb3IgYW4gb3BlbiBzdGF0ZWlkIGZvcg0KPiA+ID4gPiBhIHdy
aXRlLCB0aGVuIGluc3RlYWQgb2YgcmV0dXJuaW5nIHRoZSByZWFkIGRlbGVnYXRpb24gaWYgdGhl
IGNsaWVudCByZWNlaXZlcw0KPiA+ID4gPiBlcnJfb3Blbm1vZGUgKHdoZW4gaXQgc2VuZCB0aGUg
cmVxdWVzdCB3aXRoIHJlYWQgZGVsZWdhdGlvbiBzdGF0ZWlkDQo+ID4gPiA+IGFzIHlvdSBzYWlk
IHBlciAzNTYwYmlzKSwgY2FuJ3QgdGhlIGNsaWVudCByZXNlbmQgdGhlIHNldGF0dHIgd2l0aCB0
aGUgb3Blbg0KPiA+ID4gPiBzdGF0ZWlkPyBUaGUgb3JkZXJpbmcgb2YgdGhlIHN0YXRlaWQgdXNh
Z2UgaXMgYSAic2hvdWxkIiBhbmQgbm90IGEgIm11c3QiLg0KPiA+ID4gPiANCj4gPiA+ID4gSW4g
cmZjNTY2MSwgaXQgcmVhbGx5IGRvZXNuJ3QgbWFrZSBzZW5zZSB0byBldmVyIHNlbmQgYSBzZXRh
dHRyIHdpdGgNCj4gPiA+ID4gYSByZWFkIGRlbGVnYXRpb24gc3RhdGVpZC4gQWNjb3JkaW5nIHRv
IDkuMS4yLCB0aGUgc2VydmVyICJNVVNUIiByZXR1cm4NCj4gPiA+ID4gZXJyX29wZW5fbW9kZSIg
ZXJyb3IgaW4gdGhhdCBjYXNlLg0KPiA+ID4gPiANCj4gPiA+ID4gSSBnYXRoZXIgeW91IGFyZSBp
biB0aGlzIGNhc2UgZGVhbGluZyB3aXRoIDQuMCBkZWxlZ2F0aW9ucy4gQnV0IEkgd29uZGVyDQo+
ID4gPiA+IGlmIHlvdSdsbCBkbyBzb21ldGhpbmcgZWxzZSBmb3IgNC4xIGRlbGVnYXRpb24gdGhl
bj8NCj4gPiA+IA0KPiA+ID4gMzUzMGJpcyBoYXMgdGhlIHNhbWUgbGFuZ3VhZ2UgKCIuLi5tdXN0
IHZlcmlmeSB0aGF0IHRoZSBhY2Nlc3MgbW9kZQ0KPiA+ID4gYWxsb3dzIHdyaXRpbmcgYW5kIHJl
dHVybiBhbiBORlM0RVJSX09QRU5NT0RFIGVycm9yIGlmIGl0IGRvZXMgbm90IikuDQo+ID4gDQo+
ID4gT0ssIHNvIHdlIHNob3VsZG4ndCBzZW5kIHRoZSBkZWxlZ2F0aW9uIHN0YXRlaWQgZWl0aGVy
IGZvciB2NCBvciB2NC4xLg0KPiA+IEhvd2V2ZXIgc2hvdWxkIHdlIHByZS1lbXB0aXZlbHkgcmV0
dXJuIHRoZSBkZWxlZ2F0aW9uPyBJJ3ZlIGJlZW4NCj4gPiBhc3N1bWluZyBub3QuDQo+IA0KPiBU
aGUgc2VydmVyJ3Mgb25seSBsZWdhbCBvcHRpb24gaXMgdG8gcmVjYWxsIGl0LCBzbyBpdCBzZWVt
cyBvZGQgbm90IHRvDQo+IHByZS1lbXB0aXZlbHkgcmV0dXJuLS1idXQgYXMgeW91IHNheSB0aGVy
ZSdzIG5vdGhpbmcgdG8gcHJldmVudCB0aGUNCj4gc2VydmVyIGZyb20gdGhlbiBoYW5kaW5nIG9u
ZSByaWdodCBiYWNrLCBwb3NzaWJseSBiZWZvcmUgeW91IGdldCBhDQo+IGNoYW5jZSB0byBzZW5k
IHRoZSBzZXRhdHRyLg0KDQpXaHkgd291bGQgaXQgbmVlZCB0byByZWNhbGwgaXQgaWYgdGhpcyBp
cyB0aGUgb25seSBjbGllbnQgaG9sZGluZyBhDQpkZWxlZ2F0aW9uPyBJIGFncmVlIHRoYXQgZm9y
IHRoZSBjYXNlIG9mIE5GU3Y0IG9uY2Ugd2UgZG9uJ3Qgc2VuZCB0aGUNCnN0YXRlaWQsIHRoZSBz
ZXJ2ZXIgaGFzIG5vIHdheSB0byBrbm93IHRoYXQgdGhpcyBpcyB0aGUgc2FtZSBjbGllbnQsIGJ1
dA0KdGhlIE5GU3Y0LjEgc2VydmVyIGRvZXNuJ3QgaGF2ZSB0aGF0IGxpbWl0YXRpb24uDQoNCj4g
KEFuZCB0aGUgbGludXggc2VydmVyIG1pZ2h0IHdlbGwgZG8gdGhhdC0tbWF5YmUgaXQgc2hvdWxk
IGhhdmUgc29tZQ0KPiBoZXVyaXN0aWMgbm90IHRvIGhhbmQgb3V0IGEgZGVsZWdhdGlvbiB0aGF0
IHdhcyBqdXN0IHJldHVybmVkLS1ub3Qgc28NCj4gbXVjaCBmb3IgdGhpcyBjYXNlIGFzIGp1c3Qg
YmVjYXVzZSBhIHJldHVybiBpcyBhIHNpZ24gdGhhdCB0aGUNCj4gZGVsZWdhdGlvbiBpc24ndCB1
c2VmdWwgdG8gdGhhdCBjbGllbnQuKQ0KDQpVbW0uLi4gQW4gTkZTdjQuMSBjbGllbnQgY291bGQg
c2ltcGx5IHJlcXVlc3QgYSBkZWxlZ2F0aW9uLiBBcyBJIHNhaWQNCmVhcmxpZXIsIHdlIGRvbid0
IGtlZXAgdGFicyBvbiB3aGF0IG90aGVyIHByb2Nlc3NlcyBhcmUgZG9pbmcuDQoNCi0tIA0KVHJv
bmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9u
ZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
T24gVGh1LCAyMDEyLTAzLTA4IGF0IDE1OjQyIC0wNTAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIFRodSwgTWFyIDA4LCAyMDEyIGF0IDAzOjIzOjM0UE0gLTA1MDAsIE9sZ2EgS29ybmll
dnNrYWlhIHdyb3RlOg0KPiA+IE9uIFRodSwgTWFyIDgsIDIwMTIgYXQgMToxNSBQTSwgTXlrbGVi
dXN0LCBUcm9uZA0KPiA+IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4gd3JvdGU6DQo+ID4g
PiBPbiBUaHUsIDIwMTItMDMtMDggYXQgMTI6NTIgLTA1MDAsIE9sZ2EgS29ybmlldnNrYWlhIHdy
b3RlOg0KPiA+ID4+IHdvdWxkbid0IGl0IGJlIGJldHRlciBmb3IgeW91IHRvIHByb2FjdGl2ZWx5
IHJldHVybiBhIHJlYWQgZGVsZWdhdGlvbg0KPiA+ID4+IHRoZW4gdW5uZWNlc3NhcmlseSBlcnJv
cmluZz8NCj4gPiA+DQo+ID4gPiBJZiBub2JvZHkgZWxzZSBob2xkcyBhIGRlbGVnYXRpb24sIHRo
ZW4gdGhlIE5GUyBjbGllbnQgaXMgYWN0dWFsbHkNCj4gPiA+IGFsbG93ZWQgdG8ga2VlcCBpdHMg
cmVhZCBkZWxlZ2F0aW9uIHdoaWxlIHdyaXRpbmcgdG8gdGhlIGZpbGUuIEl0IGRvZXMNCj4gPiA+
IGFkbWl0dGVkbHkgbmVlZCB0byByZXF1ZXN0IGFuIE9QRU4gc3RhdGVpZCBmb3Igd3JpdGUgaW4g
dGhhdCBjYXNlLi4uDQo+ID4gPiAoU2VlIHNlY3Rpb24gMTAuNCBvZiBSRkMzNTMwYmlzIGRyYWZ0
IDE2KQ0KPiA+IA0KPiA+IElmIHdlIGJvdGggYWdyZWUgdGhhdCB0aGVyZSBoYXMgdG8gYmUgYSBy
ZXF1ZXN0IGZvciBhbiBvcGVuIHN0YXRlaWQgZm9yDQo+ID4gYSB3cml0ZSwgdGhlbiBpbnN0ZWFk
IG9mIHJldHVybmluZyB0aGUgcmVhZCBkZWxlZ2F0aW9uIGlmIHRoZSBjbGllbnQgcmVjZWl2ZXMN
Cj4gPiBlcnJfb3Blbm1vZGUgKHdoZW4gaXQgc2VuZCB0aGUgcmVxdWVzdCB3aXRoIHJlYWQgZGVs
ZWdhdGlvbiBzdGF0ZWlkDQo+ID4gYXMgeW91IHNhaWQgcGVyIDM1NjBiaXMpLCBjYW4ndCB0aGUg
Y2xpZW50IHJlc2VuZCB0aGUgc2V0YXR0ciB3aXRoIHRoZSBvcGVuDQo+ID4gc3RhdGVpZD8gVGhl
IG9yZGVyaW5nIG9mIHRoZSBzdGF0ZWlkIHVzYWdlIGlzIGEgInNob3VsZCIgYW5kIG5vdCBhICJt
dXN0Ii4NCj4gPiANCj4gPiBJbiByZmM1NjYxLCBpdCByZWFsbHkgZG9lc24ndCBtYWtlIHNlbnNl
IHRvIGV2ZXIgc2VuZCBhIHNldGF0dHIgd2l0aA0KPiA+IGEgcmVhZCBkZWxlZ2F0aW9uIHN0YXRl
aWQuIEFjY29yZGluZyB0byA5LjEuMiwgdGhlIHNlcnZlciAiTVVTVCIgcmV0dXJuDQo+ID4gZXJy
X29wZW5fbW9kZSIgZXJyb3IgaW4gdGhhdCBjYXNlLg0KPiA+IA0KPiA+IEkgZ2F0aGVyIHlvdSBh
cmUgaW4gdGhpcyBjYXNlIGRlYWxpbmcgd2l0aCA0LjAgZGVsZWdhdGlvbnMuIEJ1dCBJIHdvbmRl
cg0KPiA+IGlmIHlvdSdsbCBkbyBzb21ldGhpbmcgZWxzZSBmb3IgNC4xIGRlbGVnYXRpb24gdGhl
bj8NCj4gDQo+IDM1MzBiaXMgaGFzIHRoZSBzYW1lIGxhbmd1YWdlICgiLi4ubXVzdCB2ZXJpZnkg
dGhhdCB0aGUgYWNjZXNzIG1vZGUNCj4gYWxsb3dzIHdyaXRpbmcgYW5kIHJldHVybiBhbiBORlM0
RVJSX09QRU5NT0RFIGVycm9yIGlmIGl0IGRvZXMgbm90IikuDQoNCk9LLCBzbyB3ZSBzaG91bGRu
J3Qgc2VuZCB0aGUgZGVsZWdhdGlvbiBzdGF0ZWlkIGVpdGhlciBmb3IgdjQgb3IgdjQuMS4NCkhv
d2V2ZXIgc2hvdWxkIHdlIHByZS1lbXB0aXZlbHkgcmV0dXJuIHRoZSBkZWxlZ2F0aW9uPyBJJ3Zl
IGJlZW4NCmFzc3VtaW5nIG5vdC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBj
bGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3
d3cubmV0YXBwLmNvbQ0KDQo=
T24gVHVlLCAyMDEyLTAzLTA2IGF0IDExOjEwICswMTAwLCBNaWtsb3MgU3plcmVkaSB3cm90ZToN
Cj4gVGhlIGF0dGFjaGVkIHRlc3QgcHJvZ3JhbSByZWxpYWJseSBmYWlscyBvbiBhbiBORlN2NCBt
b3VudC4NCj4gDQo+ICMgbW91bnQgLXRuZnMgLW9uZnN2ZXJzPTQgMTI3LjAuMC4xOi8gL21udC9u
ZnMNCj4gIyAuL3RydW5jYXRlLXRlc3QgL21udC9uZnMvdG1wL3h5eg0KPiB0cnVuY2F0ZTogSW5w
dXQvb3V0cHV0IGVycm9yDQo+IA0KPiBUaGFua3MsDQo+IE1pa2xvcw0KDQpUaGFua3MhIEknbGwg
c2VlIGlmIEkgY2FuIHJlcHJvZHVjZSBhbmQgZmlndXJlIG91dCB3aGF0J3MgZ29pbmcgd3Jvbmcu
DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0K
TmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
If a setattr() fails because of an NFS4ERR_OPENMODE error, it is
probably due to us holding a read delegation. Ensure that the
recovery routines return that delegation in this case.
Reported-by: Miklos Szeredi <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
Cc: [email protected]
---
fs/nfs/nfs4_fs.h | 1 +
fs/nfs/nfs4proc.c | 13 ++++++++++++-
2 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 5c9b20b..8ccaf24 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -193,6 +193,7 @@ struct nfs4_exception {
long timeout;
int retry;
struct nfs4_state *state;
+ struct inode *inode;
};
struct nfs4_state_recovery_ops {
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3d26bab..bfcaa03 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -259,18 +259,28 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
{
struct nfs_client *clp = server->nfs_client;
struct nfs4_state *state = exception->state;
+ struct inode *inode = exception->inode;
int ret = errorcode;
exception->retry = 0;
switch(errorcode) {
case 0:
return 0;
+ case -NFS4ERR_OPENMODE:
+ if (nfs_have_delegation(inode, FMODE_READ)) {
+ nfs_inode_return_delegation(inode);
+ exception->retry = 1;
+ return 0;
+ }
+ if (state == NULL)
+ break;
+ nfs4_schedule_stateid_recovery(server, state);
+ goto wait_on_recovery;
case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
if (state != NULL)
nfs_remove_bad_delegation(state->inode);
- case -NFS4ERR_OPENMODE:
if (state == NULL)
break;
nfs4_schedule_stateid_recovery(server, state);
@@ -1908,6 +1918,7 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
struct nfs_server *server = NFS_SERVER(inode);
struct nfs4_exception exception = {
.state = state,
+ .inode = inode,
};
int err;
do {
--
1.7.7.6
On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
<[email protected]> wrote:
> On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
>> wouldn't it be better for you to proactively return a read delegation
>> then unnecessarily erroring?
>
> If nobody else holds a delegation, then the NFS client is actually
> allowed to keep its read delegation while writing to the file. It does
> admittedly need to request an OPEN stateid for write in that case...
> (See section 10.4 of RFC3530bis draft 16)
If we both agree that there has to be a request for an open stateid for
a write, then instead of returning the read delegation if the client receives
err_openmode (when it send the request with read delegation stateid
as you said per 3560bis), can't the client resend the setattr with the open
stateid? The ordering of the stateid usage is a "should" and not a "must".
In rfc5661, it really doesn't make sense to ever send a setattr with
a read delegation stateid. According to 9.1.2, the server "MUST" return
err_open_mode" error in that case.
I gather you are in this case dealing with 4.0 delegations. But I wonder
if you'll do something else for 4.1 delegation then?
Another comment is that I was suggesting a return of delegation only
because (from what i recall) the 4.1 servers out there will recall the
previously
given read delegation in this case.
> That said, in either case we do need to deal with the fact that a new
> delegation may be granted after we return the old one. There is no
> locking around the setattr call to prevent this.
Can that really happen since we agreed that the client also has an open
stateid for write in this case?
>> i also don't understand how this error occurs. doing a setattr in this
>> case you must have used a non-special stateid. the server would only
>> return an err_openmode if you sent the setattr with a read delegation
>> stateid. i guess my question is what stateid would you use that from
>> client's perspective represent a write-type state id but yet a server
>> would flag as having wrong access type?
>
> The read delegation stateid is being sent as per the prescription in
> section 9.1.3.6 of RFC3530bis.
>> also i'm curious why would a server, instead of returning
>> err_openmode, would not first recall your read delegation?
>
> It could, but why do so when it can just return an error value? The
> presence of the delegation stateid in the SETATTR request allows it to
> communicate directly to the client what the problem is without the need
> for any callbacks.
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
On Thu, Mar 08, 2012 at 03:23:34PM -0500, Olga Kornievskaia wrote:
> On Thu, Mar 8, 2012 at 1:15 PM, Myklebust, Trond
> <[email protected]> wrote:
> > On Thu, 2012-03-08 at 12:52 -0500, Olga Kornievskaia wrote:
> >> wouldn't it be better for you to proactively return a read delegation
> >> then unnecessarily erroring?
> >
> > If nobody else holds a delegation, then the NFS client is actually
> > allowed to keep its read delegation while writing to the file. It does
> > admittedly need to request an OPEN stateid for write in that case...
> > (See section 10.4 of RFC3530bis draft 16)
>
> If we both agree that there has to be a request for an open stateid for
> a write, then instead of returning the read delegation if the client receives
> err_openmode (when it send the request with read delegation stateid
> as you said per 3560bis), can't the client resend the setattr with the open
> stateid? The ordering of the stateid usage is a "should" and not a "must".
>
> In rfc5661, it really doesn't make sense to ever send a setattr with
> a read delegation stateid. According to 9.1.2, the server "MUST" return
> err_open_mode" error in that case.
>
> I gather you are in this case dealing with 4.0 delegations. But I wonder
> if you'll do something else for 4.1 delegation then?
3530bis has the same language ("...must verify that the access mode
allows writing and return an NFS4ERR_OPENMODE error if it does not").
--b.
>
> Another comment is that I was suggesting a return of delegation only
> because (from what i recall) the 4.1 servers out there will recall the
> previously
> given read delegation in this case.
>
> > That said, in either case we do need to deal with the fact that a new
> > delegation may be granted after we return the old one. There is no
> > locking around the setattr call to prevent this.
>
> Can that really happen since we agreed that the client also has an open
> stateid for write in this case?
>
> >> i also don't understand how this error occurs. doing a setattr in this
> >> case you must have used a non-special stateid. the server would only
> >> return an err_openmode if you sent the setattr with a read delegation
> >> stateid. i guess my question is what stateid would you use that from
> >> client's perspective represent a write-type state id but yet a server
> >> would flag as having wrong access type?
> >
> > The read delegation stateid is being sent as per the prescription in
> > section 9.1.3.6 of RFC3530bis.
>
> >> also i'm curious why would a server, instead of returning
> >> err_openmode, would not first recall your read delegation?
> >
> > It could, but why do so when it can just return an error value? The
> > presence of the delegation stateid in the SETATTR request allows it to
> > communicate directly to the client what the problem is without the need
> > for any callbacks.
>
> > Trond Myklebust
> > Linux NFS client maintainer
> >
> > NetApp
> > [email protected]
> > http://www.netapp.com
> >
> --
> 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
Myklebust, Trond [[email protected]] wrote:
> On Tue, 2012-03-06 at 11:10 +0100, Miklos Szeredi wrote:
> > The attached test program reliably fails on an NFSv4 mount.
> >
> > # mount -tnfs -onfsvers=4 127.0.0.1:/ /mnt/nfs
> > # ./truncate-test /mnt/nfs/tmp/xyz
> > truncate: Input/output error
> >
> > Thanks,
> > Miklos
>
> Thanks! I'll see if I can reproduce and figure out what's going wrong.
I could reproduce on RHEL6.2. If I remove the second open (O_RDONLY),
then it works fine.
If I do "chmod" (probably others too) on that failing file, then truncate
works on it. Looks like some kind of attribute validation issue, I will
test it more if I get a chance.
Regards, Malahal.