NFS regression - EIO is returned instead of ENOSPC

Hi Trond et al,
we seem to have a regression introduced by

commit 7b281ee026552f10862b617a2a51acf49c829554
NFS: fsync() must exit with an error if page writeback failed

which has found it's way (in different form into -stable releases).

The problem is that an NFSERR_NOSPC comes through as EIO.

e.g. if /mnt2/ if an nfs mounted filesystem that has no space then

strace dd if=/dev/zero conv=fsync >> /mnt2/afile count=1

reported Input/output error and the relevant parts of the strace output are:

write(1, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512) = 512
fsync(1) = -1 EIO (Input/output error)
close(1) = -1 ENOSPC (No space left on device)

i.e. we get an EIO from fsync, then the ENOSPC comes with the close.

If don't do the fsync, then:

strace dd if=/dev/zero >> /mnt2/afile count=1

write(1, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512) = 512
close(1) = -1 EIO (Input/output error)

we don't see the ENOSPC at all.

The problem is that filemap_fdatawait_range, when it sees a page with PageError set, will return -EIO unless AS_ENOSPC is set. NFS never sets AS_ENOSPC so we get -EIO.

Then nfs_file_fsync() will return as soon as it sees an error from filemap_write_and_wait_range(), which will be that EIO.

nfs_file_fsync_commit knows to prefer the error from ctx->error over any other error, but nfs_file_fsync() doesn't.

I see two ways to "fix" this.

We could get nfs{,4}_file_fsync() to always call nfs_file_fsync_commit() and use the error from the later in preference to the error from filemap_write_and_wait_range().

That results in this, more correct, behaviour:

write(1, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512) = 512
fsync(1) = -1 ENOSPC (No space left on device)
close(1) = 0

Or we could get nfs_context_set_write_error() to call mapping_set_error(), which would result in AS_ENOSPC being set and so filemap_write_and_wait_range() will return the "correct" error. This results in this behaviour:

write(1, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512) = 512
fsync(1) = -1 ENOSPC (No space left on device)
close(1) = -1 ENOSPC (No space left on device)

which is a bit odd. The first ENOSPC is from AS_ENOSPC. The second is from ctx->error.

I feel that calling mapping_set_error() is the "right" thing to do, but it would need a bit more work to avoid the double errors.

What approach would you prefer?

The two patches I used are:

