We bypass generic_file_aio_write() but would want to call block plug.
Signed-off-by: Peng Tao <[email protected]>
---
fs/nfs/direct.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index a1b81dd..a485560 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -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
On 05/15/2012 07:08 PM, Christoph Hellwig wrote:
> On Tue, May 15, 2012 at 11:38:22PM +0800, Peng Tao wrote:
>> We bypass generic_file_aio_write() but would want to call block plug.
>
> We in this case is the pnfs block driver at most. Thus these should
> be pnfs block code.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Tue, May 15, 2012 at 11:38:22PM +0800, Peng Tao wrote:
> We bypass generic_file_aio_write() but would want to call block plug.
We in this case is the pnfs block driver at most. Thus these should
be pnfs block code.
Signed-off-by: Peng Tao <[email protected]>
---
fs/nfs/direct.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index a485560..2cf3b45 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -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
On 05/16/2012 03:35 AM, Peng Tao wrote:
> On Wed, May 16, 2012 at 12:23 AM, Boaz Harrosh <[email protected]> wrote:
>> On 05/15/2012 07:08 PM, Christoph Hellwig wrote:
>>
>>> On Tue, May 15, 2012 at 11:38:22PM +0800, Peng Tao wrote:
>>>> We bypass generic_file_aio_write() but would want to call block plug.
>>>
>>> We in this case is the pnfs block driver at most. Thus these should
>>> be pnfs block code.
> 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.
>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>> 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.
>>
> They are empty only when !CONFIG_BLOCK. I will make it block layout
> specific as said above.
>
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
> Thanks,
> Tao
>> So is it worth it, the extra effort? I do understand the temptation
>> to get lazy here.
>>
>> Just my $0.017
>> Boaz
On Wed, May 16, 2012 at 12:23 AM, Boaz Harrosh <[email protected]> wrote:
> On 05/15/2012 07:08 PM, Christoph Hellwig wrote:
>
>> On Tue, May 15, 2012 at 11:38:22PM +0800, Peng Tao wrote:
>>> We bypass generic_file_aio_write() but would want to call block plug.
>>
>> We in this case is the pnfs block driver at most. Thus these should
>> be pnfs block code.
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.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> 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.
>
They are empty only when !CONFIG_BLOCK. I will make it block layout
specific as said above.
Thanks,
Tao
> So is it worth it, the extra effort? I do understand the temptation
> to get lazy here.
>
> Just my $0.017
> 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
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