nfs4: serialize layoutcommit

PATCH Superseded Peng Tao 3 participants 6 messages Sep 15, 2011 – Sep 16, 2011

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Peng Tao

author

2011-09-15 17:38 UTC #0 lore raw

PATCH 1/1 nfs4: serialize layoutcommit

Current pnfs_layoutcommit_inode can not handle parallel layoutcommit. As Trond suggested, there is no need for client to optimize for parallel layoutcommit. The patch add NFS_INO_LAYOUTCOMMITTING flag to mark inflight layoutcommit and serialize lalyoutcommit with it. It also fixes the pls_lc_list corruption that Vitaliy found.

Reported-by Vitaliy Gusev <[email protected]>

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/nfs4proc.c      |    6 ++++++
 fs/nfs/pnfs.c          |    9 +++++++++
 include/linux/nfs_fs.h |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)

fs/nfs/nfs4proc.c +6 -0

index 4700fae..a7ce210 100644
@@ -5970,6 +5970,7 @@ static void nfs4_layoutcommit_release(void *calldata)
 {
 	struct nfs4_layoutcommit_data *data = calldata;
 	struct pnfs_layout_segment *lseg, *tmp;
+	unsigned long *bitlock = &NFS_I(data->args.inode)->flags;
 
 	pnfs_cleanup_layoutcommit(data);
 	/* Matched by references in pnfs_set_layoutcommit */

@@ -5979,6 +5980,11 @@ static void nfs4_layoutcommit_release(void *calldata)
 				       &lseg->pls_flags))
 			put_lseg(lseg);
 	}
+
+	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
+	smp_mb__after_clear_bit();
+	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+
 	put_rpccred(data->cred);
 	kfree(data);
 }

fs/nfs/pnfs.c +9 -0

index b483bbc..fb71def 100644
@@ -1451,10 +1451,19 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 		goto out;
 	}
 
+	if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags) ||
+	    (status = wait_on_bit_lock(&nfsi->flags, NFS_INO_LAYOUTCOMMITTING,
+				nfs_wait_bit_killable, TASK_KILLABLE))) {
+		kfree(data);
+		goto out;
+	}
+
 	INIT_LIST_HEAD(&data->lseg_list);
 	spin_lock(&inode->i_lock);
 	if (!test_and_clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags)) {
+		clear_bit(NFS_INO_LAYOUTCOMMITTING, &nfsi->flags);
 		spin_unlock(&inode->i_lock);
+		wake_up_bit(&nfsi->flags, NFS_INO_LAYOUTCOMMITTING);
 		kfree(data);
 		goto out;
 	}

include/linux/nfs_fs.h +1 -0

index eaac770..c5b2b30 100644
@@ -230,6 +230,7 @@ struct nfs_inode {
 #define NFS_INO_COMMIT		(7)		/* inode is committing unstable writes */
 #define NFS_INO_PNFS_COMMIT	(8)		/* use pnfs code for commit */
 #define NFS_INO_LAYOUTCOMMIT	(9)		/* layoutcommit required */
+#define NFS_INO_LAYOUTCOMMITTING (10)		/* layoutcommit inflight */
 
 static inline struct nfs_inode *NFS_I(const struct inode *inode)
 {

Signature · 1 line

1.7.1.262.g5ef3d

Trond Myklebust

2011-09-15 17:39 UTC #1 lore raw

Quoted 43 lines · Peng Tao

Current pnfs_layoutcommit_inode can not handle parallel layoutcommit. As Trond suggested, there is no need for client to optimize for parallel layoutcommit. The patch add NFS_INO_LAYOUTCOMMITTING flag to mark inflight layoutcommit and serialize lalyoutcommit with it. It also fixes the pls_lc_list corruption that Vitaliy found.
Reported-by Vitaliy Gusev <[email protected]>
Signed-off-by Peng Tao <[email protected]>
---
 fs/nfs/nfs4proc.c      |    6 ++++++
 fs/nfs/pnfs.c          |    9 +++++++++
 include/linux/nfs_fs.h |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)
fs/nfs/nfs4proc.c +6 -0
index 4700fae..a7ce210 100644
@@ -5970,6 +5970,7 @@ static void nfs4_layoutcommit_release(void *calldata)
 {
 	struct nfs4_layoutcommit_data *data = calldata;
 	struct pnfs_layout_segment *lseg, *tmp;
+	unsigned long *bitlock = &NFS_I(data->args.inode)->flags;
 
 	pnfs_cleanup_layoutcommit(data);
 	/* Matched by references in pnfs_set_layoutcommit */
@@ -5979,6 +5980,11 @@ static void nfs4_layoutcommit_release(void *calldata)
 				       &lseg->pls_flags))
 			put_lseg(lseg);
 	}
