NFS: call block plug around direct write

We bypass generic_file_aio_write() but would want to call block plug.

---
 fs/nfs/direct.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index a1b81dd..a485560 100644
@@ -46,6 +46,7 @@
 #include <linux/kref.h>
 #include <linux/slab.h>
 #include <linux/task_io_accounting_ops.h>
+#include <linux/blkdev.h>
 
 #include <linux/nfs_fs.h>
 #include <linux/nfs_page.h>

@@ -922,6 +923,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 	struct file *file = iocb->ki_filp;
 	struct address_space *mapping = file->f_mapping;
 	size_t count;
+	struct blk_plug plug;
 
 	count = iov_length(iov, nr_segs);
 	nfs_add_stats(mapping->host, NFSIOS_DIRECTWRITTENBYTES, count);

@@ -948,7 +950,9 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 
 	task_io_account_write(count);
 
+	blk_start_plug(&plug);
 	retval = nfs_direct_write(iocb, iov, nr_segs, pos, count);
+	blk_finish_plug(&plug);
 	if (retval > 0) {
 		struct inode *inode = mapping->host;
 

1.7.1.262.g5ef3d

---
 fs/nfs/direct.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index a485560..2cf3b45 100644
@@ -866,6 +866,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, const struct iovec *iov,
 	ssize_t retval = -EINVAL;
 	struct file *file = iocb->ki_filp;
 	struct address_space *mapping = file->f_mapping;
+	struct blk_plug plug;
 	size_t count;
 
 	count = iov_length(iov, nr_segs);

@@ -886,7 +887,9 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, const struct iovec *iov,
 
 	task_io_account_read(count);
 
+	blk_start_plug(&plug);
 	retval = nfs_direct_read(iocb, iov, nr_segs, pos);
+	blk_finish_plug(&plug);
 	if (retval > 0)
 		iocb->ki_pos = pos + retval;
 

1.7.1.262.g5ef3d

T24gVHVlLCAyMDEyLTA1LTE1IGF0IDIzOjM4ICswODAwLCBQZW5nIFRhbyB3cm90ZToNCj4gU2ln bmVkLW9mZi1ieTogUGVuZyBUYW8gPHRhby5wZW5nQGVtYy5jb20+DQo+IC0tLQ0KPiAgZnMvbmZz L2RpcmVjdC5jIHwgICAgMyArKysNCj4gIDEgZmlsZXMgY2hhbmdlZCwgMyBpbnNlcnRpb25zKCsp LCAwIGRlbGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9kaXJlY3QuYyBiL2Zz L25mcy9kaXJlY3QuYw0KPiBpbmRleCBhNDg1NTYwLi4yY2YzYjQ1IDEwMDY0NA0KPiAtLS0gYS9m cy9uZnMvZGlyZWN0LmMNCj4gKysrIGIvZnMvbmZzL2RpcmVjdC5jDQo+IEBAIC04NjYsNiArODY2 LDcgQEAgc3NpemVfdCBuZnNfZmlsZV9kaXJlY3RfcmVhZChzdHJ1Y3Qga2lvY2IgKmlvY2IsIGNv bnN0IHN0cnVjdCBpb3ZlYyAqaW92LA0KPiAgCXNzaXplX3QgcmV0dmFsID0gLUVJTlZBTDsNCj4g IAlzdHJ1Y3QgZmlsZSAqZmlsZSA9IGlvY2ItPmtpX2ZpbHA7DQo+ICAJc3RydWN0IGFkZHJlc3Nf c3BhY2UgKm1hcHBpbmcgPSBmaWxlLT5mX21hcHBpbmc7DQo+ICsJc3RydWN0IGJsa19wbHVnIHBs dWc7DQo+ICAJc2l6ZV90IGNvdW50Ow0KPiAgDQo+ICAJY291bnQgPSBpb3ZfbGVuZ3RoKGlvdiwg bnJfc2Vncyk7DQo+IEBAIC04ODYsNyArODg3LDkgQEAgc3NpemVfdCBuZnNfZmlsZV9kaXJlY3Rf cmVhZChzdHJ1Y3Qga2lvY2IgKmlvY2IsIGNvbnN0IHN0cnVjdCBpb3ZlYyAqaW92LA0KPiAgDQo+ ICAJdGFza19pb19hY2NvdW50X3JlYWQoY291bnQpOw0KPiAgDQo+ICsJYmxrX3N0YXJ0X3BsdWco JnBsdWcpOw0KPiAgCXJldHZhbCA9IG5mc19kaXJlY3RfcmVhZChpb2NiLCBpb3YsIG5yX3NlZ3Ms IHBvcyk7DQo+ICsJYmxrX2ZpbmlzaF9wbHVnKCZwbHVnKTsNCj4gIAlpZiAocmV0dmFsID4gMCkN Cj4gIAkJaW9jYi0+a2lfcG9zID0gcG9zICsgcmV0dmFsOw0KPiAgDQoNCkRpdHRvLiBUaGlzIGlz IG5vdCBnZW5lcmljLi4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50 IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5l dGFwcC5jb20NCg0K

We in this case is the pnfs block driver at most. Thus these should be pnfs block code.

I thought so too.

But reading the code more closely it might be hard for the blocklayout to figure out the right state to call these two. Specially the call to blk_finish_plug(). So you might need to add a new LD API such as LD()->finish_plug() which is empty for others.

But again inspecting the code it looks like blk_start_plug() is a no-op and blk_finish_plug() is specially optimized for the empty case.

So is it worth it, the extra effort? I do understand the temptation to get lazy here.

Just my $0.017
Boaz

Agreed. Just that for buffer IO case all call into block plug, so I thought it might be OK in DIO case as well... Anyway. I will make it block specific and call it LD()->dio_begin and LD()->dio_end. I am cooking some dio alignment patches for block layout driver and would love to put them there as well.

They are empty only when !CONFIG_BLOCK. I will make it block layout specific as said above.

Thanks,
Tao

You miss understood. At the very top of blk_finish_plug() there is an if (list_empty()) return. So if there was no activity between plug/unplug the function is very fast.

But your above plan for alignment+plug sounds very good.

Thanks
Boaz

T24gVHVlLCAyMDEyLTA1LTE1IGF0IDIzOjM4ICswODAwLCBQZW5nIFRhbyB3cm90ZToNCj4gV2Ug YnlwYXNzIGdlbmVyaWNfZmlsZV9haW9fd3JpdGUoKSBidXQgd291bGQgd2FudCB0byBjYWxsIGJs b2NrIHBsdWcuDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBQZW5nIFRhbyA8dGFvLnBlbmdAZW1jLmNv bT4NCj4gLS0tDQo+ICBmcy9uZnMvZGlyZWN0LmMgfCAgICA0ICsrKysNCj4gIDEgZmlsZXMgY2hh bmdlZCwgNCBpbnNlcnRpb25zKCspLCAwIGRlbGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBh L2ZzL25mcy9kaXJlY3QuYyBiL2ZzL25mcy9kaXJlY3QuYw0KPiBpbmRleCBhMWI4MWRkLi5hNDg1 NTYwIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvZGlyZWN0LmMNCj4gKysrIGIvZnMvbmZzL2RpcmVj dC5jDQo+IEBAIC00Niw2ICs0Niw3IEBADQo+ICAjaW5jbHVkZSA8bGludXgva3JlZi5oPg0KPiAg I2luY2x1ZGUgPGxpbnV4L3NsYWIuaD4NCj4gICNpbmNsdWRlIDxsaW51eC90YXNrX2lvX2FjY291 bnRpbmdfb3BzLmg+DQo+ICsjaW5jbHVkZSA8bGludXgvYmxrZGV2Lmg+DQo+ICANCj4gICNpbmNs dWRlIDxsaW51eC9uZnNfZnMuaD4NCj4gICNpbmNsdWRlIDxsaW51eC9uZnNfcGFnZS5oPg0KPiBA QCAtOTIyLDYgKzkyMyw3IEBAIHNzaXplX3QgbmZzX2ZpbGVfZGlyZWN0X3dyaXRlKHN0cnVjdCBr aW9jYiAqaW9jYiwgY29uc3Qgc3RydWN0IGlvdmVjICppb3YsDQo+ICAJc3RydWN0IGZpbGUgKmZp bGUgPSBpb2NiLT5raV9maWxwOw0KPiAgCXN0cnVjdCBhZGRyZXNzX3NwYWNlICptYXBwaW5nID0g ZmlsZS0+Zl9tYXBwaW5nOw0KPiAgCXNpemVfdCBjb3VudDsNCj4gKwlzdHJ1Y3QgYmxrX3BsdWcg cGx1ZzsNCj4gIA0KPiAgCWNvdW50ID0gaW92X2xlbmd0aChpb3YsIG5yX3NlZ3MpOw0KPiAgCW5m c19hZGRfc3RhdHMobWFwcGluZy0+aG9zdCwgTkZTSU9TX0RJUkVDVFdSSVRURU5CWVRFUywgY291 bnQpOw0KPiBAQCAtOTQ4LDcgKzk1MCw5IEBAIHNzaXplX3QgbmZzX2ZpbGVfZGlyZWN0X3dyaXRl KHN0cnVjdCBraW9jYiAqaW9jYiwgY29uc3Qgc3RydWN0IGlvdmVjICppb3YsDQo+ICANCj4gIAl0 YXNrX2lvX2FjY291bnRfd3JpdGUoY291bnQpOw0KPiAgDQo+ICsJYmxrX3N0YXJ0X3BsdWcoJnBs dWcpOw0KPiAgCXJldHZhbCA9IG5mc19kaXJlY3Rfd3JpdGUoaW9jYiwgaW92LCBucl9zZWdzLCBw b3MsIGNvdW50KTsNCj4gKwlibGtfZmluaXNoX3BsdWcoJnBsdWcpOw0KPiAgCWlmIChyZXR2YWwg PiAwKSB7DQo+ICAJCXN0cnVjdCBpbm9kZSAqaW5vZGUgPSBtYXBwaW5nLT5ob3N0Ow0KPiAgDQoN CmJpZyBOQUNLLi4uIFRoaXMgZG9lcyBub3QgYmVsb25nIGluIGdlbmVyaWMgTkZTIGNvZGU6IGl0 IGlzIDEwMCUgYmxvY2sNCnNwZWNpZmljLCBhbmQgbmVlZHMgdG8gZ28gaW4gdGhlIHBORlMgYmxv Y2tzIGNvZGUuDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50 YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5j b20NCg0K