On Wed 03-04-13 19:41:38, Dmitry Monakhov wrote:
> On Wed, 3 Apr 2013 17:15:22 +0200, Jan Kara <jack@xxxxxxx> wrote:
> > On Wed 03-04-13 19:09:33, Dmitry Monakhov wrote:
> > > On Wed, 3 Apr 2013 16:50:55 +0200, Jan Kara <jack@xxxxxxx> wrote:
> > > > On Wed 03-04-13 18:21:46, Dmitry Monakhov wrote:
> > > > > inode store i_sync_tid and i_datasync_tid in order to optimize journal
> > > > > flushes and wait for commits only when necessary, but
> > > > > fields are declared as tid_t(not atomic_t as it done in ext3) so we
> > > > > have not synchronization between readers and writers, so gcc and cpu
> > > > > is allowed to perform prefetch, cache and other stuff.
> > > > > Looks like a bug, right?
> > > > Reads and writes to atomic_t aren't guaranteed to be any kind of a
> > > > barrier (if fact they are compiled as simple stores and loads on x86). Only
> > > > arithmetic operations on atomic types are special. So using tid_t is just
> > > > fine.
> > > Ok but what about prefetching?
> > > Compiler is allowed to prefetch on early stage ?
> > > should we use ACCESS_ONCE() or wmb() and rmb() here?
> > Yes, but prefetch can hardly happen before the syscall is started and
> > value from that time is enough. We just have to be sure that if user can
> > prove write(2) happened before fsync(2), then data written by write(2) are
> > on disk. So I don't think we need any barriers there.
> Sorry for be annoying but what prevents us from following situation?:
> DD:
> fallocate(2)
> write(2)
>
> fsync(2)
> {prefetch}commit_tid = ie->i_sync_tid (T1)
> [flushd]
> ->convert_extents
> -> ei->i_sync_tid = current_tid (T2)
>
> Observe that commit_tid == T1 (too old)
> issue a barrier and exit but
> data still in transaction which is not yet committed
Heh, ok, you are careful :). So I have to be as well. Races with extent
conversion specifically are prevented by the call to
ext4_flush_unwritten_io(). That call effectively establishes a full barrier
by taking and dropping a spinlock. So if that call saw empty list of
conversions, we must later see fresh value of i_sync_tid.
Now ext4_flush_unwritten_io() is going away in my cleanup patches but then
the synchronization of writeback (and extent conversion) from flusher
thread and fsync() happens in filemap_write_and_wait_range() which
establishes similar barrier by looking at PageWriteback bit - i.e. if it
sees PageWriteback cleared, we must also see new value of i_sync_tid.
Thanks for poking into this because it made me realize things aren't as
trivial as I thought they are.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