+
+	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
+	smp_mb__after_clear_bit();
+	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+
 	put_rpccred(data->cred);
 	kfree(data);
 }
fs/nfs/pnfs.c +0 -0
index b483bbc..fb71def 100644

fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync) · line 1456

Quoted 6 lines

fs/nfs/pnfs.c +3 -0

@@ -1451,10 +1451,19 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 		goto out;
 	}
 
+	if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags) ||
+	    (status = wait_on_bit_lock(&nfsi->flags, NFS_INO_LAYOUTCOMMITTING,
+				nfs_wait_bit_killable, TASK_KILLABLE))) {

You want the sleeping behaviour above to be subject to the 'sync' flag (in the same way we do for regular commit). If !sync, then try to grab the bit lock anyway, and exit on failure.

Quoted 25 lines

fs/nfs/pnfs.c +6 -0

+		kfree(data);
+		goto out;
+	}
+
 	INIT_LIST_HEAD(&data->lseg_list);
 	spin_lock(&inode->i_lock);
 	if (!test_and_clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags)) {
+		clear_bit(NFS_INO_LAYOUTCOMMITTING, &nfsi->flags);
 		spin_unlock(&inode->i_lock);
+		wake_up_bit(&nfsi->flags, NFS_INO_LAYOUTCOMMITTING);
 		kfree(data);
 		goto out;
 	}

include/linux/nfs_fs.h +1 -0

index eaac770..c5b2b30 100644
@@ -230,6 +230,7 @@ struct nfs_inode {
 #define NFS_INO_COMMIT		(7)		/* inode is committing unstable writes */
 #define NFS_INO_PNFS_COMMIT	(8)		/* use pnfs code for commit */
 #define NFS_INO_LAYOUTCOMMIT	(9)		/* layoutcommit required */
+#define NFS_INO_LAYOUTCOMMITTING (10)		/* layoutcommit inflight */
 
 static inline struct nfs_inode *NFS_I(const struct inode *inode)
 {

Signature · 6 lines

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

Trond Myklebust

2011-09-15 17:48 UTC #2 lore raw

Quoted 47 lines · Trond Myklebust

Quoted 46 lines · Peng Tao

Reported-by Vitaliy Gusev <[email protected]>

Signed-off-by Peng Tao <[email protected]>

---
 fs/nfs/nfs4proc.c      |    6 ++++++
 fs/nfs/pnfs.c          |    9 +++++++++
 include/linux/nfs_fs.h |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)

fs/nfs/nfs4proc.c +6 -0

index 4700fae..a7ce210 100644
@@ -5970,6 +5970,7 @@ static void nfs4_layoutcommit_release(void *calldata)
 {
 	struct nfs4_layoutcommit_data *data = calldata;
 	struct pnfs_layout_segment *lseg, *tmp;
+	unsigned long *bitlock = &NFS_I(data->args.inode)->flags;
 
 	pnfs_cleanup_layoutcommit(data);
 	/* Matched by references in pnfs_set_layoutcommit */

@@ -5979,6 +5980,11 @@ static void nfs4_layoutcommit_release(void *calldata)
 				       &lseg->pls_flags))
 			put_lseg(lseg);
 	}
