2017-11-20 21:43:12

by Scott Mayhew

[permalink] [raw]
Subject: [PATCH] nfs: make pnfs_destroy_layout() wait on commit when inode is being evicted

Otherwise nfs_commit_inode() can set I_DIRTY_DATASYNC in the inode's
i_state, allowing the following BUG_ON in evict() to be triggered:

BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));

Fixes: 1f18b82c3437 ("pNFS: Ensure we commit the layout if it has been...")
Signed-off-by: Scott Mayhew <[email protected]>
Tested-by: Tigran Mkrtchyan <[email protected]>
---
fs/nfs/filelayout/filelayout.c | 2 +-
fs/nfs/flexfilelayout/flexfilelayout.c | 2 +-
fs/nfs/nfs4super.c | 2 +-
fs/nfs/pnfs.c | 6 +++---
fs/nfs/pnfs.h | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 4e54d8b..28b7228 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -169,7 +169,7 @@ static int filelayout_async_handle_error(struct rpc_task *task,
* i/o and all i/o waiting on the slot table to the MDS until
* layout is destroyed and a new valid layout is obtained.
*/
- pnfs_destroy_layout(NFS_I(inode));
+ pnfs_destroy_layout(NFS_I(inode), 0);
rpc_wake_up(&tbl->slot_tbl_waitq);
goto reset;
/* RPC connection errors */
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index c75ad98..ab0a1e5 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -1088,7 +1088,7 @@ static int ff_layout_async_handle_error_v4(struct rpc_task *task,
* i/o and all i/o waiting on the slot table to the MDS until
* layout is destroyed and a new valid layout is obtained.
*/
- pnfs_destroy_layout(NFS_I(inode));
+ pnfs_destroy_layout(NFS_I(inode), 0);
rpc_wake_up(&tbl->slot_tbl_waitq);
goto reset;
/* RPC connection errors */
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 6fb7cb6..3b4a063 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -95,7 +95,7 @@ static void nfs4_evict_inode(struct inode *inode)
nfs_inode_return_delegation_noreclaim(inode);
/* Note that above delegreturn would trigger pnfs return-on-close */
pnfs_return_layout(inode);
- pnfs_destroy_layout(NFS_I(inode));
+ pnfs_destroy_layout(NFS_I(inode), FLUSH_SYNC);
/* First call standard NFS clear_inode() code */
nfs_clear_inode(inode);
}
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d602fe9..09e66a8 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -696,7 +696,7 @@ pnfs_free_lseg_list(struct list_head *free_me)
}

void
-pnfs_destroy_layout(struct nfs_inode *nfsi)
+pnfs_destroy_layout(struct nfs_inode *nfsi, int how)
{
struct pnfs_layout_hdr *lo;
LIST_HEAD(tmp_list);
@@ -710,7 +710,7 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
pnfs_layout_clear_fail_bit(lo, NFS_LAYOUT_RW_FAILED);
spin_unlock(&nfsi->vfs_inode.i_lock);
pnfs_free_lseg_list(&tmp_list);
- nfs_commit_inode(&nfsi->vfs_inode, 0);
+ nfs_commit_inode(&nfsi->vfs_inode, how);
pnfs_put_layout_hdr(lo);
} else
spin_unlock(&nfsi->vfs_inode.i_lock);
@@ -1849,7 +1849,7 @@ pnfs_update_layout(struct inode *ino,
}
/* Destroy the existing layout and start over */
if (time_after(jiffies, giveup))
- pnfs_destroy_layout(NFS_I(ino));
+ pnfs_destroy_layout(NFS_I(ino), 0);
/* Fallthrough */
case -EAGAIN:
break;
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 8d507c3..3fe2160 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -245,7 +245,7 @@ size_t pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio,
void pnfs_set_lo_fail(struct pnfs_layout_segment *lseg);
struct pnfs_layout_segment *pnfs_layout_process(struct nfs4_layoutget *lgp);
void pnfs_free_lseg_list(struct list_head *tmp_list);
-void pnfs_destroy_layout(struct nfs_inode *);
+void pnfs_destroy_layout(struct nfs_inode *, int how);
void pnfs_destroy_all_layouts(struct nfs_client *);
int pnfs_destroy_layouts_byfsid(struct nfs_client *clp,
struct nfs_fsid *fsid,
--
2.9.5



2017-11-29 19:40:31

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH] nfs: make pnfs_destroy_layout() wait on commit when inode is being evicted

Hi Scott,

