Re: [PATCH 07/10] fs: make generic file read/write functions plug

[Jens Axboe <jaxboe@fusionio.com>]

On 2011-01-24 04:57, Dave Chinner wrote:
> On Sat, Jan 22, 2011 at 01:17:26AM +0000, Jens Axboe wrote:
>> Signed-off-by: Jens Axboe <jaxboe@fusionio.com>
>> ---
>>  mm/filemap.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
> .....
>> @@ -2432,11 +2436,13 @@ ssize_t generic_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>  {
>>  	struct file *file = iocb->ki_filp;
>>  	struct inode *inode = file->f_mapping->host;
>> +	struct blk_plug plug;
>>  	ssize_t ret;
>>  
>>  	BUG_ON(iocb->ki_pos != pos);
>>  
>>  	mutex_lock(&inode->i_mutex);
>> +	blk_start_plug(&plug);
>>  	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
>>  	mutex_unlock(&inode->i_mutex);
>>  
>> @@ -2447,6 +2453,7 @@ ssize_t generic_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>>  		if (err < 0 && ret > 0)
>>  			ret = err;
>>  	}
>> +	blk_finish_plug(&plug);
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL(generic_file_aio_write);
> 
> Why do you want to plug all writes? For non-synchronous buffered
> writes we won't be doing any IO, so why woul dwe want to plug and
> unplug in that case? Shouldn't the plug/unplug be places in
> .writepage for the buffered writeback case (which would handle sync
> writes, too)?

Good point, it probably should be placed a bit more clever.

> Also, what is the impact of not plugging here? You change
> generic_file_aio_write, but filesystems like XFS supply their own
> .aio_write method and hence woul dneed some kind of change, too?

Generally, performance tests need to be run and the appropriate places
to plug need to be found. It could potentially cause lower queue depth
on the device side, or less merging of ios if we miss one of the heavy
IO spots.

-- 
Jens Axboe