+
+	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
+	smp_mb__after_clear_bit();
+	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+
 	put_rpccred(data->cred);
 	kfree(data);
 }

fs/nfs/pnfs.c +0 -0

index b483bbc..fb71def 100644
@@ -1451,10 +1451,19 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 		goto out;
 	}

Quoted 7 lines

fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync) · line 1456
Quoted 3 lines
fs/nfs/pnfs.c +3 -0
+	if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags) ||
+	    (status = wait_on_bit_lock(&nfsi->flags, NFS_INO_LAYOUTCOMMITTING,
+				nfs_wait_bit_killable, TASK_KILLABLE))) {
You want the sleeping behaviour above to be subject to the 'sync' flag (in the same way we do for regular commit). If !sync, then try to grab the bit lock anyway, and exit on failure.

By the way. Shouldn't pnfs_layoutcommit_inode() also be marking the inode as dirty on failure?

Signature · 6 lines

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

[email protected]

2011-09-16 11:59 UTC #3 lore raw

SGksIFRyb25kLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFRyb25k IE15a2xlYnVzdCBbbWFpbHRvOlRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tXQ0KPiBTZW50OiBG cmlkYXksIFNlcHRlbWJlciAxNiwgMjAxMSAxOjQ4IEFNDQo+IFRvOiBQZW5nIFRhbw0KPiBDYzog YmhhbGV2eUB0b25pYW4uY29tOyBWaXRhbGl5IEd1c2V2OyBsaW51eC1uZnNAdmdlci5rZXJuZWwu b3JnOyBQZW5nLCBUYW8NCj4gU3ViamVjdDogUmU6IFtQQVRDSF0gbmZzNDogc2VyaWFsaXplIGxh eW91dGNvbW1pdA0KPiANCj4gT24gVGh1LCAyMDExLTA5LTE1IGF0IDEzOjM4IC0wNDAwLCBUcm9u ZCBNeWtsZWJ1c3Qgd3JvdGU6DQo+ID4gT24gU3VuLCAyMDExLTA5LTExIGF0IDIxOjQ4IC0wNzAw LCBQZW5nIFRhbyB3cm90ZToNCj4gPiA+IEN1cnJlbnQgcG5mc19sYXlvdXRjb21taXRfaW5vZGUg Y2FuIG5vdCBoYW5kbGUgcGFyYWxsZWwgbGF5b3V0Y29tbWl0Lg0KPiA+ID4gQXMgVHJvbmQgc3Vn Z2VzdGVkLCB0aGVyZSBpcyBubyBuZWVkIGZvciBjbGllbnQgdG8gb3B0aW1pemUgZm9yDQo+ID4g PiBwYXJhbGxlbCBsYXlvdXRjb21taXQuIFRoZSBwYXRjaCBhZGQgTkZTX0lOT19MQVlPVVRDT01N SVRUSU5HIGZsYWcgdG8NCj4gPiA+IG1hcmsgaW5mbGlnaHQgbGF5b3V0Y29tbWl0IGFuZCBzZXJp YWxpemUgbGFseW91dGNvbW1pdCB3aXRoIGl0Lg0KPiA+ID4gSXQgYWxzbyBmaXhlcyB0aGUgcGxz X2xjX2xpc3QgY29ycnVwdGlvbiB0aGF0IFZpdGFsaXkgZm91bmQuDQo+ID4gPg0KPiA+ID4gUmVw b3J0ZWQtYnk6IFZpdGFsaXkgR3VzZXYgPGd1c2V2LnZpdGFsaXlAbmV4ZW50YS5jb20+DQo+ID4g PiBTaWduZWQtb2ZmLWJ5OiBQZW5nIFRhbyA8cGVuZ190YW9AZW1jLmNvbT4NCj4gPiA+IC0tLQ0K PiA+ID4gIGZzL25mcy9uZnM0cHJvYy5jICAgICAgfCAgICA2ICsrKysrKw0KPiA+ID4gIGZzL25m cy9wbmZzLmMgICAgICAgICAgfCAgICA5ICsrKysrKysrKw0KPiA+ID4gIGluY2x1ZGUvbGludXgv bmZzX2ZzLmggfCAgICAxICsNCj4gPiA+ICAzIGZpbGVzIGNoYW5nZWQsIDE2IGluc2VydGlvbnMo KyksIDAgZGVsZXRpb25zKC0pDQo+ID4gPg0KPiA+ID4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0 cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiA+IGluZGV4IDQ3MDBmYWUuLmE3Y2UyMTAg MTAwNjQ0DQo+ID4gPiAtLS0gYS9mcy9uZnMvbmZzNHByb2MuYw0KPiA+ID4gKysrIGIvZnMvbmZz L25mczRwcm9jLmMNCj4gPiA+IEBAIC01OTcwLDYgKzU5NzAsNyBAQCBzdGF0aWMgdm9pZCBuZnM0 X2xheW91dGNvbW1pdF9yZWxlYXNlKHZvaWQgKmNhbGxkYXRhKQ0KPiA+ID4gIHsNCj4gPiA+ICAJ c3RydWN0IG5mczRfbGF5b3V0Y29tbWl0X2RhdGEgKmRhdGEgPSBjYWxsZGF0YTsNCj4gPiA+ICAJ c3RydWN0IHBuZnNfbGF5b3V0X3NlZ21lbnQgKmxzZWcsICp0bXA7DQo+ID4gPiArCXVuc2lnbmVk IGxvbmcgKmJpdGxvY2sgPSAmTkZTX0koZGF0YS0+YXJncy5pbm9kZSktPmZsYWdzOw0KPiA+ID4N Cj4gPiA+ICAJcG5mc19jbGVhbnVwX2xheW91dGNvbW1pdChkYXRhKTsNCj4gPiA+ICAJLyogTWF0 Y2hlZCBieSByZWZlcmVuY2VzIGluIHBuZnNfc2V0X2xheW91dGNvbW1pdCAqLw0KPiA+ID4gQEAg LTU5NzksNiArNTk4MCwxMSBAQCBzdGF0aWMgdm9pZCBuZnM0X2xheW91dGNvbW1pdF9yZWxlYXNl KHZvaWQNCj4gKmNhbGxkYXRhKQ0KPiA+ID4gIAkJCQkgICAgICAgJmxzZWctPnBsc19mbGFncykp DQo+ID4gPiAgCQkJcHV0X2xzZWcobHNlZyk7DQo+ID4gPiAgCX0NCj4gPiA+ICsNCj4gPiA+ICsJ Y2xlYXJfYml0X3VubG9jayhORlNfSU5PX0xBWU9VVENPTU1JVFRJTkcsIGJpdGxvY2spOw0KPiA+ ID4gKwlzbXBfbWJfX2FmdGVyX2NsZWFyX2JpdCgpOw0KPiA+ID4gKwl3YWtlX3VwX2JpdChiaXRs b2NrLCBORlNfSU5PX0xBWU9VVENPTU1JVFRJTkcpOw0KPiA+ID4gKw0KPiA+ID4gIAlwdXRfcnBj Y3JlZChkYXRhLT5jcmVkKTsNCj4gPiA+ICAJa2ZyZWUoZGF0YSk7DQo+ID4gPiAgfQ0KPiA+ID4g ZGlmZiAtLWdpdCBhL2ZzL25mcy9wbmZzLmMgYi9mcy9uZnMvcG5mcy5jDQo+ID4gPiBpbmRleCBi NDgzYmJjLi5mYjcxZGVmIDEwMDY0NA0KPiA+ID4gLS0tIGEvZnMvbmZzL3BuZnMuYw0KPiA+ID4g KysrIGIvZnMvbmZzL3BuZnMuYw0KPiA+ID4gQEAgLTE0NTEsMTAgKzE0NTEsMTkgQEAgcG5mc19s YXlvdXRjb21taXRfaW5vZGUoc3RydWN0IGlub2RlICppbm9kZSwNCj4gYm9vbCBzeW5jKQ0KPiA+ ID4gIAkJZ290byBvdXQ7DQo+ID4gPiAgCX0NCj4gPiA+DQo+ID4gPiArCWlmICghdGVzdF9iaXQo TkZTX0lOT19MQVlPVVRDT01NSVQsICZuZnNpLT5mbGFncykgfHwNCj4gPiA+ICsJICAgIChzdGF0 dXMgPSB3YWl0X29uX2JpdF9sb2NrKCZuZnNpLT5mbGFncywNCj4gTkZTX0lOT19MQVlPVVRDT01N SVRUSU5HLA0KPiA+ID4gKwkJCQluZnNfd2FpdF9iaXRfa2lsbGFibGUsIFRBU0tfS0lMTEFCTEUp KSkgew0KPiA+DQo+ID4gWW91IHdhbnQgdGhlIHNsZWVwaW5nIGJlaGF2aW91ciBhYm92ZSB0byBi ZSBzdWJqZWN0IHRvIHRoZSAnc3luYycgZmxhZw0KPiA+IChpbiB0aGUgc2FtZSB3YXkgd2UgZG8g Zm9yIHJlZ3VsYXIgY29tbWl0KS4gSWYgIXN5bmMsIHRoZW4gdHJ5IHRvIGdyYWINCj4gPiB0aGUg Yml0IGxvY2sgYW55d2F5LCBhbmQgZXhpdCBvbiBmYWlsdXJlLg0KSSdtIGEgbGl0dGxlIGNvbmZ1 c2VkIGFib3V0IHRoZSAiIXN5bmMiIGhlcmUuIElmIHdlIHJldHVybiBmYWlsdXJlLCB3aGF0IGRv IHdlIGV4cGVjdCBjYWxsZXJzIHRvIGRvPw0KTG9va2luZyBhdCB0aGUgY2FsbGluZyBzdGFjaywg SSBkb24ndCBzZWUgYW55IHVzZXJzIHJldHJ5aW5nIG9uIC1FQUdBSU4uIERvZXMgdGhlICIhc3lu YyIgbmVjZXNzYXJpbHkgbWVhbiBub24tYmxvY2tpbmc/DQoNCj4gDQo+IEJ5IHRoZSB3YXkuIFNo b3VsZG4ndCBwbmZzX2xheW91dGNvbW1pdF9pbm9kZSgpIGFsc28gYmUgbWFya2luZyB0aGUNCj4g aW5vZGUgYXMgZGlydHkgb24gZmFpbHVyZT8NClllYWgsIHJpZ2h0LCB3ZSBzaG91bGQgbWFyayBp bm9kZSBhcyBkaXJ0eSBvbiBmYWlsdXJlLg0KDQpUaGFua3MsDQpUYW8NCgTvv717Lm7vv70r77+9 77+977+977+977+977+977+9KyXvv73vv71sendt77+977+9Yu+/veunsu+/ve+/vXLvv73vv716 WO+/ve+/vRnfsinvv73vv73vv713Kh9qZ++/ve+/ve+/vR7vv73vv73vv73vv73vv73domov77+9 77+977+9eu+/vd6W77+977+9Mu+/vd6Z77+977+977+9Ju+/vSnfoe+/vWHvv73vv71/77+977+9 Hu+/vUfvv73vv73vv71o77+9D++/vWo6K3bvv73vv73vv71377+92aU=

Trond Myklebust

2011-09-16 15:04 UTC #4 lore raw

Quoted 67 lines · On Fri, 2011-09-16 at 07:58 -0400, [email protected] wrote:

Hi, Trond,
Quoted 54 lines
-----Original Message-----
From: Trond Myklebust [mailto:[email protected]]
Sent: Friday, September 16, 2011 1:48 AM
To: Peng Tao
Cc: [email protected]; Vitaliy Gusev; [email protected]; Peng, Tao
Subject: Re: [PATCH] nfs4: serialize layoutcommit
Quoted 25 lines · Trond Myklebust
Quoted 24 lines · Peng Tao
Current pnfs_layoutcommit_inode can not handle parallel layoutcommit. As Trond suggested, there is no need for client to optimize for parallel layoutcommit. The patch add NFS_INO_LAYOUTCOMMITTING flag to mark inflight layoutcommit and serialize lalyoutcommit with it. It also fixes the pls_lc_list corruption that Vitaliy found.
Reported-by Vitaliy Gusev <[email protected]>
Signed-off-by Peng Tao <[email protected]>
---
 fs/nfs/nfs4proc.c      |    6 ++++++
 fs/nfs/pnfs.c          |    9 +++++++++
 include/linux/nfs_fs.h |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)
fs/nfs/nfs4proc.c +1 -0
index 4700fae..a7ce210 100644
@@ -5970,6 +5970,7 @@ static void nfs4_layoutcommit_release(void *calldata)
 {
 	struct nfs4_layoutcommit_data *data = calldata;
 	struct pnfs_layout_segment *lseg, *tmp;
+	unsigned long *bitlock = &NFS_I(data->args.inode)->flags;
 
Quoted 3 lines
fs/nfs/nfs4proc.c static void nfs4_layoutcommit_release(void
Quoted 3 lines
fs/nfs/nfs4proc.c +0 -0
 	pnfs_cleanup_layoutcommit(data);
 	/* Matched by references in pnfs_set_layoutcommit */
@@ -5979,6 +5980,11 @@ static void nfs4_layoutcommit_release(void
*calldata)
Quoted 8 lines
Quoted 8 lines
fs/nfs/nfs4proc.c +5 -0
 				       &lseg->pls_flags))
 			put_lseg(lseg);
 	}
+
+	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
+	smp_mb__after_clear_bit();
+	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+
Quoted 8 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode,
Quoted 8 lines
fs/nfs/nfs4proc.c +0 -0
 	put_rpccred(data->cred);
 	kfree(data);
 }
fs/nfs/pnfs.c +0 -0
index b483bbc..fb71def 100644
@@ -1451,10 +1451,19 @@ pnfs_layoutcommit_inode(struct inode *inode,
bool sync)
Quoted 11 lines
Quoted 5 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync)
Quoted 5 lines
fs/nfs/pnfs.c +2 -0
 		goto out;
 	}
 
+	if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags) ||
+	    (status = wait_on_bit_lock(&nfsi->flags,
NFS_INO_LAYOUTCOMMITTING,
Quoted 5 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync) · line 1456
Quoted 1 line
fs/nfs/pnfs.c +1 -0
+				nfs_wait_bit_killable, TASK_KILLABLE))) {
You want the sleeping behaviour above to be subject to the 'sync' flag (in the same way we do for regular commit). If !sync, then try to grab the bit lock anyway, and exit on failure.

Quoted 7 lines

I'm a little confused about the "!sync" here. If we return failure, what do we expect callers to do? Looking at the calling stack, I don't see any users retrying on -EAGAIN. Does the "!sync" necessarily mean non-blocking?
Quoted 3 lines
By the way. Shouldn't pnfs_layoutcommit_inode() also be marking the inode as dirty on failure?
Yeah, right, we should mark inode as dirty on failure.

The !sync means that we're being called from a context such as kswapd where we really don't want to sleep or block the thread.

In the case where this happens, and the NFS_INO_LAYOUTCOMMITTING bit can't be set, we should exit and mark the inode as dirty so that we can retry at a later time. Please see the 'commit' code, which has the same properties...

Cheers
Trond

Signature · 6 lines

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

Peng Tao

author

2011-09-16 15:28 UTC #5 lore raw

Quoted 76 lines · Trond Myklebust

Quoted 67 lines · On Fri, 2011-09-16 at 07:58 -0400, [email protected] wrote:
Hi, Trond,
Quoted 54 lines
-----Original Message-----
From: Trond Myklebust [mailto:[email protected]]
Sent: Friday, September 16, 2011 1:48 AM
To: Peng Tao
Cc: [email protected]; Vitaliy Gusev; [email protected]; Peng, Tao
Subject: Re: [PATCH] nfs4: serialize layoutcommit
Quoted 25 lines · Trond Myklebust
Quoted 24 lines · Peng Tao
Current pnfs_layoutcommit_inode can not handle parallel layoutcommit. As Trond suggested, there is no need for client to optimize for parallel layoutcommit. The patch add NFS_INO_LAYOUTCOMMITTING flag to mark inflight layoutcommit and serialize lalyoutcommit with it. It also fixes the pls_lc_list corruption that Vitaliy found.
Reported-by Vitaliy Gusev <[email protected]>
Signed-off-by Peng Tao <[email protected]>
---
fs/nfs/nfs4proc.c | 6 ++++++
fs/nfs/pnfs.c | 9 +++++++++
include/linux/nfs_fs.h | 1 +
 3 files changed, 16 insertions(+), 0 deletions(-)
fs/nfs/nfs4proc.c +1 -0
index 4700fae..a7ce210 100644
@@ -5970,6 +5970,7 @@ static void nfs4_layoutcommit_release(void *calldata)
  {
         struct nfs4_layoutcommit_data *data = calldata;
         struct pnfs_layout_segment *lseg, *tmp;
+       unsigned long *bitlock = &NFS_I(data->args.inode)->flags;
 
Quoted 3 lines
fs/nfs/nfs4proc.c static void nfs4_layoutcommit_release(void
Quoted 3 lines
fs/nfs/nfs4proc.c +0 -0
         pnfs_cleanup_layoutcommit(data);
         /* Matched by references in pnfs_set_layoutcommit */
@@ -5979,6 +5980,11 @@ static void nfs4_layoutcommit_release(void
*calldata)
Quoted 8 lines
Quoted 8 lines
fs/nfs/nfs4proc.c +5 -0
                                        &lseg->pls_flags))
                         put_lseg(lseg);
         }