On 11/20/2017 04:43 PM, Scott Mayhew wrote:
> Otherwise nfs_commit_inode() can set I_DIRTY_DATASYNC in the inode's
> i_state, allowing the following BUG_ON in evict() to be triggered:
>
> BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
>
> Fixes: 1f18b82c3437 ("pNFS: Ensure we commit the layout if it has been...")
> Signed-off-by: Scott Mayhew <[email protected]>
> Tested-by: Tigran Mkrtchyan <[email protected]>
> ---
> fs/nfs/filelayout/filelayout.c | 2 +-
> fs/nfs/flexfilelayout/flexfilelayout.c | 2 +-
> fs/nfs/nfs4super.c | 2 +-
> fs/nfs/pnfs.c | 6 +++---
> fs/nfs/pnfs.h | 2 +-
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
> index 4e54d8b..28b7228 100644
> --- a/fs/nfs/filelayout/filelayout.c
> +++ b/fs/nfs/filelayout/filelayout.c
> @@ -169,7 +169,7 @@ static int filelayout_async_handle_error(struct rpc_task *task,
> * i/o and all i/o waiting on the slot table to the MDS until
> * layout is destroyed and a new valid layout is obtained.
> */
> - pnfs_destroy_layout(NFS_I(inode));
> + pnfs_destroy_layout(NFS_I(inode), 0);
> rpc_wake_up(&tbl->slot_tbl_waitq);
> goto reset;
> /* RPC connection errors */
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> index c75ad98..ab0a1e5 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> @@ -1088,7 +1088,7 @@ static int ff_layout_async_handle_error_v4(struct rpc_task *task,
> * i/o and all i/o waiting on the slot table to the MDS until
> * layout is destroyed and a new valid layout is obtained.
> */
> - pnfs_destroy_layout(NFS_I(inode));
> + pnfs_destroy_layout(NFS_I(inode), 0);
> rpc_wake_up(&tbl->slot_tbl_waitq);
> goto reset;
> /* RPC connection errors */
> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> index 6fb7cb6..3b4a063 100644
> --- a/fs/nfs/nfs4super.c
> +++ b/fs/nfs/nfs4super.c
> @@ -95,7 +95,7 @@ static void nfs4_evict_inode(struct inode *inode)
> nfs_inode_return_delegation_noreclaim(inode);
> /* Note that above delegreturn would trigger pnfs return-on-close */
> pnfs_return_layout(inode);
> - pnfs_destroy_layout(NFS_I(inode));
> + pnfs_destroy_layout(NFS_I(inode), FLUSH_SYNC);
> /* First call standard NFS clear_inode() code */
> nfs_clear_inode(inode);
> }
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index d602fe9..09e66a8 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -696,7 +696,7 @@ pnfs_free_lseg_list(struct list_head *free_me)
> }
>
> void
> -pnfs_destroy_layout(struct nfs_inode *nfsi)
> +pnfs_destroy_layout(struct nfs_inode *nfsi, int how)
> {
> struct pnfs_layout_hdr *lo;
> LIST_HEAD(tmp_list);
> @@ -710,7 +710,7 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
> pnfs_layout_clear_fail_bit(lo, NFS_LAYOUT_RW_FAILED);
> spin_unlock(&nfsi->vfs_inode.i_lock);
> pnfs_free_lseg_list(&tmp_list);
> - nfs_commit_inode(&nfsi->vfs_inode, 0);
> + nfs_commit_inode(&nfsi->vfs_inode, how);
> pnfs_put_layout_hdr(lo);
> } else
> spin_unlock(&nfsi->vfs_inode.i_lock);
> @@ -1849,7 +1849,7 @@ pnfs_update_layout(struct inode *ino,
> }
> /* Destroy the existing layout and start over */
> if (time_after(jiffies, giveup))
> - pnfs_destroy_layout(NFS_I(ino));
> + pnfs_destroy_layout(NFS_I(ino), 0);
> /* Fallthrough */
> case -EAGAIN:
> break;
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index 8d507c3..3fe2160 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -245,7 +245,7 @@ size_t pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio,
> void pnfs_set_lo_fail(struct pnfs_layout_segment *lseg);
> struct pnfs_layout_segment *pnfs_layout_process(struct nfs4_layoutget *lgp);
> void pnfs_free_lseg_list(struct list_head *tmp_list);
> -void pnfs_destroy_layout(struct nfs_inode *);
> +void pnfs_destroy_layout(struct nfs_inode *, int how);

Can you also update the fallback pnfs_destroy_layout() for when CONFIG_NFS_V4_1=n?

fs/nfs/nfs4super.c: In function 'nfs4_evict_inode':
fs/nfs/nfs4super.c:98:2: error: too many arguments to function 'pnfs_destroy_layout'
pnfs_destroy_layout(NFS_I(inode), FLUSH_SYNC);
^~~~~~~~~~~~~~~~~~~
In file included from fs/nfs/nfs4super.c:13:0:
fs/nfs/pnfs.h:627:20: note: declared here
static inline void pnfs_destroy_layout(struct nfs_inode *nfsi)
^~~~~~~~~~~~~~~~~~~

Thanks,
Anna

> void pnfs_destroy_all_layouts(struct nfs_client *);
> int pnfs_destroy_layouts_byfsid(struct nfs_client *clp,
> struct nfs_fsid *fsid,
>

2017-11-29 20:32:28

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] nfs: make pnfs_destroy_layout() wait on commit when inode is being evicted

