spurious sillyrename after O_DIRECT writes get ENOSPC

PATCH New J. Bruce Fields 2 participants 7 messages Dec 8, 2017 – Jan 16, 2018

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2017-12-08 22:16 UTC #0 lore raw

Last year Christoph noticed a bug that could result in a file being unnecessarily sillyrenamed after O_DIRECT writes get ENOSPC:

	http://lkml.kernel.org/r/[email protected]

It's reproduceable on upstream, over v3 or v4.

I looked into it some more, and it seems to reproduce whenever a write system call results in multiple WRITE calls, only some of which receive ENOSPC. I think that's resulting in a leak of the wb_kref on some nfs_pages (possibly the ones corresponding to the ENOSPC failures?). Those nfs_pages in turn hold references on nfs_{lock,open}_contexts. So a "rm" on the client (even after the file is closed) results in a sillyrename.

I'll keep looking at this, but the relevant code is pretty opaque to me so far. Any ideas welcomed.

--b.

J. Bruce Fields

author

2017-12-13 17:18 UTC #1 lore raw

Quoted 15 lines · J. Bruce Fields

Last year Christoph noticed a bug that could result in a file being unnecessarily sillyrenamed after O_DIRECT writes get ENOSPC:
	http://lkml.kernel.org/r/[email protected]
It's reproduceable on upstream, over v3 or v4.
I looked into it some more, and it seems to reproduce whenever a write system call results in multiple WRITE calls, only some of which receive ENOSPC. I think that's resulting in a leak of the wb_kref on some nfs_pages (possibly the ones corresponding to the ENOSPC failures?). Those nfs_pages in turn hold references on nfs_{lock,open}_contexts. So a "rm" on the client (even after the file is closed) results in a sillyrename.

Quoted 2 lines

I'll keep looking at this, but the relevant code is pretty opaque to me so far. Any ideas welcomed.

Actually it looks like a leak of dreq->io_count? That prevents commits from being sent (which I'm also seeing in network traces--the succesfull WRITEs are unstable but never get committed), which means nfs_direct_commit_complete() is never called, and the reference taken on wb_kref in the request_commit case of nfs_direct_write_completion is never put.

--b.

Benjamin Coddington

2017-12-14 13:08 UTC #2 lore raw

Quoted 19 lines · J. Bruce Fields

Quoted 15 lines · J. Bruce Fields
Last year Christoph noticed a bug that could result in a file being unnecessarily sillyrenamed after O_DIRECT writes get ENOSPC:
	http://lkml.kernel.org/r/[email protected]
It's reproduceable on upstream, over v3 or v4.
I looked into it some more, and it seems to reproduce whenever a write system call results in multiple WRITE calls, only some of which receive ENOSPC. I think that's resulting in a leak of the wb_kref on some nfs_pages (possibly the ones corresponding to the ENOSPC failures?). Those nfs_pages in turn hold references on nfs_{lock,open}_contexts. So a "rm" on the client (even after the file is closed) results in a sillyrename.
Quoted 2 lines
I'll keep looking at this, but the relevant code is pretty opaque to me so far. Any ideas welcomed.

Quoted 6 lines

Actually it looks like a leak of dreq->io_count? That prevents commits from being sent (which I'm also seeing in network traces--the succesfull WRITEs are unstable but never get committed), which means nfs_direct_commit_complete() is never called, and the reference taken on wb_kref in the request_commit case of nfs_direct_write_completion is never put.

This sounds to me like the problem Scott's working - he sent a patch yesterday "nfs/pnfs: fix nfs_direct_req ref leak when i/o falls back to the mds".

I think the the rule should be that once we call nfs_pgio_completion_ops->init_hdr, we have to finish with ->completion. However, there are some paths where that is not the case.

The callgraph in between nfs_pgheader_init() and nfs_initiate_pgio() in nfs_generic_pg_pgios() for this case might show where we're bailing out early.

Ben

J. Bruce Fields

author

2017-12-14 16:36 UTC #3 lore raw

Quoted 29 lines · Benjamin Coddington

Quoted 19 lines · J. Bruce Fields
Quoted 15 lines · J. Bruce Fields
Last year Christoph noticed a bug that could result in a file being unnecessarily sillyrenamed after O_DIRECT writes get ENOSPC:
	http://lkml.kernel.org/r/[email protected]
It's reproduceable on upstream, over v3 or v4.
I looked into it some more, and it seems to reproduce whenever a write system call results in multiple WRITE calls, only some of which receive ENOSPC. I think that's resulting in a leak of the wb_kref on some nfs_pages (possibly the ones corresponding to the ENOSPC failures?). Those nfs_pages in turn hold references on nfs_{lock,open}_contexts. So a "rm" on the client (even after the file is closed) results in a sillyrename.
Quoted 2 lines
I'll keep looking at this, but the relevant code is pretty opaque to me so far. Any ideas welcomed.
Quoted 6 lines
Actually it looks like a leak of dreq->io_count? That prevents commits from being sent (which I'm also seeing in network traces--the succesfull WRITEs are unstable but never get committed), which means nfs_direct_commit_complete() is never called, and the reference taken on wb_kref in the request_commit case of nfs_direct_write_completion is never put.

Quoted 7 lines

This sounds to me like the problem Scott's working - he sent a patch yesterday "nfs/pnfs: fix nfs_direct_req ref leak when i/o falls back to the mds".
I think the the rule should be that once we call nfs_pgio_completion_ops->init_hdr, we have to finish with ->completion. However, there are some paths where that is not the case.

Yes, I wondered about that....

Unfortunately after some more tests now I was think I was wrong, the dreq_get()s and put()s are balanced and the bug is somewhere else--in the case of my particular reproducer. Argh. But yes I can easily believe there could be a leak there.

--b.

Quoted 4 lines

The callgraph in between nfs_pgheader_init() and nfs_initiate_pgio() in nfs_generic_pg_pgios() for this case might show where we're bailing out early.

J. Bruce Fields

author

2017-12-14 18:55 UTC #4 lore raw

Quoted 38 lines · J. Bruce Fields

Quoted 29 lines · Benjamin Coddington
Quoted 19 lines · J. Bruce Fields
Quoted 15 lines · J. Bruce Fields
Last year Christoph noticed a bug that could result in a file being unnecessarily sillyrenamed after O_DIRECT writes get ENOSPC:
	http://lkml.kernel.org/r/[email protected]
It's reproduceable on upstream, over v3 or v4.
I looked into it some more, and it seems to reproduce whenever a write system call results in multiple WRITE calls, only some of which receive ENOSPC. I think that's resulting in a leak of the wb_kref on some nfs_pages (possibly the ones corresponding to the ENOSPC failures?). Those nfs_pages in turn hold references on nfs_{lock,open}_contexts. So a "rm" on the client (even after the file is closed) results in a sillyrename.
Quoted 2 lines
I'll keep looking at this, but the relevant code is pretty opaque to me so far. Any ideas welcomed.
Quoted 6 lines
Actually it looks like a leak of dreq->io_count? That prevents commits from being sent (which I'm also seeing in network traces--the succesfull WRITEs are unstable but never get committed), which means nfs_direct_commit_complete() is never called, and the reference taken on wb_kref in the request_commit case of nfs_direct_write_completion is never put.
Quoted 7 lines
This sounds to me like the problem Scott's working - he sent a patch yesterday "nfs/pnfs: fix nfs_direct_req ref leak when i/o falls back to the mds".
I think the the rule should be that once we call nfs_pgio_completion_ops->init_hdr, we have to finish with ->completion. However, there are some paths where that is not the case.

Quoted 6 lines

Yes, I wondered about that....
Unfortunately after some more tests now I was think I was wrong, the dreq_get()s and put()s are balanced and the bug is somewhere else--in the case of my particular reproducer. Argh. But yes I can easily believe there could be a leak there.

So actually what happens is if you do a direct io write where some WRITEs succeed and the one fails, then this:

	if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
		dreq->flags = 0;
		dreq->error = hdr->error;
	}

clears the NFS_ODIRECT_DO_COMMIT flag, so nfs_direct_write_complete never scheduels the commit calls. It looks like that leaves a bunch of nfs_pages on some to-be-committed list, so we end up leaking a bunch of stuff, with the most visible symptom being an unnecessarily sillyrename on close.

I can just remove that clear of dreq_flags and that fixes the problem, but I doubt that's correct.

--b.

J. Bruce Fields

author

2017-12-19 16:56 UTC #5 lore raw

Quoted 8 lines · J. Bruce Fields

So actually what happens is if you do a direct io write where some WRITEs succeed and the one fails, then this:
	if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
		dreq->flags = 0;
		dreq->error = hdr->error;
	}

Quoted 8 lines

clears the NFS_ODIRECT_DO_COMMIT flag, so nfs_direct_write_complete never scheduels the commit calls. It looks like that leaves a bunch of nfs_pages on some to-be-committed list, so we end up leaking a bunch of stuff, with the most visible symptom being an unnecessarily sillyrename on close.
I can just remove that clear of dreq_flags and that fixes the problem, but I doubt that's correct.

Or, maybe it is. If *any* WRITE(s) involved in this write might need a commit or reschedule, then surely we should run nfs_direct_write_schedule_work and let it sort them out? I'm having trouble seeing why clearing that field partway through could ever be correct.

Trond, Anna, does the following look right?

--b.

J. Bruce Fields

author

2018-01-16 15:08 UTC #6 lore raw

PATCH 1/1 NFS: commit direct writes even if they fail partially

From: "J. Bruce Fields" <[email protected]>

If some of the WRITE calls making up an O_DIRECT write syscall fail, we neglect to commit, even if some of the WRITEs succeed.

We also depend on the commit code to free the reference count on the nfs_page taken in the "if (request_commit)" case at the end of nfs_direct_write_completion(). The problem was originally noticed because ENOSPC's encountered partway through a write would result in a closed file being sillyrenamed when it should have been unlinked.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfs/direct.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/nfs/direct.c +1 -3

index d2972d537469..8c10b0562e75 100644
@@ -775,10 +775,8 @@ static void nfs_direct_write_completion(struct nfs_pgio_header *hdr)
 
 	spin_lock(&dreq->lock);
 
-	if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
-		dreq->flags = 0;
+	if (test_bit(NFS_IOHDR_ERROR, &hdr->flags))
 		dreq->error = hdr->error;
-	}
 	if (dreq->error == 0) {
 		nfs_direct_good_bytes(dreq, hdr);
 		if (nfs_write_need_commit(hdr)) {

Signature · 1 line

2.14.3