+
+       clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
+       smp_mb__after_clear_bit();
+       wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+
Quoted 8 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode,
Quoted 8 lines
fs/nfs/nfs4proc.c +0 -0
         put_rpccred(data->cred);
         kfree(data);
  }
fs/nfs/pnfs.c +0 -0
index b483bbc..fb71def 100644
@@ -1451,10 +1451,19 @@ pnfs_layoutcommit_inode(struct inode *inode,
bool sync)
Quoted 11 lines
Quoted 5 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync)
Quoted 5 lines
fs/nfs/pnfs.c +2 -0
                 goto out;
         }
 
+       if (!test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags) ||
+           (status = wait_on_bit_lock(&nfsi->flags,
NFS_INO_LAYOUTCOMMITTING,
Quoted 5 lines
fs/nfs/pnfs.c pnfs_layoutcommit_inode(struct inode *inode, bool sync) · line 1456
Quoted 1 line
fs/nfs/pnfs.c +1 -0
+                               nfs_wait_bit_killable, TASK_KILLABLE))) {
You want the sleeping behaviour above to be subject to the 'sync' flag (in the same way we do for regular commit). If !sync, then try to grab the bit lock anyway, and exit on failure.
Quoted 7 lines
I'm a little confused about the "!sync" here. If we return failure, what do we expect callers to do? Looking at the calling stack, I don't see any users retrying on -EAGAIN. Does the "!sync" necessarily mean non-blocking?
Quoted 3 lines
By the way. Shouldn't pnfs_layoutcommit_inode() also be marking the inode as dirty on failure?
Yeah, right, we should mark inode as dirty on failure.

Quoted 7 lines

The !sync means that we're being called from a context such as kswapd where we really don't want to sleep or block the thread.
In the case where this happens, and the NFS_INO_LAYOUTCOMMITTING bit can't be set, we should exit and mark the inode as dirty so that we can retry at a later time. Please see the 'commit' code, which has the same properties...

I see. Thanks for the explanation. I will update the patch.

Best,
Tao