T24gTW9uLCAyMDE3LTExLTIwIGF0IDE2OjQzIC0wNTAwLCBTY290dCBNYXloZXcgd3JvdGU6DQo+
IE90aGVyd2lzZSBuZnNfY29tbWl0X2lub2RlKCkgY2FuIHNldCBJX0RJUlRZX0RBVEFTWU5DIGlu
IHRoZSBpbm9kZSdzDQo+IGlfc3RhdGUsIGFsbG93aW5nIHRoZSBmb2xsb3dpbmcgQlVHX09OIGlu
IGV2aWN0KCkgdG8gYmUgdHJpZ2dlcmVkOg0KPiANCj4gICAgICAgICBCVUdfT04oaW5vZGUtPmlf
c3RhdGUgIT0gKElfRlJFRUlORyB8IElfQ0xFQVIpKTsNCj4gDQo+IEZpeGVzOiAxZjE4YjgyYzM0
MzcgKCJwTkZTOiBFbnN1cmUgd2UgY29tbWl0IHRoZSBsYXlvdXQgaWYgaXQgaGFzDQo+IGJlZW4u
Li4iKQ0KPiBTaWduZWQtb2ZmLWJ5OiBTY290dCBNYXloZXcgPHNtYXloZXdAcmVkaGF0LmNvbT4N
Cj4gVGVzdGVkLWJ5OiBUaWdyYW4gTWtydGNoeWFuIDx0aWdyYW4ubWtydGNoeWFuQGRlc3kuZGU+
DQo+IC0tLQ0KPiAgZnMvbmZzL2ZpbGVsYXlvdXQvZmlsZWxheW91dC5jICAgICAgICAgfCAyICst
DQo+ICBmcy9uZnMvZmxleGZpbGVsYXlvdXQvZmxleGZpbGVsYXlvdXQuYyB8IDIgKy0NCj4gIGZz
L25mcy9uZnM0c3VwZXIuYyAgICAgICAgICAgICAgICAgICAgIHwgMiArLQ0KPiAgZnMvbmZzL3Bu
ZnMuYyAgICAgICAgICAgICAgICAgICAgICAgICAgfCA2ICsrKy0tLQ0KPiAgZnMvbmZzL3BuZnMu
aCAgICAgICAgICAgICAgICAgICAgICAgICAgfCAyICstDQo+ICA1IGZpbGVzIGNoYW5nZWQsIDcg
aW5zZXJ0aW9ucygrKSwgNyBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMv
ZmlsZWxheW91dC9maWxlbGF5b3V0LmMNCj4gYi9mcy9uZnMvZmlsZWxheW91dC9maWxlbGF5b3V0
LmMNCj4gaW5kZXggNGU1NGQ4Yi4uMjhiNzIyOCAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL2ZpbGVs
YXlvdXQvZmlsZWxheW91dC5jDQo+ICsrKyBiL2ZzL25mcy9maWxlbGF5b3V0L2ZpbGVsYXlvdXQu
Yw0KPiBAQCAtMTY5LDcgKzE2OSw3IEBAIHN0YXRpYyBpbnQgZmlsZWxheW91dF9hc3luY19oYW5k
bGVfZXJyb3Ioc3RydWN0DQo+IHJwY190YXNrICp0YXNrLA0KPiAgCQkgKiBpL28gYW5kIGFsbCBp
L28gd2FpdGluZyBvbiB0aGUgc2xvdCB0YWJsZSB0byB0aGUNCj4gTURTIHVudGlsDQo+ICAJCSAq
IGxheW91dCBpcyBkZXN0cm95ZWQgYW5kIGEgbmV3IHZhbGlkIGxheW91dCBpcw0KPiBvYnRhaW5l
ZC4NCj4gIAkJICovDQo+IC0JCXBuZnNfZGVzdHJveV9sYXlvdXQoTkZTX0koaW5vZGUpKTsNCj4g
KwkJcG5mc19kZXN0cm95X2xheW91dChORlNfSShpbm9kZSksIDApOw0KPiAgCQlycGNfd2FrZV91
cCgmdGJsLT5zbG90X3RibF93YWl0cSk7DQo+ICAJCWdvdG8gcmVzZXQ7DQo+ICAJLyogUlBDIGNv
bm5lY3Rpb24gZXJyb3JzICovDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvZmxleGZpbGVsYXlvdXQv
ZmxleGZpbGVsYXlvdXQuYw0KPiBiL2ZzL25mcy9mbGV4ZmlsZWxheW91dC9mbGV4ZmlsZWxheW91
dC5jDQo+IGluZGV4IGM3NWFkOTguLmFiMGExZTUgMTAwNjQ0DQo+IC0tLSBhL2ZzL25mcy9mbGV4
ZmlsZWxheW91dC9mbGV4ZmlsZWxheW91dC5jDQo+ICsrKyBiL2ZzL25mcy9mbGV4ZmlsZWxheW91
dC9mbGV4ZmlsZWxheW91dC5jDQo+IEBAIC0xMDg4LDcgKzEwODgsNyBAQCBzdGF0aWMgaW50DQo+
IGZmX2xheW91dF9hc3luY19oYW5kbGVfZXJyb3JfdjQoc3RydWN0IHJwY190YXNrICp0YXNrLA0K
PiAgCQkgKiBpL28gYW5kIGFsbCBpL28gd2FpdGluZyBvbiB0aGUgc2xvdCB0YWJsZSB0byB0aGUN
Cj4gTURTIHVudGlsDQo+ICAJCSAqIGxheW91dCBpcyBkZXN0cm95ZWQgYW5kIGEgbmV3IHZhbGlk
IGxheW91dCBpcw0KPiBvYnRhaW5lZC4NCj4gIAkJICovDQo+IC0JCXBuZnNfZGVzdHJveV9sYXlv
dXQoTkZTX0koaW5vZGUpKTsNCj4gKwkJcG5mc19kZXN0cm95X2xheW91dChORlNfSShpbm9kZSks
IDApOw0KPiAgCQlycGNfd2FrZV91cCgmdGJsLT5zbG90X3RibF93YWl0cSk7DQo+ICAJCWdvdG8g
cmVzZXQ7DQo+ICAJLyogUlBDIGNvbm5lY3Rpb24gZXJyb3JzICovDQo+IGRpZmYgLS1naXQgYS9m
cy9uZnMvbmZzNHN1cGVyLmMgYi9mcy9uZnMvbmZzNHN1cGVyLmMNCj4gaW5kZXggNmZiN2NiNi4u
M2I0YTA2MyAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL25mczRzdXBlci5jDQo+ICsrKyBiL2ZzL25m
cy9uZnM0c3VwZXIuYw0KPiBAQCAtOTUsNyArOTUsNyBAQCBzdGF0aWMgdm9pZCBuZnM0X2V2aWN0
X2lub2RlKHN0cnVjdCBpbm9kZSAqaW5vZGUpDQo+ICAJbmZzX2lub2RlX3JldHVybl9kZWxlZ2F0
aW9uX25vcmVjbGFpbShpbm9kZSk7DQo+ICAJLyogTm90ZSB0aGF0IGFib3ZlIGRlbGVncmV0dXJu
IHdvdWxkIHRyaWdnZXIgcG5mcyByZXR1cm4tb24tDQo+IGNsb3NlICovDQo+ICAJcG5mc19yZXR1
cm5fbGF5b3V0KGlub2RlKTsNCj4gLQlwbmZzX2Rlc3Ryb3lfbGF5b3V0KE5GU19JKGlub2RlKSk7
DQo+ICsJcG5mc19kZXN0cm95X2xheW91dChORlNfSShpbm9kZSksIEZMVVNIX1NZTkMpOw0KPiAg
CS8qIEZpcnN0IGNhbGwgc3RhbmRhcmQgTkZTIGNsZWFyX2lub2RlKCkgY29kZSAqLw0KPiAgCW5m
c19jbGVhcl9pbm9kZShpbm9kZSk7DQo+ICB9DQo+IA0KDQpOQUNLLiBJZiB5b3Ugc3RpbGwgaGF2
ZSBkaXJ0eSBwYWdlcyB3aGVuIHlvdSBnZXQgdG8gbmZzNF9ldmljdF9pbm9kZSgpLA0KdGhlbiB5
b3UgYXJlIHNjcmV3ZWQgbm8gbWF0dGVyIHdoYXQuDQoNClRoaXMgcHJvYmxlbSBzaG91bGQgYmUg
Zml4YWJsZSBpbiBuZnNfY29tbWl0X2lub2RlKCkgYnkgc2ltcGx5IGV4aXRpbmcNCmFmdGVyIHRo
ZSBjYWxsIHRvIG5mc19jb21taXRfZW5kKCkgaWYgcmVzID09IDAuDQoNCi0tIA0KVHJvbmQgTXlr
bGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5t
eWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQo=


2017-11-30 14:37:44

by Scott Mayhew

[permalink] [raw]
Subject: Re: [PATCH] nfs: make pnfs_destroy_layout() wait on commit when inode is being evicted

On Wed, 29 Nov 2017, Trond Myklebust wrote:

> On Mon, 2017-11-20 at 16:43 -0500, Scott Mayhew wrote:
> > Otherwise nfs_commit_inode() can set I_DIRTY_DATASYNC in the inode's
> > i_state, allowing the following BUG_ON in evict() to be triggered:
> >
> > BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
> >
> > Fixes: 1f18b82c3437 ("pNFS: Ensure we commit the layout if it has
> > been...")
> > Signed-off-by: Scott Mayhew <[email protected]>
> > Tested-by: Tigran Mkrtchyan <[email protected]>
> > ---
> > fs/nfs/filelayout/filelayout.c | 2 +-
> > fs/nfs/flexfilelayout/flexfilelayout.c | 2 +-
> > fs/nfs/nfs4super.c | 2 +-
> > fs/nfs/pnfs.c | 6 +++---
> > fs/nfs/pnfs.h | 2 +-
> > 5 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/nfs/filelayout/filelayout.c
> > b/fs/nfs/filelayout/filelayout.c
> > index 4e54d8b..28b7228 100644
> > --- a/fs/nfs/filelayout/filelayout.c
> > +++ b/fs/nfs/filelayout/filelayout.c
> > @@ -169,7 +169,7 @@ static int filelayout_async_handle_error(struct
> > rpc_task *task,
> > * i/o and all i/o waiting on the slot table to the
> > MDS until
> > * layout is destroyed and a new valid layout is
> > obtained.
> > */
> > - pnfs_destroy_layout(NFS_I(inode));
> > + pnfs_destroy_layout(NFS_I(inode), 0);
> > rpc_wake_up(&tbl->slot_tbl_waitq);
> > goto reset;
> > /* RPC connection errors */
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
> > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index c75ad98..ab0a1e5 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -1088,7 +1088,7 @@ static int
> > ff_layout_async_handle_error_v4(struct rpc_task *task,
> > * i/o and all i/o waiting on the slot table to the
> > MDS until
> > * layout is destroyed and a new valid layout is
> > obtained.
> > */
> > - pnfs_destroy_layout(NFS_I(inode));
> > + pnfs_destroy_layout(NFS_I(inode), 0);
> > rpc_wake_up(&tbl->slot_tbl_waitq);
> > goto reset;
> > /* RPC connection errors */
> > diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> > index 6fb7cb6..3b4a063 100644
> > --- a/fs/nfs/nfs4super.c
> > +++ b/fs/nfs/nfs4super.c
> > @@ -95,7 +95,7 @@ static void nfs4_evict_inode(struct inode *inode)
> > nfs_inode_return_delegation_noreclaim(inode);
> > /* Note that above delegreturn would trigger pnfs return-on-
> > close */
> > pnfs_return_layout(inode);
> > - pnfs_destroy_layout(NFS_I(inode));
> > + pnfs_destroy_layout(NFS_I(inode), FLUSH_SYNC);
> > /* First call standard NFS clear_inode() code */
> > nfs_clear_inode(inode);
> > }
> >
>
> NACK. If you still have dirty pages when you get to nfs4_evict_inode(),
> then you are screwed no matter what.
>
> This problem should be fixable in nfs_commit_inode() by simply exiting
> after the call to nfs_commit_end() if res == 0.

Okay, I'll try that out. Thanks for looking!
-Scott

>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> [email protected]

2017-12-08 21:00:13

by Scott Mayhew

[permalink] [raw]
Subject: [PATCH] nfs: don't wait on commit in nfs_commit_inode() if there were no commit requests

If there were no commit requests, then nfs_commit_inode() should not
wait on the commit or mark the inode dirty, otherwise the following
BUG_ON can be triggered:

[ 1917.130762] kernel BUG at fs/inode.c:578!
[ 1917.130766] Oops: Exception in kernel mode, sig: 5 [#1]
[ 1917.130768] SMP NR_CPUS=2048 NUMA pSeries
[ 1917.130772] Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi blocklayoutdriver rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc sg nx_crypto pseries_rng ip_tables xfs libcrc32c sd_mod crc_t10dif crct10dif_generic crct10dif_common ibmvscsi scsi_transport_srp ibmveth scsi_tgt dm_mirror dm_region_hash dm_log dm_mod
[ 1917.130805] CPU: 2 PID: 14923 Comm: umount.nfs4 Tainted: G ------------ T 3.10.0-768.el7.ppc64 #1
[ 1917.130810] task: c0000005ecd88040 ti: c00000004cea0000 task.ti: c00000004cea0000
[ 1917.130813] NIP: c000000000354178 LR: c000000000354160 CTR: c00000000012db80
[ 1917.130816] REGS: c00000004cea3720 TRAP: 0700 Tainted: G ------------ T (3.10.0-768.el7.ppc64)
[ 1917.130820] MSR: 8000000100029032 <SF,EE,ME,IR,DR,RI> CR: 22002822 XER: 20000000
[ 1917.130828] CFAR: c00000000011f594 SOFTE: 1
GPR00: c000000000354160 c00000004cea39a0 c0000000014c4700 c0000000018cc750
GPR04: 000000000000c750 80c0000000000000 0600000000000000 04eeb76bea749a03
GPR08: 0000000000000034 c0000000018cc758 0000000000000001 d000000005e619e8
GPR12: c00000000012db80 c000000007b31200 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 c000000000dfc3ec 0000000000000000 c0000005eefc02c0
GPR28: d0000000079dbd50 c0000005b94a02c0 c0000005b94a0250 c0000005b94a01c8
[ 1917.130867] NIP [c000000000354178] .evict+0x1c8/0x350
[ 1917.130871] LR [c000000000354160] .evict+0x1b0/0x350
[ 1917.130873] Call Trace:
[ 1917.130876] [c00000004cea39a0] [c000000000354160] .evict+0x1b0/0x350 (unreliable)
[ 1917.130880] [c00000004cea3a30] [c0000000003558cc] .evict_inodes+0x13c/0x270
[ 1917.130884] [c00000004cea3af0] [c000000000327d20] .kill_anon_super+0x70/0x1e0
[ 1917.130896] [c00000004cea3b80] [d000000005e43e30] .nfs_kill_super+0x20/0x60 [nfs]
[ 1917.130900] [c00000004cea3c00] [c000000000328a20] .deactivate_locked_super+0xa0/0x1b0
[ 1917.130903] [c00000004cea3c80] [c00000000035ba54] .cleanup_mnt+0xd4/0x180
[ 1917.130907] [c00000004cea3d10] [c000000000119034] .task_work_run+0x114/0x150
[ 1917.130912] [c00000004cea3db0] [c00000000001ba6c] .do_notify_resume+0xcc/0x100
[ 1917.130916] [c00000004cea3e30] [c00000000000a7b0] .ret_from_except_lite+0x5c/0x60
[ 1917.130919] Instruction dump:
[ 1917.130921] 7fc3f378 486734b5 60000000 387f00a0 38800003 4bdcb365 60000000 e95f00a0
[ 1917.130927] 694a0060 7d4a0074 794ad182 694a0001 <0b0a0000> 892d02a4 2f890000 40de0134

Signed-off-by: Scott Mayhew <[email protected]>
---
fs/nfs/write.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 5b5f464..4a379d7 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1890,6 +1890,8 @@ int nfs_commit_inode(struct inode *inode, int how)
if (res)
error = nfs_generic_commit_list(inode, &head, how, &cinfo);
nfs_commit_end(cinfo.mds);
+ if (res == 0)
+ return res;
if (error < 0)
goto out_error;
if (!may_wait)
--
2.9.5


2017-12-08 21:52:22

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] nfs: don't wait on commit in nfs_commit_inode() if there were no commit requests

T24gRnJpLCAyMDE3LTEyLTA4IGF0IDE2OjAwIC0wNTAwLCBTY290dCBNYXloZXcgd3JvdGU6DQo+
IElmIHRoZXJlIHdlcmUgbm8gY29tbWl0IHJlcXVlc3RzLCB0aGVuIG5mc19jb21taXRfaW5vZGUo
KSBzaG91bGQgbm90DQo+IHdhaXQgb24gdGhlIGNvbW1pdCBvciBtYXJrIHRoZSBpbm9kZSBkaXJ0
eSwgb3RoZXJ3aXNlIHRoZSBmb2xsb3dpbmcNCj4gQlVHX09OIGNhbiBiZSB0cmlnZ2VyZWQ6DQo+
IA0KPiBbIDE5MTcuMTMwNzYyXSBrZXJuZWwgQlVHIGF0IGZzL2lub2RlLmM6NTc4IQ0KPiBbIDE5
MTcuMTMwNzY2XSBPb3BzOiBFeGNlcHRpb24gaW4ga2VybmVsIG1vZGUsIHNpZzogNSBbIzFdDQo+
IFsgMTkxNy4xMzA3NjhdIFNNUCBOUl9DUFVTPTIwNDggTlVNQSBwU2VyaWVzDQo+IFsgMTkxNy4x
MzA3NzJdIE1vZHVsZXMgbGlua2VkIGluOiBpc2NzaV90Y3AgbGliaXNjc2lfdGNwIGxpYmlzY3Np
DQo+IHNjc2lfdHJhbnNwb3J0X2lzY3NpIGJsb2NrbGF5b3V0ZHJpdmVyIHJwY3NlY19nc3Nfa3Ji
NSBhdXRoX3JwY2dzcw0KPiBuZnN2NCBkbnNfcmVzb2x2ZXIgbmZzIGxvY2tkIGdyYWNlIGZzY2Fj
aGUgc3VucnBjIHNnIG54X2NyeXB0bw0KPiBwc2VyaWVzX3JuZyBpcF90YWJsZXMgeGZzIGxpYmNy
YzMyYyBzZF9tb2QgY3JjX3QxMGRpZg0KPiBjcmN0MTBkaWZfZ2VuZXJpYyBjcmN0MTBkaWZfY29t
bW9uIGlibXZzY3NpIHNjc2lfdHJhbnNwb3J0X3NycA0KPiBpYm12ZXRoIHNjc2lfdGd0IGRtX21p
cnJvciBkbV9yZWdpb25faGFzaCBkbV9sb2cgZG1fbW9kDQo+IFsgMTkxNy4xMzA4MDVdIENQVTog
MiBQSUQ6IDE0OTIzIENvbW06IHVtb3VudC5uZnM0IFRhaW50ZWQ6DQo+IEcgICAgICAgICAgICAg
ICAtLS0tLS0tLS0tLS0gVCAzLjEwLjAtNzY4LmVsNy5wcGM2NCAjMQ0KPiBbIDE5MTcuMTMwODEw
XSB0YXNrOiBjMDAwMDAwNWVjZDg4MDQwIHRpOiBjMDAwMDAwMDRjZWEwMDAwIHRhc2sudGk6DQo+
IGMwMDAwMDAwNGNlYTAwMDANCj4gWyAxOTE3LjEzMDgxM10gTklQOiBjMDAwMDAwMDAwMzU0MTc4
IExSOiBjMDAwMDAwMDAwMzU0MTYwIENUUjoNCj4gYzAwMDAwMDAwMDEyZGI4MA0KPiBbIDE5MTcu
MTMwODE2XSBSRUdTOiBjMDAwMDAwMDRjZWEzNzIwIFRSQVA6IDA3MDAgICBUYWludGVkOg0KPiBH
ICAgICAgICAgICAgICAgLS0tLS0tLS0tLS0tIFQgICgzLjEwLjAtNzY4LmVsNy5wcGM2NCkNCj4g
WyAxOTE3LjEzMDgyMF0gTVNSOiA4MDAwMDAwMTAwMDI5MDMyIDxTRixFRSxNRSxJUixEUixSST4g
IENSOg0KPiAyMjAwMjgyMiAgWEVSOiAyMDAwMDAwMA0KPiBbIDE5MTcuMTMwODI4XSBDRkFSOiBj
MDAwMDAwMDAwMTFmNTk0IFNPRlRFOiAxDQo+IEdQUjAwOiBjMDAwMDAwMDAwMzU0MTYwIGMwMDAw
MDAwNGNlYTM5YTAgYzAwMDAwMDAwMTRjNDcwMA0KPiBjMDAwMDAwMDAxOGNjNzUwDQo+IEdQUjA0
OiAwMDAwMDAwMDAwMDBjNzUwIDgwYzAwMDAwMDAwMDAwMDAgMDYwMDAwMDAwMDAwMDAwMA0KPiAw
NGVlYjc2YmVhNzQ5YTAzDQo+IEdQUjA4OiAwMDAwMDAwMDAwMDAwMDM0IGMwMDAwMDAwMDE4Y2M3
NTggMDAwMDAwMDAwMDAwMDAwMQ0KPiBkMDAwMDAwMDA1ZTYxOWU4DQo+IEdQUjEyOiBjMDAwMDAw
MDAwMTJkYjgwIGMwMDAwMDAwMDdiMzEyMDAgMDAwMDAwMDAwMDAwMDAwMA0KPiAwMDAwMDAwMDAw
MDAwMDAwDQo+IEdQUjE2OiAwMDAwMDAwMDAwMDAwMDAwIDAwMDAwMDAwMDAwMDAwMDAgMDAwMDAw
MDAwMDAwMDAwMA0KPiAwMDAwMDAwMDAwMDAwMDAwDQo+IEdQUjIwOiAwMDAwMDAwMDAwMDAwMDAw
IDAwMDAwMDAwMDAwMDAwMDAgMDAwMDAwMDAwMDAwMDAwMA0KPiAwMDAwMDAwMDAwMDAwMDAwDQo+
IEdQUjI0OiAwMDAwMDAwMDAwMDAwMDAwIGMwMDAwMDAwMDBkZmMzZWMgMDAwMDAwMDAwMDAwMDAw
MA0KPiBjMDAwMDAwNWVlZmMwMmMwDQo+IEdQUjI4OiBkMDAwMDAwMDA3OWRiZDUwIGMwMDAwMDA1
Yjk0YTAyYzAgYzAwMDAwMDViOTRhMDI1MA0KPiBjMDAwMDAwNWI5NGEwMWM4DQo+IFsgMTkxNy4x
MzA4NjddIE5JUCBbYzAwMDAwMDAwMDM1NDE3OF0gLmV2aWN0KzB4MWM4LzB4MzUwDQo+IFsgMTkx
Ny4xMzA4NzFdIExSIFtjMDAwMDAwMDAwMzU0MTYwXSAuZXZpY3QrMHgxYjAvMHgzNTANCj4gWyAx
OTE3LjEzMDg3M10gQ2FsbCBUcmFjZToNCj4gWyAxOTE3LjEzMDg3Nl0gW2MwMDAwMDAwNGNlYTM5
YTBdIFtjMDAwMDAwMDAwMzU0MTYwXQ0KPiAuZXZpY3QrMHgxYjAvMHgzNTAgKHVucmVsaWFibGUp
DQo+IFsgMTkxNy4xMzA4ODBdIFtjMDAwMDAwMDRjZWEzYTMwXSBbYzAwMDAwMDAwMDM1NThjY10N
Cj4gLmV2aWN0X2lub2RlcysweDEzYy8weDI3MA0KPiBbIDE5MTcuMTMwODg0XSBbYzAwMDAwMDA0
Y2VhM2FmMF0gW2MwMDAwMDAwMDAzMjdkMjBdDQo+IC5raWxsX2Fub25fc3VwZXIrMHg3MC8weDFl
MA0KPiBbIDE5MTcuMTMwODk2XSBbYzAwMDAwMDA0Y2VhM2I4MF0gW2QwMDAwMDAwMDVlNDNlMzBd
DQo+IC5uZnNfa2lsbF9zdXBlcisweDIwLzB4NjAgW25mc10NCj4gWyAxOTE3LjEzMDkwMF0gW2Mw
MDAwMDAwNGNlYTNjMDBdIFtjMDAwMDAwMDAwMzI4YTIwXQ0KPiAuZGVhY3RpdmF0ZV9sb2NrZWRf
c3VwZXIrMHhhMC8weDFiMA0KPiBbIDE5MTcuMTMwOTAzXSBbYzAwMDAwMDA0Y2VhM2M4MF0gW2Mw
MDAwMDAwMDAzNWJhNTRdDQo+IC5jbGVhbnVwX21udCsweGQ0LzB4MTgwDQo+IFsgMTkxNy4xMzA5
MDddIFtjMDAwMDAwMDRjZWEzZDEwXSBbYzAwMDAwMDAwMDExOTAzNF0NCj4gLnRhc2tfd29ya19y
dW4rMHgxMTQvMHgxNTANCj4gWyAxOTE3LjEzMDkxMl0gW2MwMDAwMDAwNGNlYTNkYjBdIFtjMDAw
MDAwMDAwMDFiYTZjXQ0KPiAuZG9fbm90aWZ5X3Jlc3VtZSsweGNjLzB4MTAwDQo+IFsgMTkxNy4x
MzA5MTZdIFtjMDAwMDAwMDRjZWEzZTMwXSBbYzAwMDAwMDAwMDAwYTdiMF0NCj4gLnJldF9mcm9t
X2V4Y2VwdF9saXRlKzB4NWMvMHg2MA0KPiBbIDE5MTcuMTMwOTE5XSBJbnN0cnVjdGlvbiBkdW1w
Og0KPiBbIDE5MTcuMTMwOTIxXSA3ZmMzZjM3OCA0ODY3MzRiNSA2MDAwMDAwMCAzODdmMDBhMCAz
ODgwMDAwMyA0YmRjYjM2NQ0KPiA2MDAwMDAwMCBlOTVmMDBhMA0KPiBbIDE5MTcuMTMwOTI3XSA2
OTRhMDA2MCA3ZDRhMDA3NCA3OTRhZDE4MiA2OTRhMDAwMSA8MGIwYTAwMDA+DQo+IDg5MmQwMmE0
IDJmODkwMDAwIDQwZGUwMTM0DQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBTY290dCBNYXloZXcgPHNt
YXloZXdAcmVkaGF0LmNvbT4NCj4gLS0tDQo+ICBmcy9uZnMvd3JpdGUuYyB8IDIgKysNCj4gIDEg
ZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKykNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMv
d3JpdGUuYyBiL2ZzL25mcy93cml0ZS5jDQo+IGluZGV4IDViNWY0NjQuLjRhMzc5ZDcgMTAwNjQ0
DQo+IC0tLSBhL2ZzL25mcy93cml0ZS5jDQo+ICsrKyBiL2ZzL25mcy93cml0ZS5jDQo+IEBAIC0x
ODkwLDYgKzE4OTAsOCBAQCBpbnQgbmZzX2NvbW1pdF9pbm9kZShzdHJ1Y3QgaW5vZGUgKmlub2Rl
LCBpbnQNCj4gaG93KQ0KPiAgCWlmIChyZXMpDQo+ICAJCWVycm9yID0gbmZzX2dlbmVyaWNfY29t
bWl0X2xpc3QoaW5vZGUsICZoZWFkLCBob3csDQo+ICZjaW5mbyk7DQo+ICAJbmZzX2NvbW1pdF9l
bmQoY2luZm8ubWRzKTsNCj4gKwlpZiAocmVzID09IDApDQo+ICsJCXJldHVybiByZXM7DQo+ICAJ
aWYgKGVycm9yIDwgMCkNCj4gIAkJZ290byBvdXRfZXJyb3I7DQo+ICAJaWYgKCFtYXlfd2FpdCkN
Cg0KTG9va3MgZ29vZCB0byBtZS4NCg0KUmV2aWV3ZWQtYnk6IFRyb25kIE15a2xlYnVzdCA8dHJv
bmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4NCg0KQW5uYSwgc2luY2UgdGhpcyBpcyBhbiBP
b3BzYWJsZSBpc3N1ZSwgcGVyaGFwcyB3ZSBzaG91bGQgcHJvcGFnYXRlIGl0DQp0aHJvdWdoIHN0
YWJsZUB2Z2VyPw0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWlu
dGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K