Message-Id: <1255979091.4257.3.camel@mingming-laptop>
Date:	Mon, 19 Oct 2009 12:04:51 -0700
From:	Mingming <cmm@...ibm.com>
To:	Jiaying Zhang <jiayingz@...gle.com>
Cc:	ext4 development <linux-ext4@...r.kernel.org>,
	Michael Rubin <mrubin@...gle.com>,
	Manuel Benitez <rickyb@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: ext4 DIO read performance issue on SSD

On Thu, 2009-10-15 at 18:27 -0700, Jiaying Zhang wrote:
> Thinking about this again, I think there is another benefit to always
> allocate uninitialized blocks first in all kinds of write, including
> size-extending write.
> 
> Currently, we avoid exposing stale data during size-extending
> write by adding the inode to the orphan list before changing the
> file size. 

Jan kara corrected me on this misunderstanding before. The orphan list
in the direct IO code is there to trim the inode to the original size if
the system/fs crash in the middle of expanding a file. The file won't
get size updated until all the IO is completed.

The ordeded journal mode is there to avoid exposing stale data during
the size extending write.

> This helps us with journaled ext4 because the journal
> guarantees that the on-disk orphan list will be updated first before
> data is written to disk and i_size is updated. But with non-journal ext4,
> we don't update orphan list during size-extending write because
> without a journal, there is no guarantee on the ordering of writes to disk.
> So if the system crashes after the on-disk i_size is updated but before
> data hits to disk (this is rare but can happen during fsync), we may
> end up exposing stale data with non-journal ext4.
> 
> I think allocating uninitialized extend first during such size-extending
> write and then converting the extend into initialized during end_io
> time can help close this security hole with non-journal ext4.
> 
> Any thoughts on this?
> 

Probably true, something similar to data=guarded mode.
> Jiaying
> On Thu, Oct 15, 2009 at 10:27 AM, Mingming <cmm@...ibm.com> wrote:
> > On Wed, 2009-10-14 at 14:42 -0700, Jiaying Zhang wrote:
> >> On Wed, Oct 14, 2009 at 1:57 PM, Mingming <cmm@...ibm.com> wrote:
> >> > On Wed, 2009-10-14 at 12:48 -0700, Jiaying Zhang wrote:
> >> >> On Wed, Oct 14, 2009 at 11:48 AM, Mingming <cmm@...ibm.com> 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.
> >> >> >
> >> >> >>  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,
> >> >>
> >> >> Hmm, do you mean the filemap_write_and_wait_range() in
> >> >> __blockdev_direct_IO?
> >> >
> >> > yes, that's the one to flush the page cache before direct read.
> >> >
> >> I don't think that function is called with DIO_NO_LOCKING.
> >
> > Oh, I mean the filemap_write_and_wait_range() in generic_file_aio_read()
> >
> >> Also, if we no longer hold i_mutex lock during dio read, I think
> >> there is a time window that a buffer write can allocate an
> >> initialize block after dio read flushes page cache but
> >> before it calls get_block. Then that dio read can get that
> >> initialized block with stale data.
> >>
> >
> > ah, I think it over, the key is prevent get_block() expose initialized
> > extent to direct read. concurrent buffered write to hole could result in
> > get_block() allocate blocks before direct IO read. That could be
> > addressed in a similar way we did for async direct IO write to hole...
> >> Jiaying
> >>
> >> >> Or do we flush page cache after calling
> >> >> get_block in dio read?
> >> >>
> >> >> Jiaying
> >> >>
> >> >> > 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@...r.kernel.org
> >> >> >> 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@...r.kernel.org
> >> >> 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@...r.kernel.org
> >> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists