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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D3DCEDD.4010509@fusionio.com>
Date:	Mon, 24 Jan 2011 20:11:25 +0100
From:	Jens Axboe <jaxboe@...ionio.com>
To:	Dave Chinner <david@...morbit.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"hch@...radead.org" <hch@...radead.org>
Subject: Re: [PATCH 07/10] fs: make generic file read/write functions plug

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@...ionio.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/