linux-ext4 - Re: per inode fsync optimization question

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] [day] [month] [year] [list]

Message-ID: <20130403160353.GB17415@quack.suse.cz>
Date:	Wed, 3 Apr 2013 18:03:53 +0200
From:	Jan Kara <jack@...e.cz>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:	Jan Kara <jack@...e.cz>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: per inode fsync optimization question

On Wed 03-04-13 19:41:38, Dmitry Monakhov wrote:
> On Wed, 3 Apr 2013 17:15:22 +0200, Jan Kara <jack@...e.cz> wrote:
> > On Wed 03-04-13 19:09:33, Dmitry Monakhov wrote:
> > > On Wed, 3 Apr 2013 16:50:55 +0200, Jan Kara <jack@...e.cz> 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@...e.cz>
SUSE Labs, CR
--
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