Message-ID: <6601abe90907241118u57370bf0j53a8c22147a892f0@mail.gmail.com>
Date:	Fri, 24 Jul 2009 11:18:38 -0700
From:	Curt Wohlgemuth <curtw@...gle.com>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Cc:	Mingming <cmm@...ibm.com>, Theodore Tso <tytso@....edu>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	Eric Sandeen <sandeen@...hat.com>,
	Andreas Dilger <adilger@....com>
Subject: Re: Fallocate and DirectIO

On Fri, Jul 24, 2009 at 11:02 AM, Aneesh Kumar
K.V<aneesh.kumar@...ux.vnet.ibm.com> wrote:
> On Fri, Jul 24, 2009 at 09:30:08AM -0700, Curt Wohlgemuth wrote:
>> On Thu, Jul 23, 2009 at 5:56 PM, Mingming<cmm@...ibm.com> wrote:
>> > On Thu, 2009-07-23 at 08:56 -0700, Curt Wohlgemuth wrote:
>> >> On Tue, Jul 21, 2009 at 6:27 PM, Curt Wohlgemuth<curtw@...gle.com> wrote:
>> >> > I spent a bit of time looking at this today.
>> >> >
>> >> > On Fri, Jun 12, 2009 at 10:33 AM, Theodore Tso<tytso@....edu> wrote:
>> >> >> On Fri, Jun 12, 2009 at 06:01:12PM +0530, Aneesh Kumar K.V wrote:
>> >> >>> Hi,
>> >> >>>
>> >> >>> I noticed yesterday that a write to fallocate
>> >> >>> space via directIO results in fallback to buffer_IO. ie the userspace
>> >> >>> pages get copied to the page cache and then call a sync.
>> >> >>>
>> >> >>> I guess this defeat the purpose of using directIO. May be we should
>> >> >>> consider this a high priority bug.
>> >> >
>> >> > My simple experiment -- without a journal -- shows that you're
>> >> > observation is correct.  *Except* if FALLOC_FL_KEEP_SIZE is used in
>> >> > the fallocate() call, in which case the page cache is *not* used.
>> >> >
>> >> > Pseudo-code example:
>> >> >
>> >> >  open(O_DIRECT)
>> >> >  fallocate(mode, 512MB)
>> >> >  while (! written 100MB)
>> >> >     write(64K)
>> >> >  close()
>> >> >
>> >> > If mode == FALLOC_FL_KEEP_SIZE, then no page cache is used.
>> >> > Otherwise, we *do* go through the page cache.
>> >> >
>> >> > It comes down to the fact that, since the i_size is not updated with
>> >> > KEEP_SIZE, then ext4_get_block() is called with create = 1, since the
>> >> > block that's needed is "beyond" the file end.
>> >>
>> > I think so.
>> > In the case of KEEP_SIZE, get_block() is called with create=1 before dio
>> > submit the real data IO, thus dio get a chance to convert the
>> > uninitalized extents to initialized before returns back to the caller.
>>
>> Ah, I see this now in ext4_direct_IO().  Thanks.
>>
>> > But in the case of non KEEP_SIZE, i.e. updating i_size after fallocate()
>> > case, we now have to fall back to buffered IO to ensure the extents
>> > conversion is happened in an ordering. Because if we convert the extents
>> > before submit the IO, and this conversion reached to disk, if system
>> > crash before the real data IO finished, then it could expose the stale
>> > data out, as the extent has already marked "initialized".
>>
>> Yes, that makes sense -- since i_size already covers the formerly
>> uninitialized, now initialized, extents.
>>
>> >> Ted, given your concerns over the performance impact of updating the
>> >> extents during direct I/O writes, it would seem that the fact that
>> >> when KEEP_SIZE is specified we do the DMA (and don't go through the
>> >> page cache) would be a problem/bug.  At least, it seems that the
>> >> performance issue is the same regardless of whether KEEP_SIZE is used
>> >> on the fallocate or not: in both we're dealing with an uninitialized
>> >> extent.  Do you agree?
>> >
>> > Here is what I thought...
>> >
>> > I think updating the extents itself is not a big performance concern, In
>> > the non KEEP_SIZE case, if we don't want to fall back to buffered IO,
>> > ext4 DIO has to wait for the journal to commit the transaction which
>> > converts extents to complete, before DIO could return back apps, this
>> > could be a big latency. That seems what xfs does.
>>
>> Wouldn't this still be an exposure to stale data?  The only way for
>> this to work, if i_size already covers the uninit extents, is to make
>> sure the data goes to disk before the extents get converted and
>> committed.  Since the extents are converted in the ext4_get_block()
>> path, before DIO actually performs the data write, this seems to be
>> too late.
>>
>> > For KEEP_SIZE case, The conversion actually could happen before the
>> > related IO reach to disk, I guess the oraph inode list protects stale
>> > data get exposed in this case.
>>
>> I'm sorry, I don't follow you here.
>>
>> >> I'm exploring (a) what this performance penalty is for the journal
>> >> commit; and (b) can we at least avoid the page cache if your
>> >> conditions above (no journal commit; no new extent blocks) are met.
>> >
>> > In fact, in the case of no journal, as long as the extents conversion
>> > happens after the data IO reach to disk, it should be safe, am I right?
>> > If system crash before the extent conversion finish, we only lost
>> > recently updated IO, but won't expose the stale data out, as the extents
>> > is still marked as uninitialized.
>>
>> But again, the extent conversion (and mark_inode_dirty()) happens at
>> get_block time, before the data goes to disk.
>>
>> For KEEP_SIZE, this isn't an exposure because i_size prevents the data
>> from being read.  But without KEEP_SIZE, this would seem to be a
>> problem, right?
>>
>> (From a practical perspective, there's also a problem getting real DIO
>> to work without KEEP_SIZE in the fallocate():  the decision to send
>> "create=0" to ext4_get_block() happens in VFS code, and there's no way
>> to tell in the get_block path that "this is a 'no create' for a write,
>> vs. a read.)
>
> What we need is to track I/O's untill they hit the disk. This will
> help us to do data=guarded and also help in the above case. So
> for directIO we should use blockdev_direct_IO_own_locking and the get_block
> used should split the uninit extent the needed way but still mark it
> uninit. That would make sure a read will see the uninit extent and return
> zero as expected. Now on IO completion we should mark split uninit extent
> as init.

I can see how using DIO_OWN_LOCKING would allow a write to send
"create=1" to ext4_get_block().  That would be cool.

Are you then saying that we would need to postpone the
ext4_ext_convert_to_initialized() call in ext4_ext_get_blocks(), and
then have ext4_direct_IO() do this conversion on return from
blockdev_direct_IO_own_locking()?  That would seem to be required...

Thanks,
Curt
--
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