index 582bb88..19c06b9 100644
@@ -295,12 +295,12 @@ nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	struct inode *inode = file->f_path.dentry->d_inode;
 
 	do {
-		ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
-		if (ret != 0)
-			break;
+		int ret1 = filemap_write_and_wait_range(inode->i_mapping, start, end);
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
 		mutex_unlock(&inode->i_mutex);
+		if (ret == 0)
+			ret = ret1;
 		/*
 		 * If nfs_file_fsync_commit detected a server reboot, then
 		 * resend all dirty pages that might have been covered by

index afddd66..f0d9a88 100644
@@ -96,15 +96,15 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	struct inode *inode = file->f_path.dentry->d_inode;
 
 	do {
-		ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
-		if (ret != 0)
-			break;
+		int ret1 = filemap_write_and_wait_range(inode->i_mapping, start, end);
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
 		if (!ret && !datasync)
 			/* application has asked for meta-data sync */
 			ret = pnfs_layoutcommit_inode(inode, true);
 		mutex_unlock(&inode->i_mutex);
+		if (ret == 0)
+			ret = ret1;
 		/*
 		 * If nfs_file_fsync_commit detected a server reboot, then
 		 * resend all dirty pages that might have been covered by

and

index 9347ab7..b0f5bb7 100644
@@ -140,6 +140,7 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
 	ctx->error = error;
 	smp_wmb();
 	set_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	mapping_set_error(ctx->dentry->d_inode->i_mapping, error);
 }
 
 static struct nfs_page *

Thanks,
NeilBrown

Bit of a clarification here. It didn't get into -stable, but we have the bug in our 3.0 based SLES11-SP2 through a different route (I assumed it came through stable but was being too hasty).

The bug first arrived in v3.1-rc1

commit 02c24a82187d5a628c68edfe71ae60dc135cd178

    fs: push i_mutex and filemap_write_and_wait down into ->fsync() handlers

was fixed by me in v3.3-rc1

commit 2edb6bc3852c681c0d948245bd55108dc6407604
NFS - fix recent breakage to NFS error handling.

the code was then messed up a bit by

commit a5c58892b427a2752e3ec44b0aad4ce9221dc63b
NFS: Create a v4-specific fsync function
in v3.6-rc1

and that mess was fixed by

commit 7b281ee026552f10862b617a2a51acf49c829554
NFS: fsync() must exit with an error if page writeback failed

which re-introduced the original problem in v3.6-rc6.

That first patch has been backported to SLES11 so now I'm fixing the bug again and finding it in mainline again :-)

NeilBrown

T24gV2VkLCAyMDEyLTEyLTEyIGF0IDA5OjUzICsxMTAwLCBOZWlsQnJvd24gd3JvdGU6DQo+IE9u IFdlZCwgMTIgRGVjIDIwMTIgMDk6Mjg6MTMgKzExMDAgTmVpbEJyb3duIDxuZWlsYkBzdXNlLmRl PiB3cm90ZToNCj4gDQo+ID4gDQo+ID4gSGkgVHJvbmQgZXQgYWwsDQo+ID4gIHdlIHNlZW0gdG8g aGF2ZSBhIHJlZ3Jlc3Npb24gaW50cm9kdWNlZCBieSANCj4gPiANCj4gPiBjb21taXQgN2IyODFl ZTAyNjU1MmYxMDg2MmI2MTdhMmE1MWFjZjQ5YzgyOTU1NA0KPiA+ICAgICBORlM6IGZzeW5jKCkg bXVzdCBleGl0IHdpdGggYW4gZXJyb3IgaWYgcGFnZSB3cml0ZWJhY2sgZmFpbGVkDQo+ID4gDQo+ ID4gd2hpY2ggaGFzIGZvdW5kIGl0J3Mgd2F5IChpbiBkaWZmZXJlbnQgZm9ybSBpbnRvIC1zdGFi bGUgcmVsZWFzZXMpLg0KPiANCj4gQml0IG9mIGEgY2xhcmlmaWNhdGlvbiBoZXJlLiAgSXQgZGlk bid0IGdldCBpbnRvIC1zdGFibGUsIGJ1dCB3ZSBoYXZlIHRoZSBidWcNCj4gaW4gb3VyIDMuMCBi YXNlZCBTTEVTMTEtU1AyIHRocm91Z2ggYSBkaWZmZXJlbnQgcm91dGUgKEkgYXNzdW1lZCBpdCBj YW1lDQo+IHRocm91Z2ggc3RhYmxlIGJ1dCB3YXMgYmVpbmcgdG9vIGhhc3R5KS4NCj4gDQo+IFRo ZSBidWcgZmlyc3QgYXJyaXZlZCBpbiB2My4xLXJjMQ0KPiANCj4gY29tbWl0IDAyYzI0YTgyMTg3 ZDVhNjI4YzY4ZWRmZTcxYWU2MGRjMTM1Y2QxNzgNCj4gICAgIGZzOiBwdXNoIGlfbXV0ZXggYW5k IGZpbGVtYXBfd3JpdGVfYW5kX3dhaXQgZG93biBpbnRvIC0+ZnN5bmMoKSBoYW5kbGVycw0KPiAN Cj4gDQo+IHdhcyBmaXhlZCBieSBtZSBpbiB2My4zLXJjMQ0KPiANCj4gY29tbWl0IDJlZGI2YmMz ODUyYzY4MWMwZDk0ODI0NWJkNTUxMDhkYzY0MDc2MDQNCj4gICAgIE5GUyAtIGZpeCByZWNlbnQg YnJlYWthZ2UgdG8gTkZTIGVycm9yIGhhbmRsaW5nLg0KPiANCj4gdGhlIGNvZGUgd2FzIHRoZW4g bWVzc2VkIHVwIGEgYml0IGJ5IA0KPiANCj4gY29tbWl0IGE1YzU4ODkyYjQyN2EyNzUyZTNlYzQ0 YjBhYWQ0Y2U5MjIxZGM2M2INCj4gICAgIE5GUzogQ3JlYXRlIGEgdjQtc3BlY2lmaWMgZnN5bmMg ZnVuY3Rpb24NCj4gaW4gdjMuNi1yYzENCj4gDQo+IGFuZCB0aGF0IG1lc3Mgd2FzIGZpeGVkIGJ5 IA0KPiANCj4gY29tbWl0IDdiMjgxZWUwMjY1NTJmMTA4NjJiNjE3YTJhNTFhY2Y0OWM4Mjk1NTQN Cj4gICAgIE5GUzogZnN5bmMoKSBtdXN0IGV4aXQgd2l0aCBhbiBlcnJvciBpZiBwYWdlIHdyaXRl YmFjayBmYWlsZWQNCj4gDQo+IHdoaWNoIHJlLWludHJvZHVjZWQgdGhlIG9yaWdpbmFsIHByb2Js ZW0gaW4gdjMuNi1yYzYuDQo+IA0KPiBUaGF0IGZpcnN0IHBhdGNoIGhhcyBiZWVuIGJhY2twb3J0 ZWQgdG8gU0xFUzExIHNvIG5vdyBJJ20gZml4aW5nIHRoZSBidWcNCj4gYWdhaW4gYW5kIGZpbmRp bmcgaXQgaW4gbWFpbmxpbmUgYWdhaW4gOi0pDQoNCkhtbS4uLiBJIGNhbiBzZWUgMiBwbGFjZXMg d2hlcmUgd2UncmUgc2V0dGluZyB0aGUgUGFnZUVycm9yIGZsYWcuDQoNCiAgICAgMS4gbmZzX3Vw ZGF0ZXBhZ2UoKTogaW4gdGhpcyBjYXNlLCB0aGUgZXJyb3Igb2NjdXJyZWQgd2hlbiB3ZSB0cmll ZA0KICAgICAgICB0byBjaGFuZ2UgdGhlIHBhZ2UgY29udGVudHMuIFNpbmNlIHdlJ3JlIGhvbGRp bmcgdGhlIHBhZ2UgbG9jaywNCiAgICAgICAgYW5kIHNvIHJhdGhlciB0aGFuIG1hcmsgdGhlIHBh Z2UgYXMgYmFkLCB3ZSBjb3VsZCBwcm9iYWJseSBqdXN0DQogICAgICAgIHdyaXRlIGJhY2sgZXhp c3RpbmcgZGlydHkgYXJlYXMgKHVzaW5nIG5mc193Yl9wYWdlKCkpIGFuZCB0aGVuDQogICAgICAg IHJlbW92ZSBpdCBmcm9tIHRoZSBtYXBwaW5nLg0KICAgICAyLiAgbmZzX3dyaXRlX2NvbXBsZXRp b24oKTogaGVyZSB0aGUgd3JpdGViYWNrIGVycm9yIGFwcGxpZXMgdG8gdGhlDQogICAgICAgIGVu dGlyZSBkaXJ0eSBhcmVhIG9uIHRoZSBwYWdlLCBhbmQgdGhlcmUgaXMgbm8gcG9pbnQgaW4gdHJ5 IHRvDQogICAgICAgIHdyaXRlIGJhY2sgYWdhaW4uIEJldHRlciBqdXN0IGV2aWN0IHRoZSBwYWdl IGZyb20gdGhlIHBhZ2UgY2FjaGUNCiAgICAgICAgKHdoaWNoIGlzIHdoYXQgbmZzX3phcF9tYXBw aW5nKCkgaXMgc3VwcG9zZWQgdG8gZG8pLiBXaGlsZQ0KICAgICAgICBzZXR0aW5nIHRoZSBQYWdl RXJyb3IgZmxhZyBkb2VzIGNhdXNlIHNvbWUgb2YgdGhlIHdyaXRlYmFjaw0KICAgICAgICBmdW5j dGlvbnMgdG8gcmV0dXJuIEVJTywgdGhhdCdzIG5vdCByZWFsbHkgd2hhdCB3ZSdyZSBhZnRlcjsg d2UNCiAgICAgICAgYWxyZWFkeSByZXBvcnQgZXJyb3JzIG1vcmUgY29tcGxldGVseSB2aWEgdGhl IG9wZW4gY29udGV4dC4NCg0KU28gZm9yIG5vdywgY2FuJ3Qgd2UganVzdCBjaGFuZ2UgbmZzX3Nl dF9wYWdlZXJyb3IoKSB0byBub3QgYm90aGVyDQpzZXR0aW5nIHRoZSBQR19lcnJvciBmbGFnPyBU aGVuIGluIHRoZSBmdXR1cmUgd2UgbWlnaHQgd2FudCB0byBtYWtlDQpuZnNfdXBkYXRlcGFnZSBh IGJpdCBtb3JlIHNvcGhpc3RpY2F0ZWQgaW4gaG93IGl0IGRlYWxzIHdpdGggdGhvc2UNCmVycm9y cy4uLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVy DQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

T24gVHVlLCAyMDEyLTEyLTExIGF0IDE4OjE2IC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6 DQo+IEhtbS4uLiBJIGNhbiBzZWUgMiBwbGFjZXMgd2hlcmUgd2UncmUgc2V0dGluZyB0aGUgUGFn ZUVycm9yIGZsYWcuDQo+IA0KPiAgICAgIDEuIG5mc191cGRhdGVwYWdlKCk6IGluIHRoaXMgY2Fz ZSwgdGhlIGVycm9yIG9jY3VycmVkIHdoZW4gd2UgdHJpZWQNCj4gICAgICAgICB0byBjaGFuZ2Ug dGhlIHBhZ2UgY29udGVudHMuIFNpbmNlIHdlJ3JlIGhvbGRpbmcgdGhlIHBhZ2UgbG9jaywNCj4g ICAgICAgICBhbmQgc28gcmF0aGVyIHRoYW4gbWFyayB0aGUgcGFnZSBhcyBiYWQsIHdlIGNvdWxk IHByb2JhYmx5IGp1c3QNCj4gICAgICAgICB3cml0ZSBiYWNrIGV4aXN0aW5nIGRpcnR5IGFyZWFz ICh1c2luZyBuZnNfd2JfcGFnZSgpKSBhbmQgdGhlbg0KPiAgICAgICAgIHJlbW92ZSBpdCBmcm9t IHRoZSBtYXBwaW5nLg0KPiAgICAgIDIuICBuZnNfd3JpdGVfY29tcGxldGlvbigpOiBoZXJlIHRo ZSB3cml0ZWJhY2sgZXJyb3IgYXBwbGllcyB0byB0aGUNCj4gICAgICAgICBlbnRpcmUgZGlydHkg YXJlYSBvbiB0aGUgcGFnZSwgYW5kIHRoZXJlIGlzIG5vIHBvaW50IGluIHRyeSB0bw0KPiAgICAg ICAgIHdyaXRlIGJhY2sgYWdhaW4uIEJldHRlciBqdXN0IGV2aWN0IHRoZSBwYWdlIGZyb20gdGhl IHBhZ2UgY2FjaGUNCj4gICAgICAgICAod2hpY2ggaXMgd2hhdCBuZnNfemFwX21hcHBpbmcoKSBp cyBzdXBwb3NlZCB0byBkbykuIFdoaWxlDQo+ICAgICAgICAgc2V0dGluZyB0aGUgUGFnZUVycm9y IGZsYWcgZG9lcyBjYXVzZSBzb21lIG9mIHRoZSB3cml0ZWJhY2sNCj4gICAgICAgICBmdW5jdGlv bnMgdG8gcmV0dXJuIEVJTywgdGhhdCdzIG5vdCByZWFsbHkgd2hhdCB3ZSdyZSBhZnRlcjsgd2UN Cj4gICAgICAgICBhbHJlYWR5IHJlcG9ydCBlcnJvcnMgbW9yZSBjb21wbGV0ZWx5IHZpYSB0aGUg b3BlbiBjb250ZXh0Lg0KPiANCj4gU28gZm9yIG5vdywgY2FuJ3Qgd2UganVzdCBjaGFuZ2UgbmZz X3NldF9wYWdlZXJyb3IoKSB0byBub3QgYm90aGVyDQo+IHNldHRpbmcgdGhlIFBHX2Vycm9yIGZs YWc/IFRoZW4gaW4gdGhlIGZ1dHVyZSB3ZSBtaWdodCB3YW50IHRvIG1ha2UNCj4gbmZzX3VwZGF0 ZXBhZ2UgYSBiaXQgbW9yZSBzb3BoaXN0aWNhdGVkIGluIGhvdyBpdCBkZWFscyB3aXRoIHRob3Nl DQo+IGVycm9ycy4uLg0KDQpVbHRpbWF0ZWx5LCB3aGF0IEknbSBzYXlpbmcgaXMgdGhhdCBQYWdl RXJyb3IgaXMgYSBoYWNrIGZvciBwYXNzaW5nDQplcnJvcnMgYXJvdW5kLiBTaW5jZSB3ZSBoYXZl IG91ciBvd24gaGFjayBmb3IgZG9pbmcgdGhlIHNhbWUsIHRoZW4gd2h5DQp1c2UgUGFnZUVycm9y IGF0IGFsbD8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRh aW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNv bQ0K

Sounds like a real possibility.

I just tested with

index 9347ab7..e5da5e8 100644
@@ -202,7 +202,6 @@ out:
 /* A writeback failed: mark the page as bad, and invalidate the page cache */
 static void nfs_set_pageerror(struct page *page)
 {
-	SetPageError(page);
 	nfs_zap_mapping(page_file_mapping(page)->host, page_file_mapping(page));
 }
 

and I get the correct "ENOSPC" error, so that's a good sign.

if you like.

I've haven't tried to examine all the consequences of the change to ensure there are no unintended side effect, but you know the code better than me so if you think it is safe - I suspect it is.

Thanks,
NeilBrown