linux-ext4 - Re: Fun with fdatasync()

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

Message-ID: <20091012220043.GC3965@duck.suse.cz>
Date:	Tue, 13 Oct 2009 00:00:43 +0200
From:	Jan Kara <jack@...e.cz>
To:	Chris Mason <chris.mason@...cle.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-ext4@...r.kernel.org, jack@...e.cz, jbacik@...hat.com
Subject: Re: Fun with fdatasync()

  Hi,

On Mon 12-10-09 10:00:49, Chris Mason wrote:
> Josef has been doing some benchmarking around rpm performance on the
> filesystem and noticed that ext3 was going really fast on the
> fdatasyncs.
> 
> It seems pretty surprising to me that rpm -Uvh should do fdatasync
> without forcing fsyncs.  The files get overwritten, and any time we mark
> an inode dirty I_DIRTY_DATASYNC is getting set.
> 
> Handling of I_DIRTY_DATASYNC seems to work like this:
> 
> mark_inode_dirty() will set I_DIRTY_DATASYNC
> 
> ext3_sync_file will force a full commit on I_DIRTY_DATASYNC
> 
> This part makes good sense.  If the inode has changed, we're supposed to
> do a full commit.
> 
> writeback_single_inode is where things seem to go wrong:
> 
>         /* Set I_SYNC, reset I_DIRTY */
>         dirty = inode->i_state & I_DIRTY;
>         inode->i_state |= I_SYNC;
>         inode->i_state &= ~I_DIRTY;
> 
> Whoops, we just lost I_DIRTY_DATASYNC.  So, if pdflush comes in and does some
> writeback before we fdatasync, we'll skip the full commit because
> I_DIRTY_DATASYNC is gone.
> 
> The solution to me seems to be that we need to keep I_DIRTY_DATASYNC
> until the FS does an fsync/O_SYNC operation, and make the FS
> responsible for clearing it.
> 
> This does risk extra full fsyncs if the FS does a transaction commit on
> its own, but the FS should be responsible for keeping track of which
> transaction last changed a given file and doing a shortcut in the fsync
> code if the file is already on safely on disk.
> 
> Am I missing something?  I don't see how fdatasync is safe in our
> current usage.
  Yeah, we already discussed similar problems I_DIRTY flags with Ted and
others in thread "fsync on ext[34] working only by an accident" on
linux-ext4.
  I don't quite like clearing dirty flags only on sync - pdflush would then
unnecessarily try to get rid of those inodes and burn CPU on them.
Actually, mapping->private_list (and bh->b_assoc_buffers) is meant to be
used exactly for the purpose of tracking what needs to be written on fsync
so my current plan is to somehow utilize that list to fix the problem.
  Maybe I even get to that tomorrow ;) Thanks for the reminder.

									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