Signed-off-by: Jim Rees <[email protected]>
---
fs/nfs/delegation.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index bbbc6bf..5bc4f7e 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -176,9 +176,11 @@ void nfs_inode_reclaim_delegation(struct inode *inode, struct rpc_cred *cred,
static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *delegation, int issync)
{
+ struct nfs_inode *nfsi = NFS_I(inode);
int res = 0;
res = nfs4_proc_delegreturn(inode, delegation->cred, &delegation->stateid, issync);
+ rcu_assign_pointer(nfsi->delegation, NULL);
nfs_free_delegation(delegation);
return res;
}
--
1.7.1
On Mon, Feb 28, 2011 at 07:00:33PM -0500, Trond Myklebust wrote:
> On Mon, 2011-02-28 at 18:22 -0500, Jim Rees wrote:
> > Trond Myklebust wrote:
> >
> > The procedure for returning delegations is supposed to work as follows:
> >
> > 1. Remove the nfsi->delegation so that it is no longer visible to
> > new open() requests
> > 2. write back any dirty data to the server
> > 3. Reclaim any locks
> > 4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
> > 5. delegreturn
> >
> > While there may indeed be the odd READ or WRITE that races between 4.
> > and 5., so that the server receives the delegation stateid after the
> > delegreturn, that shouldn't matter: the server returns an error, and the
> > client should retry using the new open stateid.
> >
> > What is failing to work correctly here?
> >
> > That helps, thanks. I'll see if I can figure out what's going wrong.
> >
> > At the server, I see a delegreturn followed by a setattr using the returned
> > stateid. The server returns BAD_STATEID. I'll look to see if the client
> > then retries.
> >
> > At the client, I see a non-null nfsi->delegation after the delgreturn, and
> > the application gets a EIO.
>
> That's because your server is confusing the hell out of us all by giving
> out a delegation as part of the reply (in frame 5328) to the
> OPEN(CLAIM_DELEGATE_CUR) in frame 5325.
>
> IOW: the client is in the process of returning the delegation, and asks
> to trade in the delegation stateid into an open stateid, then the server
> replies with an open stateid, and the delegation stateid...
Which server is this, Jim?
(I checked the Linux server quickly and it doesn't look like it should
do this....)
--b.
On Mon, 2011-02-28 at 18:22 -0500, Jim Rees wrote:
> Trond Myklebust wrote:
>
> The procedure for returning delegations is supposed to work as follows:
>
> 1. Remove the nfsi->delegation so that it is no longer visible to
> new open() requests
> 2. write back any dirty data to the server
> 3. Reclaim any locks
> 4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
> 5. delegreturn
>
> While there may indeed be the odd READ or WRITE that races between 4.
> and 5., so that the server receives the delegation stateid after the
> delegreturn, that shouldn't matter: the server returns an error, and the
> client should retry using the new open stateid.
>
> What is failing to work correctly here?
>
> That helps, thanks. I'll see if I can figure out what's going wrong.
>
> At the server, I see a delegreturn followed by a setattr using the returned
> stateid. The server returns BAD_STATEID. I'll look to see if the client
> then retries.
>
> At the client, I see a non-null nfsi->delegation after the delgreturn, and
> the application gets a EIO.
That's because your server is confusing the hell out of us all by giving
out a delegation as part of the reply (in frame 5328) to the
OPEN(CLAIM_DELEGATE_CUR) in frame 5325.
IOW: the client is in the process of returning the delegation, and asks
to trade in the delegation stateid into an open stateid, then the server
replies with an open stateid, and the delegation stateid...
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
Trond Myklebust wrote:
nfsi->delegation is released under the appropriate locks well before we
get here. The above line is 100% racy and risks clobbering any new
delegation that has been issued after the delegreturn completed...
I'd have been amazed if I'd gotten it right. But there really is a problem
here, the client does try to use delegations that have been returned, and
this patch does solve that problem. I'm happy to keep after this but would
appreciate any suggestions or nudges in the right direction.
On Mon, 2011-02-28 at 16:54 -0500, Jim Rees wrote:
> Trond Myklebust wrote:
>
> nfsi->delegation is released under the appropriate locks well before we
> get here. The above line is 100% racy and risks clobbering any new
> delegation that has been issued after the delegreturn completed...
>
> I'd have been amazed if I'd gotten it right. But there really is a problem
> here, the client does try to use delegations that have been returned, and
> this patch does solve that problem. I'm happy to keep after this but would
> appreciate any suggestions or nudges in the right direction.
The procedure for returning delegations is supposed to work as follows:
1. Remove the nfsi->delegation so that it is no longer visible to
new open() requests
2. write back any dirty data to the server
3. Reclaim any locks
4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
5. delegreturn
While there may indeed be the odd READ or WRITE that races between 4.
and 5., so that the server receives the delegation stateid after the
delegreturn, that shouldn't matter: the server returns an error, and the
client should retry using the new open stateid.
What is failing to work correctly here?
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Mon, 2011-02-28 at 17:01 -0500, Trond Myklebust wrote:
> On Mon, 2011-02-28 at 16:54 -0500, Jim Rees wrote:
> > Trond Myklebust wrote:
> >
> > nfsi->delegation is released under the appropriate locks well before we
> > get here. The above line is 100% racy and risks clobbering any new
> > delegation that has been issued after the delegreturn completed...
> >
> > I'd have been amazed if I'd gotten it right. But there really is a problem
> > here, the client does try to use delegations that have been returned, and
> > this patch does solve that problem. I'm happy to keep after this but would
> > appreciate any suggestions or nudges in the right direction.
>
> The procedure for returning delegations is supposed to work as follows:
>
> 1. Remove the nfsi->delegation so that it is no longer visible to
> new open() requests
> 2. write back any dirty data to the server
> 3. Reclaim any locks
> 4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
Errr.... lock reclaim comes after open reclaim, of course...
The rest is correct.
> 5. delegreturn
>
> While there may indeed be the odd READ or WRITE that races between 4.
> and 5., so that the server receives the delegation stateid after the
> delegreturn, that shouldn't matter: the server returns an error, and the
> client should retry using the new open stateid.
>
> What is failing to work correctly here?
>
Trond Myklebust wrote:
The procedure for returning delegations is supposed to work as follows:
1. Remove the nfsi->delegation so that it is no longer visible to
new open() requests
2. write back any dirty data to the server
3. Reclaim any locks
4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
5. delegreturn
While there may indeed be the odd READ or WRITE that races between 4.
and 5., so that the server receives the delegation stateid after the
delegreturn, that shouldn't matter: the server returns an error, and the
client should retry using the new open stateid.
What is failing to work correctly here?
That helps, thanks. I'll see if I can figure out what's going wrong.
At the server, I see a delegreturn followed by a setattr using the returned
stateid. The server returns BAD_STATEID. I'll look to see if the client
then retries.
At the client, I see a non-null nfsi->delegation after the delgreturn, and
the application gets a EIO.
There is a pcap here:
http://www.citi.umich.edu/projects/nfsv4/pnfs/block/bad-deleg.pcap
The stateid is 1880654d0300970000000400, the delegreturn is in packet 5589,
the client tries to use it for setattr in packet 5596. I showed this to
Andy and Fred earlier, they may already be looking at it.
The client is running Benny's pnfs-all-latest.
On Mon, 2011-02-28 at 19:00 -0500, Trond Myklebust wrote:
> That's because your server is confusing the hell out of us all by giving
> out a delegation as part of the reply (in frame 5328) to the
> OPEN(CLAIM_DELEGATE_CUR) in frame 5325.
>
> IOW: the client is in the process of returning the delegation, and asks
> to trade in the delegation stateid into an open stateid, then the server
> replies with an open stateid, and the delegation stateid...
BTW: it is interesting to note that the only place in the spec where it
is explicitly stateid that the server should not send a delegation in
the case of CLAIM_DELEGATE_CUR is in section 8.1.8 (Use of Open
Confirmation).
Perhaps we should add some words in section 14.2.16 to that effect, Tom?
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Mon, 2011-02-28 at 16:31 -0500, Jim Rees wrote:
> Signed-off-by: Jim Rees <[email protected]>
> ---
> fs/nfs/delegation.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index bbbc6bf..5bc4f7e 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -176,9 +176,11 @@ void nfs_inode_reclaim_delegation(struct inode *inode, struct rpc_cred *cred,
>
> static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *delegation, int issync)
> {
> + struct nfs_inode *nfsi = NFS_I(inode);
> int res = 0;
>
> res = nfs4_proc_delegreturn(inode, delegation->cred, &delegation->stateid, issync);
> + rcu_assign_pointer(nfsi->delegation, NULL);
> nfs_free_delegation(delegation);
> return res;
> }
Big NACK...
nfsi->delegation is released under the appropriate locks well before we
get here. The above line is 100% racy and risks clobbering any new
delegation that has been issued after the delegreturn completed...
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVHJvbmQgTXlrbGVidXN0
IFttYWlsdG86VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb21dDQo+IFNlbnQ6IG1hcmRpIDEgbWFy
cyAyMDExIDEzOjE2DQo+IFRvOiBHYXJkZXJlLCBEYW5pZWwNCj4gQ2M6IGJmaWVsZHNAZmllbGRz
ZXMub3JnOyByZWVzQHVtaWNoLmVkdTsgYmhhbGV2eUBwYW5hc2FzLmNvbTsgbGludXgtDQo+IG5m
c0B2Z2VyLmtlcm5lbC5vcmc7IGhvbmV5QGNpdGkudW1pY2guZWR1DQo+IFN1YmplY3Q6IFJFOiBb
UEFUQ0hdIHplcm8gb3V0IGRlbGVnYXRpb24gaW4gdGhlIGlub2RlIGFmdGVyIGl0IGhhcyBiZWVu
DQo+IHJldHVybmVkDQo+IA0KPiBPbiBUdWUsIDIwMTEtMDMtMDEgYXQgMDQ6NTIgLTA1MDAsIGRh
bmllbC5nYXJkZXJlQGVtYy5jb20gd3JvdGU6DQo+ID4gSXQgaXMgRU1DIHNlcnZlci4NCj4gPg0K
PiA+IFRoZSBzZXJ2ZXIgcmV0dXJucyB0aGUgZGVsZWdhdGlvbiB0byB0aGUgT1BFTiBDTEFJTV9E
RUxFR0FURV9DVVINCj4gb3BlcmF0aW9uLiBBdCB0aGlzIHBvaW50IHRoZSBkZWxlZ2F0aW9uIGlz
IHN0aWxsIHZhbGlkLiBUaGlzIGlzIGZhcg0KPiBiZWZvcmUgdGhlIERFTEVHUkVUVVJOOyBzbyB0
aGlzIGRvZXNuJ3QgZXhwbGFpbiB3aHkNCj4gPiB0aGUgZGVsZWdhdGlvbiBpcyB1c2VkIGluIFNF
VEFUVFIgKGZyYW1lIDU1OTYpIGFmdGVyIHRoZSBERUxFR1JFVFVSTg0KPiAoZnJhbWUgNTU4OSwg
cmVwbHkgaW4gNTU5MikuDQo+IA0KPiBJdCBleHBsYWlucyBfZXhhY3RseV8gd2hhdCBpcyBoYXBw
ZW5pbmcuLi4NCj4gDQo+IFRoZSBzZXJ2ZXIgaGFuZHMgb3V0IGEgZGVsZWdhdGlvbiBpbiBzdGVw
IDQuIG9mIHRoZSByZWNvdmVyeSBwcm9jZXNzLA0KPiB3aGljaCBjYXVzZXMgdGhlIGNsaWVudCB0
byBhdHRhY2ggYSBuZXcgZGVsZWdhdGlvbiB0byB0aGUNCj4gbmZzaS0+ZGVsZWdhdGlvbi4gWW91
IGNhbiBhcmd1ZSB0aGF0IHRoZSBjbGllbnQgc2hvdWxkbid0IGJlIGRvaW5nIHRoYXQNCj4gd2hp
bGUgaXQgaXMgaW4gdGhlIG1pZGRsZSBvZiBhIHJlY292ZXJ5IHByb2Nlc3MsIGJ1dCBhcyBmYXIg
YXMgb3VyDQo+IGNsaWVudCBpcyBjb25jZXJuZWQsIGl0IGRvZXNuJ3QgZXhwZWN0IHRoZSBzZXJ2
ZXIgdG8gdmlvbGF0ZSB0aGUNCj4gcHJvdG9jb2wgaW4gdGhpcyBtYW5uZXIuDQo+IA0KT0sgSSB1
bmRlcnN0YW5kIHRoZSBzaXR1YXRpb24uDQoNCj4gPiBSZXR1cm5pbmcgYSBkZWxlZ2F0aW9uIHRv
IE9QRU4gQ0xBSU1fREVMRUdBVEVfQ1VSIGlzIHByb2JhYmx5DQo+IHVzZWxlc3MsIGFzIHRoZSBj
bGllbnQgYWxyZWFkeSBoYXMgdGhpcyBkZWxlZ2F0aW9uLiBCdXQsIEkgZG9uJ3QgdGhpbmsNCj4g
dGhhdCBpdCBpcyBpbGxlZ2FsLg0KPiA+IElmIHRoaXMgY3JlYXRlcyB0aGUgY29uZnVzaW9uLCB3
ZSBjYW4gYXZvaWQgcmV0dXJuaW5nIHRoZSBkZWxlZ2F0aW9uDQo+IGluIHRoaXMgY2FzZS4NCj4g
DQo+IFNlZSBzZWN0aW9uIDguMS44IChhcyBJIG5vdGVkIGluIG15IGZvbGxvd3VwIGVtYWlsKS4g
VGhlIHNlcnZlciBpcyBub3QNCj4gc3VwcG9zZWQgdG8gc2VuZCBhIGRlbGVnYXRpb24gYXMgcGFy
dCBvZiBhIENMQUlNX0RFTEVHQVRFX0NVUiByZXF1ZXN0Lg0KPiBZb3UgYXJlIHZpb2xhdGluZyB0
aGUgc3BlYy4NCj4gDQpBZ3JlZWQuIEkgbWlzc2VkIHRoaXMgcG9pbnQgaW4gdGhlIFJGQy4NCg0K
PiBTZW5kaW5nIGEgZGVsZWdhdGlvbiBtb3JlIHRoYW4gb25jZSBpcyBuZWl0aGVyIG5lY2Vzc2Fy
eSwgbm9yIGlzIGl0DQo+IHJlYWxseSBhIGdvb2QgaWRlYSBldmVuIGlmIHRoZSBjbGllbnQgc2Vu
ZHMgbXVsdGlwbGUgQ0xBSU1fTlVMTCBjYWxscy4NCj4gSW4gZmFjdCBpdCBpcyBhIHJlY2lwZSBm
b3IgY3JlYXRpbmcgcmFjZXMuLi4NCj4gVGhlIHJlYXNvbnMgYXJlOg0KPiANCj4gICAgICAgKiBV
bmxpa2UgbGF5b3V0cywgdGhlcmUgaXMgbm8gJ2ZvcmdldGZ1bCcgbW9kZWwgZm9yIGRlbGVnYXRp
b25zLg0KPiAgICAgICAgIE9uY2UgdGhlIGNsaWVudCByZWNlaXZlcyBhIGRlbGVnYXRpb24sIGl0
IGlzIGV4cGVjdGVkIHRvIGtlZXANCj4gICAgICAgICB0cmFjayBvZiB0aGF0IGRlbGVnYXRpb24g
YW5kIHJldHVybiBpdCB3aGVuIGl0IG5vIGxvbmdlciBuZWVkcw0KPiAgICAgICAgIGl0Lg0KPiAg
ICAgICAqIEluIE5GU3Y0LjAsIHRoZXJlIGlzIG5vIGdvb2Qgd2F5IG9mIGRlYWxpbmcgd2l0aCBy
YWNlcyBiZXR3ZWVuDQo+ICAgICAgICAgT1BFTiBhbmQgQ0JfUkVDQUxMLiBJZiB0aGUgc2VydmVy
IGlzIGhhbmRpbmcgb3V0IGEgZGVsZWdhdGlvbiwNCj4gICAgICAgICBidXQgdGhlbiBuZWVkcyB0
byByZWNhbGwgaXQgZm9yIHNvbWUgcmVhc29uLCB0aGVuIGl0IGhhcyBubyB3YXkNCj4gICAgICAg
ICBvZiBlbnN1cmluZyB0aGF0IHRoZSBjbGllbnQgcHJvY2Vzc2VzIHRoZSBDQl9SRUNBTEwgX2Fm
dGVyXyB0aGUNCj4gICAgICAgICByZXBseSB0byB0aGUgT1BFTiBpbiB3aGljaCB0aGUgZGVsZWdh
dGlvbiB3YXMgaGFuZGVkIG91dCBmb3INCj4gdGhlDQo+ICAgICAgICAgc2Vjb25kIHRpbWUuDQo+
IA0KPiBJT1c6IHRoZXJlIGlzIGEgZGFuZ2VyIG9mIHNvbWV0aGluZyBsaWtlIHRoZSBmb2xsb3dp
bmcgcmFjZSBvY2N1cnJpbmc6DQo+IA0KPiBDbGllbnQJCQkJCVNlcnZlcg0KPiANCj4gU2VuZCBP
UEVOKENMQUlNX05VTEwpDQo+IAkJCQloYW5kcyBvdXQgZGVsZWdhdGlvbg0KPiBwcm9jZXNzIG9w
ZW4gcmVzdWx0DQo+IGNsaWVudCBub3cgdGhpbmtzIGl0DQo+IGhvbGRzIGEgZGVsZWdhdGlvbg0K
PiANCj4gCQkJCVNlcnZlciBub3cgdGhpbmtzIGNsaWVudA0KPiAJCQkJaG9sZHMgYSBkZWxlZ2F0
aW9uDQo+IA0KPiBTZW5kIE9QRU4oQ0xBSU1fTlVMTCkNCj4gCQkJCWhhbmRzIG91dCBkZWxlZ2F0
aW9uIGFnYWluDQo+IAkJCQlTZW5kIENCX1JFQ0FMTA0KPiBwcm9jZXNzIENCX1JFQ0FMTA0KPiBT
ZW5kIERFTEVHUkVUVVJODQo+IA0KPiBwcm9jZXNzIG9wZW4gcmVzdWx0DQo+IGNsaWVudCBub3cg
dGhpbmtzIGl0DQo+IGhvbGRzIGEgZGVsZWdhdGlvbg0KPiANCj4gU2VydmVyIHRoaW5rcyBjbGll
bnQgZG9lc24ndCBob2xkDQo+IGEgZGVsZWdhdGlvbi4NCj4gDQo+IA0KVGhpcyBjYXNlIGlzIGEg
bGl0dGxlIGJpdCBkaWZmZXJlbnQuIEkgYW0gZ29pbmcgdG8gbWFrZSBzdXJlIHRoYXQgZGVsZWdh
dGlvbnMgYXJlIHJldHVybmVkIG9ubHkgb25jZS4NCj4gDQo+ID4gLS0tLS1PcmlnaW5hbCBNZXNz
YWdlLS0tLS0NCj4gPiBGcm9tOiBsaW51eC1uZnMtb3duZXJAdmdlci5rZXJuZWwub3JnIFttYWls
dG86bGludXgtbmZzLQ0KPiBvd25lckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBPZiBKLiBC
cnVjZSBGaWVsZHMNCj4gPiBTZW50OiBtYXJkaSAxIG1hcnMgMjAxMSAwNToxMA0KPiA+IFRvOiBU
cm9uZCBNeWtsZWJ1c3QNCj4gPiBDYzogSmltIFJlZXM7IEJlbm55IEhhbGV2eTsgbGludXgtbmZz
QHZnZXIua2VybmVsLm9yZzsgcGV0ZXIgaG9uZXltYW4NCj4gPiBTdWJqZWN0OiBSZTogW1BBVENI
XSB6ZXJvIG91dCBkZWxlZ2F0aW9uIGluIHRoZSBpbm9kZSBhZnRlciBpdCBoYXMNCj4gYmVlbiBy
ZXR1cm5lZA0KPiA+DQo+ID4gT24gTW9uLCBGZWIgMjgsIDIwMTEgYXQgMDc6MDA6MzNQTSAtMDUw
MCwgVHJvbmQgTXlrbGVidXN0IHdyb3RlOg0KPiA+ID4gT24gTW9uLCAyMDExLTAyLTI4IGF0IDE4
OjIyIC0wNTAwLCBKaW0gUmVlcyB3cm90ZToNCj4gPiA+ID4gVHJvbmQgTXlrbGVidXN0IHdyb3Rl
Og0KPiA+ID4gPg0KPiA+ID4gPiAgIFRoZSBwcm9jZWR1cmUgZm9yIHJldHVybmluZyBkZWxlZ2F0
aW9ucyBpcyBzdXBwb3NlZCB0byB3b3JrIGFzDQo+IGZvbGxvd3M6DQo+ID4gPiA+DQo+ID4gPiA+
ICAgICAgICAxLiBSZW1vdmUgdGhlIG5mc2ktPmRlbGVnYXRpb24gc28gdGhhdCBpdCBpcyBubyBs
b25nZXINCj4gdmlzaWJsZSB0bw0KPiA+ID4gPiAgICAgICAgICAgbmV3IG9wZW4oKSByZXF1ZXN0
cw0KPiA+ID4gPiAgICAgICAgMi4gd3JpdGUgYmFjayBhbnkgZGlydHkgZGF0YSB0byB0aGUgc2Vy
dmVyDQo+ID4gPiA+ICAgICAgICAzLiBSZWNsYWltIGFueSBsb2Nrcw0KPiA+ID4gPiAgICAgICAg
NC4gUmVjbGFpbSBhbnkgb3BlbiBzdGF0ZWlkcyAodXNpbmcgQ0xBSU1fREVMRUdBVEVfQ1VSKQ0K
PiA+ID4gPiAgICAgICAgNS4gZGVsZWdyZXR1cm4NCj4gPiA+ID4NCj4gPiA+ID4gICBXaGlsZSB0
aGVyZSBtYXkgaW5kZWVkIGJlIHRoZSBvZGQgUkVBRCBvciBXUklURSB0aGF0IHJhY2VzDQo+IGJl
dHdlZW4gNC4NCj4gPiA+ID4gICBhbmQgNS4sIHNvIHRoYXQgdGhlIHNlcnZlciByZWNlaXZlcyB0
aGUgZGVsZWdhdGlvbiBzdGF0ZWlkDQo+IGFmdGVyIHRoZQ0KPiA+ID4gPiAgIGRlbGVncmV0dXJu
LCB0aGF0IHNob3VsZG4ndCBtYXR0ZXI6IHRoZSBzZXJ2ZXIgcmV0dXJucyBhbg0KPiBlcnJvciwg
YW5kIHRoZQ0KPiA+ID4gPiAgIGNsaWVudCBzaG91bGQgcmV0cnkgdXNpbmcgdGhlIG5ldyBvcGVu
IHN0YXRlaWQuDQo+ID4gPiA+DQo+ID4gPiA+ICAgV2hhdCBpcyBmYWlsaW5nIHRvIHdvcmsgY29y
cmVjdGx5IGhlcmU/DQo+ID4gPiA+DQo+ID4gPiA+IFRoYXQgaGVscHMsIHRoYW5rcy4gIEknbGwg
c2VlIGlmIEkgY2FuIGZpZ3VyZSBvdXQgd2hhdCdzIGdvaW5nDQo+IHdyb25nLg0KPiA+ID4gPg0K
PiA+ID4gPiBBdCB0aGUgc2VydmVyLCBJIHNlZSBhIGRlbGVncmV0dXJuIGZvbGxvd2VkIGJ5IGEg
c2V0YXR0ciB1c2luZw0KPiB0aGUgcmV0dXJuZWQNCj4gPiA+ID4gc3RhdGVpZC4gIFRoZSBzZXJ2
ZXIgcmV0dXJucyBCQURfU1RBVEVJRC4gIEknbGwgbG9vayB0byBzZWUgaWYNCj4gdGhlIGNsaWVu
dA0KPiA+ID4gPiB0aGVuIHJldHJpZXMuDQo+ID4gPiA+DQo+ID4gPiA+IEF0IHRoZSBjbGllbnQs
IEkgc2VlIGEgbm9uLW51bGwgbmZzaS0+ZGVsZWdhdGlvbiBhZnRlciB0aGUNCj4gZGVsZ3JldHVy
biwgYW5kDQo+ID4gPiA+IHRoZSBhcHBsaWNhdGlvbiBnZXRzIGEgRUlPLg0KPiA+ID4NCj4gPiA+
IFRoYXQncyBiZWNhdXNlIHlvdXIgc2VydmVyIGlzIGNvbmZ1c2luZyB0aGUgaGVsbCBvdXQgb2Yg
dXMgYWxsIGJ5DQo+IGdpdmluZw0KPiA+ID4gb3V0IGEgZGVsZWdhdGlvbiBhcyBwYXJ0IG9mIHRo
ZSByZXBseSAoaW4gZnJhbWUgNTMyOCkgdG8gdGhlDQo+ID4gPiBPUEVOKENMQUlNX0RFTEVHQVRF
X0NVUikgaW4gZnJhbWUgNTMyNS4NCj4gPiA+DQo+ID4gPiBJT1c6IHRoZSBjbGllbnQgaXMgaW4g
dGhlIHByb2Nlc3Mgb2YgcmV0dXJuaW5nIHRoZSBkZWxlZ2F0aW9uLCBhbmQNCj4gYXNrcw0KPiA+
ID4gdG8gdHJhZGUgaW4gdGhlIGRlbGVnYXRpb24gc3RhdGVpZCBpbnRvIGFuIG9wZW4gc3RhdGVp
ZCwgdGhlbiB0aGUNCj4gc2VydmVyDQo+ID4gPiByZXBsaWVzIHdpdGggYW4gb3BlbiBzdGF0ZWlk
LCBhbmQgdGhlIGRlbGVnYXRpb24gc3RhdGVpZC4uLg0KPiA+DQo+ID4gV2hpY2ggc2VydmVyIGlz
IHRoaXMsIEppbT8NCj4gPg0KPiA+IChJIGNoZWNrZWQgdGhlIExpbnV4IHNlcnZlciBxdWlja2x5
IGFuZCBpdCBkb2Vzbid0IGxvb2sgbGlrZSBpdA0KPiBzaG91bGQNCj4gPiBkbyB0aGlzLi4uLikN
Cj4gPg0KPiA+IC0tYi4NCj4gPiAtLQ0KPiA+IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0
OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1uZnMiDQo+IGluDQo+ID4gdGhlIGJv
ZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCj4gPiBNb3JlIG1h
am9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0
bWwNCj4gPg0KPiANCj4gLS0NCj4gVHJvbmQgTXlrbGVidXN0DQo+IExpbnV4IE5GUyBjbGllbnQg
bWFpbnRhaW5lcg0KPiANCj4gTmV0QXBwDQo+IFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQo+
IHd3dy5uZXRhcHAuY29tDQo+IA0KDQoT77+977+97Lm7HO+/vSbvv71+77+9Ju+/vRjvv73vv70r
Le+/ve+/vd22F++/ve+/vXfvv73vv73Lm++/ve+/ve+/vW3vv71i77+977+9Z37Ip++/vRfvv73v
v73cqH3vv73vv73vv73GoHrvv70majordu+/ve+/ve+/vQfvv73vv73vv73vv716Wivvv73vv70r
embvv73vv73vv71o77+977+977+9fu+/ve+/ve+/ve+/vWnvv73vv73vv71677+9Hu+/vXfvv73v
v73vv70/77+977+977+977+9Ju+/vSnfohtm
On Tue, 2011-03-01 at 04:52 -0500, [email protected] wrote:
> It is EMC server.
>
> The server returns the delegation to the OPEN CLAIM_DELEGATE_CUR operation. At this point the delegation is still valid. This is far before the DELEGRETURN; so this doesn't explain why
> the delegation is used in SETATTR (frame 5596) after the DELEGRETURN (frame 5589, reply in 5592).
It explains _exactly_ what is happening...
The server hands out a delegation in step 4. of the recovery process,
which causes the client to attach a new delegation to the
nfsi->delegation. You can argue that the client shouldn't be doing that
while it is in the middle of a recovery process, but as far as our
client is concerned, it doesn't expect the server to violate the
protocol in this manner.
> Returning a delegation to OPEN CLAIM_DELEGATE_CUR is probably useless, as the client already has this delegation. But, I don't think that it is illegal.
> If this creates the confusion, we can avoid returning the delegation in this case.
See section 8.1.8 (as I noted in my followup email). The server is not
supposed to send a delegation as part of a CLAIM_DELEGATE_CUR request.
You are violating the spec.
Sending a delegation more than once is neither necessary, nor is it
really a good idea even if the client sends multiple CLAIM_NULL calls.
In fact it is a recipe for creating races...
The reasons are:
* Unlike layouts, there is no 'forgetful' model for delegations.
Once the client receives a delegation, it is expected to keep
track of that delegation and return it when it no longer needs
it.
* In NFSv4.0, there is no good way of dealing with races between
OPEN and CB_RECALL. If the server is handing out a delegation,
but then needs to recall it for some reason, then it has no way
of ensuring that the client processes the CB_RECALL _after_ the
reply to the OPEN in which the delegation was handed out for the
second time.
IOW: there is a danger of something like the following race occurring:
Client Server
Send OPEN(CLAIM_NULL)
hands out delegation
process open result
client now thinks it
holds a delegation
Server now thinks client
holds a delegation
Send OPEN(CLAIM_NULL)
hands out delegation again
Send CB_RECALL
process CB_RECALL
Send DELEGRETURN
process open result
client now thinks it
holds a delegation
Server thinks client doesn't hold
a delegation.
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of J. Bruce Fields
> Sent: mardi 1 mars 2011 05:10
> To: Trond Myklebust
> Cc: Jim Rees; Benny Halevy; [email protected]; peter honeyman
> Subject: Re: [PATCH] zero out delegation in the inode after it has been returned
>
> On Mon, Feb 28, 2011 at 07:00:33PM -0500, Trond Myklebust wrote:
> > On Mon, 2011-02-28 at 18:22 -0500, Jim Rees wrote:
> > > Trond Myklebust wrote:
> > >
> > > The procedure for returning delegations is supposed to work as follows:
> > >
> > > 1. Remove the nfsi->delegation so that it is no longer visible to
> > > new open() requests
> > > 2. write back any dirty data to the server
> > > 3. Reclaim any locks
> > > 4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
> > > 5. delegreturn
> > >
> > > While there may indeed be the odd READ or WRITE that races between 4.
> > > and 5., so that the server receives the delegation stateid after the
> > > delegreturn, that shouldn't matter: the server returns an error, and the
> > > client should retry using the new open stateid.
> > >
> > > What is failing to work correctly here?
> > >
> > > That helps, thanks. I'll see if I can figure out what's going wrong.
> > >
> > > At the server, I see a delegreturn followed by a setattr using the returned
> > > stateid. The server returns BAD_STATEID. I'll look to see if the client
> > > then retries.
> > >
> > > At the client, I see a non-null nfsi->delegation after the delgreturn, and
> > > the application gets a EIO.
> >
> > That's because your server is confusing the hell out of us all by giving
> > out a delegation as part of the reply (in frame 5328) to the
> > OPEN(CLAIM_DELEGATE_CUR) in frame 5325.
> >
> > IOW: the client is in the process of returning the delegation, and asks
> > to trade in the delegation stateid into an open stateid, then the server
> > replies with an open stateid, and the delegation stateid...
>
> Which server is this, Jim?
>
> (I checked the Linux server quickly and it doesn't look like it should
> do this....)
>
> --b.
> --
> 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
>
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
It is EMC server.
The server returns the delegation to the OPEN CLAIM_DELEGATE_CUR operation. At this point the delegation is still valid. This is far before the DELEGRETURN; so this doesn't explain why
the delegation is used in SETATTR (frame 5596) after the DELEGRETURN (frame 5589, reply in 5592).
Returning a delegation to OPEN CLAIM_DELEGATE_CUR is probably useless, as the client already has this delegation. But, I don't think that it is illegal.
If this creates the confusion, we can avoid returning the delegation in this case.
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of J. Bruce Fields
Sent: mardi 1 mars 2011 05:10
To: Trond Myklebust
Cc: Jim Rees; Benny Halevy; [email protected]; peter honeyman
Subject: Re: [PATCH] zero out delegation in the inode after it has been returned
On Mon, Feb 28, 2011 at 07:00:33PM -0500, Trond Myklebust wrote:
> On Mon, 2011-02-28 at 18:22 -0500, Jim Rees wrote:
> > Trond Myklebust wrote:
> >
> > The procedure for returning delegations is supposed to work as follows:
> >
> > 1. Remove the nfsi->delegation so that it is no longer visible to
> > new open() requests
> > 2. write back any dirty data to the server
> > 3. Reclaim any locks
> > 4. Reclaim any open stateids (using CLAIM_DELEGATE_CUR)
> > 5. delegreturn
> >
> > While there may indeed be the odd READ or WRITE that races between 4.
> > and 5., so that the server receives the delegation stateid after the
> > delegreturn, that shouldn't matter: the server returns an error, and the
> > client should retry using the new open stateid.
> >
> > What is failing to work correctly here?
> >
> > That helps, thanks. I'll see if I can figure out what's going wrong.
> >
> > At the server, I see a delegreturn followed by a setattr using the returned
> > stateid. The server returns BAD_STATEID. I'll look to see if the client
> > then retries.
> >
> > At the client, I see a non-null nfsi->delegation after the delgreturn, and
> > the application gets a EIO.
>
> That's because your server is confusing the hell out of us all by giving
> out a delegation as part of the reply (in frame 5328) to the
> OPEN(CLAIM_DELEGATE_CUR) in frame 5325.
>
> IOW: the client is in the process of returning the delegation, and asks
> to trade in the delegation stateid into an open stateid, then the server
> replies with an open stateid, and the delegation stateid...
Which server is this, Jim?
(I checked the Linux server quickly and it doesn't look like it should
do this....)
--b.