linux-kernel - Re: [PATCH] ext3: call blkdev_issue

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: <4BAD0D06.3000607@redhat.com>
Date:	Fri, 26 Mar 2010 14:37:42 -0500
From:	Eric Sandeen <sandeen@...hat.com>
To:	Andreas Dilger <adilger@....com>
CC:	Surbhi Palande <surbhi.palande@...onical.com>,
	linux-ext4@...r.kernel.org, Stephen Tweedie <sct@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ext3: call blkdev_issue_flush on fsync

On 03/26/2010 02:24 PM, Andreas Dilger wrote:
> On 2010-03-26, at 10:50, Surbhi Palande wrote:
>> To ensure that bits are truly on-disk after an fsync,
>> we should call blkdev_issue_flush if barriers are supported.
>>
>> @@ -87,5 +89,7 @@ int ext3_sync_file(struct file * file, struct  
>> dentry *dentry, int datasync)
>> 		ret = sync_inode(inode, &wbc);
>> 	}
>> out:
>> +	if (journal && (journal->j_flags & JFS_BARRIER))
>> +		blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
>> 	return ret;
> 
> 
> I don't think we need yet ANOTHER barrier here.  If the filesystem is  
> mounted in data={journaled,ordered} mode it will have flushed the data  
> to disk as part of the journal commit.  If there is an external  
> journal, there were patches posted to have it flush the data on the  
> filesystem device at transaction commit time.
> 
> Since fsync on any inode always implies sync of the journal, the only  
> time that this would be needed is if we are running in no-journal  
> mode, or possibly in data=writeback mode.

And no-journal mode isn't possible in ext3 :)

Actually unless I'm totally confused, this patch doesn't apply at all,
and we already have:

        if (log_start_commit(journal, commit_tid)) {
                log_wait_commit(journal, commit_tid);
                goto out;
        }

        /*
         * In case we didn't commit a transaction, we have to flush
         * disk caches manually so that data really is on persistent
         * storage
         */
        if (test_opt(inode->i_sb, BARRIER))
                blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
out:
        return ret;

in ext3_sync_file(), from commit 56fcad29d4b3cbcbb2ed47a9d3ceca3f57175417...

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/