On Fri, Oct 16, 2009 at 11:56 AM, Mingming <cmm@xxxxxxxxxx> wrote: > On Thu, 2009-10-15 at 16:33 -0700, Jiaying Zhang wrote: >> On Thu, Oct 15, 2009 at 4:28 PM, Mingming <cmm@xxxxxxxxxx> wrote: >> > On Thu, 2009-10-15 at 13:07 -0700, Jiaying Zhang wrote: >> >> On Thu, Oct 15, 2009 at 10:31 AM, Mingming <cmm@xxxxxxxxxx> wrote: >> >> > On Wed, 2009-10-14 at 22:14 -0700, Jiaying Zhang wrote: >> >> >> Mingming, >> >> >> >> >> > >> >> > Hi Jiaying, >> >> > >> >> >> On Wed, Oct 14, 2009 at 11:48 AM, Mingming <cmm@xxxxxxxxxx> wrote: >> >> >> > On Fri, 2009-10-09 at 16:34 -0700, Jiaying Zhang wrote: >> >> >> >> Hello, >> >> >> >> >> >> >> >> Recently, we are evaluating the ext4 performance on a high speed SSD. >> >> >> >> One problem we found is that ext4 performance doesn't scale well with >> >> >> >> multiple threads or multiple AIOs reading a single file with O_DIRECT. >> >> >> >> E.g., with 4k block size, multiple-thread DIO AIO random read on ext4 >> >> >> >> can lose up to 50% throughput compared to the results we get via RAW IO. >> >> >> >> >> >> >> >> After some initial analysis, we think the ext4 performance problem is caused >> >> >> >> by the use of i_mutex lock during DIO read. I.e., during DIO read, we grab >> >> >> >> the i_mutex lock in __blockdev_direct_IO because ext4 uses the default >> >> >> >> DIO_LOCKING from the generic fs code. I did a quick test by calling >> >> >> >> blockdev_direct_IO_no_locking() in ext4_direct_IO() and I saw ext4 DIO read >> >> >> >> got 99% performance as raw IO. >> >> >> >> >> >> >> > >> >> >> > This is very interesting...and impressive number. >> >> >> > >> >> >> > I tried to change ext4 to call blockdev_direct_IO_no_locking() directly, >> >> >> > but then realize that we can't do this all the time, as ext4 support >> >> >> > ext3 non-extent based files, and uninitialized extent is not support on >> >> >> > ext3 format file. >> >> >> > >> >> >> >> As we understand, the reason why we want to take i_mutex lock during DIO >> >> >> >> read is to prevent it from accessing stale data that may be exposed by a >> >> >> >> simultaneous write. We saw that Mingming Cao has implemented a patch set >> >> >> >> with which when a get_block request comes from direct write, ext4 only >> >> >> >> allocates or splits an uninitialized extent. That uninitialized extent >> >> >> >> will be marked as initialized at the end_io callback. >> >> >> > >> >> >> > Though I need to clarify that with all the patches in mainline, we only >> >> >> > treat new allocated blocks form direct io write to holes, not to writes >> >> >> > to the end of file. I actually have proposed to treat the write to the >> >> >> > end of file also as unintialized extents, but there is some concerns >> >> >> > that this getting tricky with updating inode size when it is async IO >> >> >> > direct IO. So it didn't getting done yet. >> >> >> >> >> >> I read you previous email thread again. As I understand, the main >> >> >> concern for allocating uninitialized blocks in i_size extending write >> >> >> is that we may end up having uninitialized blocks beyond i_size >> >> >> if the system crashes during write. Can we protect this case by >> >> >> adding the inode into the orphan list in ext4_ext_direct_IO, >> >> >> i.e., same as we have done in ext4_ind_direct_IO? >> >> >> >> >> > >> >> > Sure we could do that, though initially I hoped we could get rid of >> >> > that:) >> >> > >> >> > The tricky part is async direct write to the end of file. By the time >> >> > the IO is completed, the inode may be truncated or extended larger. >> >> > Updating the most "safe" size is the part I haven't thought through yet. >> >> > >> >> >> >> Ok. I think I understand the problem better now :). >> >> >> >> Looking at the __blockdev_direct_IO(), I saw it actually treats >> >> size-extending aio dio write as synchronous and expects the dio to >> >> complete before return (fs/direct-io.c line 1204 and line 1056-1061). >> > >> > Oh? It seems it will keep the write async as long as it's not a partial >> > write >> > /* >> > * The only time we want to leave bios in flight is when a successful >> > * partial aio read or full aio write have been setup. In that case >> > * bio completion will call aio_complete. The only time it's safe to >> > * call aio_complete is when we return -EIOCBQUEUED, so we key on that. >> > * This had *better* be the only place that raises -EIOCBQUEUED. >> > */ >> > BUG_ON(ret == -EIOCBQUEUED); >> > if (dio->is_async && ret == 0 && dio->result && >> > ((rw & READ) || (dio->result == dio->size))) >> > ret = -EIOCBQUEUED; >> > >> > if (ret != -EIOCBQUEUED) >> > dio_await_completion(dio); >> > >> >> If I read the code correctly, dio->is_async is not set for file extending write: >> >> /* >> * For file extending writes updating i_size before data >> * writeouts complete can expose uninitialized blocks. So >> * even for AIO, we need to wait for i/o to complete before >> * returning in this case. >> */ >> dio->is_async = !is_sync_kiocb(iocb) && !((rw & WRITE) && >> (end > i_size_read(inode))); >> > > Ah... that's a little surprise to know. Async write to the end of file > is actually synchonous. That explains why ext3 async direct IO could > always expand inode i_size safely without having to worry about expose > stale data out. > > It would be better to fix this with end_io callback, user expects get > higher performance via async mode. > > But, these are seperate from removing i_mutex lock from direct IO read, > IMHO. The dio write to end of file won't extend inode size until the IO > is completed (AIO or non AIO mode). So if we remove the i_mutex lock > from the dio read, in the case of parallel dio/buffered write to end of > file and direct io read, it won't expose updated inode's block mapping > tree to parallel direct IO read, as the i_size hasn't been changed yet. > > The only concern to remove the the lock from dio read path, is the > buffered write to the hole. Isn't it? > Hmm. Will we be safe between dio read and size-extending buffer write if we don't hold i_mutex lock during dio read but still allocating initialized blocks in size-extending buffer write? As I understand, we update file size in ext4_write_end. The buffer write have finished at this time but the changes are still in memory. Currently, we prevent dio read from reading stale data by calling filemap_write_and_wait_range() after holding the i_mutex lock in __blockdev_direct_IO(). If we no longer hold i_mutex during dio read, a buffer write can finish after a dio read calls filemap_write_and_wait_range() but before it calls get_block(). At the time that dio read calls get_block(), the i_size of the file has been updated, but the written data is still in memory, so it may read stale data. I was thinking that we may change to call filemap_write_and_wait_range() after get_block() during dio read. This seems to eliminate the above problem. However, I am also worried about non-journal ext4. With the current code, I think there is a chance that we can expose stale data after reboot if the system crashed at a bad time. I think always converting uninitialized blocks to initialized during end_io time may help to close this security hole in non-journal ext4. Jiaying >> Jiaying >> >> >> Can we follow the same rule and check whether it is a size-extending >> >> aio write in ext4_end_io_dio()? In such cases, we can call >> >> ext4_end_aio_dio_nolock() synchronously instead of queuing >> >> the work. I think this will protect us from truncate because we >> >> are still holding i_mutex and i_alloc_sem. >> >> >> >> Jiaying >> >> >> >> > >> >> > >> >> >> Jiaying >> >> >> >> >> >> > >> >> >> >> We are wondering >> >> >> >> whether we can extend this idea to buffer write as well. I.e., we always >> >> >> >> allocate an uninitialized extent first during any write and convert it >> >> >> >> as initialized at the time of end_io callback. This will eliminate the need >> >> >> >> to hold i_mutex lock during direct read because a DIO read should never get >> >> >> >> a block marked initialized before the block has been written with new data. >> >> >> >> >> >> >> > >> >> >> > Oh I don't think so. For buffered IO, the data is being copied to >> >> >> > buffer, direct IO read would first flush what's in page cache to disk, >> >> >> > then read from disk. So if there is concurrent buffered write and direct >> >> >> > read, removing the i_mutex locks from the direct IO path should still >> >> >> > gurantee the right order, without having to treat buffered allocation >> >> >> > with uninitialized extent/end_io. >> >> >> > >> >> >> > The i_mutex lock, from my understanding, is there to protect direct IO >> >> >> > write to hole and concurrent direct IO read, we should able to remove >> >> >> > this lock for extent based ext4 file. >> >> >> > >> >> >> >> We haven't implemented anything yet because we want to ask here first to >> >> >> >> see whether this proposal makes sense to you. >> >> >> >> >> >> >> > >> >> >> > It does make sense to me. >> >> >> > >> >> >> > Mingming >> >> >> >> Regards, >> >> >> >> >> >> >> >> Jiaying >> >> >> >> -- >> >> >> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> >> >> >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> >> >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >> > >> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
