linux-kernel - Re: [PATCH][RESEND] fs: always set I_DIRTY

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: <20161031224606.GA23793@quack2.suse.cz>
Date:   Mon, 31 Oct 2016 23:46:06 +0100
From:   Jan Kara <jack@...e.cz>
To:     Naohiro Aota <naota@...sp.net>
Cc:     linux-fsdevel@...r.kernel.org, viro@...iv.linux.org.uk,
        linux-kernel@...r.kernel.org, tytso@....edu, asraaiteng@...il.com,
        Jens Axboe <axboe@...com>
Subject: Re: [PATCH][RESEND] fs: always set I_DIRTY_TIME to fsync correctly
 on lazytime

On Tue 01-11-16 04:02:45, Naohiro Aota wrote:
> While lazytime states that "The on-disk timestamps are updated only
> when: ... - the application employs fsync(2), syncfs(2), or sync(2)"
> [1], it does not write a timestamp update on fsync().
> 
> [1] http://manpages.ubuntu.com/manpages/xenial/man8/mount.8.html
> 
> The following commands will reproduce the problem:
> 
> $ mount -o noatime,lazytime ext4.img /mnt/tmp
> $ cd /mnt/tmp
> (create an 128M file to fio, not to observe size update)
> $ dd if=/dev/zero of=wxyz.0.0 bs=1M count=128
> (do write/fsync)
> $ fio --name wxyz --direct=1 --buffered=0 --size=128m --bs=64k --rw=write \
>   --ioengine=sync --numjobs=1 --fsync=5
> 
> Since fio invokes 1 fsync per 5 writes, we should see rapid journal
> commits for timestamp update by tracing jbd2:jbd2_end_commit trace
> point. Only we can see are, however, some periodic (~5 sec) commits from
> bdi flush like below.
> 
> $ trace jbd2:jbd2_end_commit
>     jbd2/loop0-8-1617  [002] ....    96.637351: jbd2_end_commit: dev 7,0 transaction 5393 sync 0 head 5393
>     jbd2/loop0-8-1617  [000] ....   101.679411: jbd2_end_commit: dev 7,0 transaction 5394 sync 0 head 5393
>     jbd2/loop0-8-1617  [003] ....   106.743628: jbd2_end_commit: dev 7,0 transaction 5395 sync 0 head 5393
>     jbd2/loop0-8-1617  [001] ....   111.801964: jbd2_end_commit: dev 7,0 transaction 5396 sync 0 head 5393
> ...
> 
> The problem is __mark_inode_dirty() does not always flag I_DIRTY_TIME.
> It seems that it is no use to mark an inode I_DIRTY_TIME when the inode
> is already I_DIRTY_INODE. However, by that decision, we're skipping
> journal write if we invoke two fsync()s between two bdi flushes. As the
> following table shows, any fsync after the first fsync do nothing (if
> there's no update other than timestamp).
> 
> Event                | i_state      | journal
> ---------------------+--------------+------------------------
> <timestamp update>   | I_DIRTY_TIME | no write (lazytime)
> <fsync>              | I_DIRTY_SYNC | write timestamp update
> <timestamp update>   | I_DIRTY_SYNC | no write (lazytime)
> <fsync>              | I_DIRTY_SYNC | no write *BUG*
> ...
> <bdi flush>          | 0            |
> <timestamp update>   | I_DIRTY_TIME | no write (lazytime)
> <fsync>              | I_DIRTY_SYNC | write timestamp update
> 
> We should set I_DIRTY_TIME on the second timestamp update to let fsync()
> notice there's a timestamp update after the last inode writeout.
> 
> After this patch, we can see rapid trace of journal commit:
> $ trace jbd2:jbd2_end_commit
>     jbd2/loop0-8-1879  [002] ....   208.275057: jbd2_end_commit: dev 7,0 transaction 5364 sync 0 head 3343
>     jbd2/loop0-8-1879  [000] ....   208.302539: jbd2_end_commit: dev 7,0 transaction 5365 sync 0 head 3343
>     jbd2/loop0-8-1879  [000] ....   208.327238: jbd2_end_commit: dev 7,0 transaction 5366 sync 0 head 3343
>     jbd2/loop0-8-1879  [003] ....   208.347618: jbd2_end_commit: dev 7,0 transaction 5367 sync 0 head 3343
> ...
> 
> Reported-by: Asraa Ali Mardan <asraaiteng@...il.com>
> Signed-off-by: Naohiro Aota <naota@...sp.net>

Thanks for the patch. It makes sense. You can add:

Reviewed-by: Jan Kara <jack@...e.cz>

Jens, can you please merge the patch? Thanks!

								Honza
> ---
> 
>  fs/fs-writeback.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 05713a5..ace628c 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -2100,16 +2100,17 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  	 */
>  	smp_mb();
>  
> -	if (((inode->i_state & flags) == flags) ||
> -	    (dirtytime && (inode->i_state & I_DIRTY_INODE)))
> +	if ((inode->i_state & flags) == flags)
>  		return;
>  
>  	if (unlikely(block_dump))
>  		block_dump___mark_inode_dirty(inode);
>  
>  	spin_lock(&inode->i_lock);
> -	if (dirtytime && (inode->i_state & I_DIRTY_INODE))
> +	if (dirtytime && (inode->i_state & I_DIRTY_INODE)) {
> +		inode->i_state |= I_DIRTY_TIME;
>  		goto out_unlock_inode;
> +	}
>  	if ((inode->i_state & flags) != flags) {
>  		const int was_dirty = inode->i_state & I_DIRTY;
>  
> -- 
> 2.8.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR