Prev: What are the goals for the architecture of an in-kernel IR system?
Next: [PATCH] debugobjects: section mismatch cleanup
From: Andreas Dilger on 26 Mar 2010 15:30
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.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eric Sandeen on 26 Mar 2010 15:40
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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Surbhi Palande on 26 Mar 2010 16:00
Please ignore this patch! Sorry for this!

Warm Regards,
Surbhi.

On Fri, 2010-03-26 at 18:50 +0200, Surbhi Palande wrote:
> To ensure that bits are truly on-disk after an fsync,
> we should call blkdev_issue_flush if barriers are supported.
>
> This code is seen in ext4 through commits
> d755fb384250d6bd7fd18a0930e71965acc8e72e and
> 5f3481e9a80c240f169b36ea886e2325b9aeb745.
>
> Signed-off-by: Surbhi Palande <surbhi.palande(a)canonical.com>
> ---
> fs/ext3/fsync.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext3/fsync.c b/fs/ext3/fsync.c
> index d336341..2184a40 100644
> --- a/fs/ext3/fsync.c
> +++ b/fs/ext3/fsync.c
> @@ -29,6 +29,7 @@
> #include <linux/jbd.h>
> #include <linux/ext3_fs.h>
> #include <linux/ext3_jbd.h>
> +#include <linux/blkdev.h>
>
> /*
> * akpm: A new design for ext3_sync_file().
> @@ -46,6 +47,7 @@ int ext3_sync_file(struct file * file, struct dentry *dentry, int datasync)
> {
> struct inode *inode = dentry->d_inode;
> int ret = 0;
> + journal_t *journal = EXT3_SB(inode->i_sb)->s_journal;
>
> J_ASSERT(ext3_journal_current_handle() == NULL);
>
> @@ -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;
> }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: What are the goals for the architecture of an in-kernel IR system?
Next: [PATCH] debugobjects: section mismatch cleanup