Re: [PATCH 4/7] ext4: fsync should wait for DIO writers

On Mon, 10 Sep 2012 11:51:35 +0200, Jan Kara [off-list ref] wrote:

On Sun 09-09-12 21:27:11, Dmitry Monakhov wrote:

quoted

fsync and punch_hole are the places where we have to wait for all
existing writers (writeback, aio, dio), but currently we simply
flush pended end_io request which is not sufficient.

  Why not? I guess you mean the fact that there can be DIO in flight for
which end_io() was not called so it is not queued in the queue? But that is
OK - we have not yet called aio_complete() for that IO so for userspace the
write has not happened yet. Thus there's no need to flush it to disk -
fsync() does not say anything about writes in progress while fsync is
called.
From posix point of view(which is good one) may be wait for aio-dio's is

overwhelming guarantee.

quoted

Even more i_mutex is not holded while punch_hole which obviously
result in dangerous data corruption due to write-after-free.

  Yes, that's a bug. I also noticed that but didn't get to fixing it (I'm
actually working on a more long term fix using range locking but that's
more of a research project so having somehow fixed at least the most
blatant locking problems is good).

Yes you right. In order to do things right we should block:
1) direct io
2) pagecache /mmap users (writeback, readpage)

A assumes I've fixed (1) but (2) is still exist

My current assumption is to do actions similar to writeback

   down_write(EXT4_I(inode)->i_data_sem)
   while (index <= end && pagevec_lookup(&pvec, mapping, index,...) {
        lock_page(pvec[i]);
        zero_user_page(pvec[i], 0, PAGE_SIZE);
        ret = try_to_release_page(pvec[i]);
   }
   /* At this moment we know that we locked all pages in range,
    * NOTE!!!! currently ext_remove_space may drop i_data_sem internally
    * so it should be modified to exit once i_mutex was dropped
   */
   ret = ext4_ext_remove_space(inode, from, to, NO_RELOCK)
   while (pvec_num)
         unlock_page(pvec[i])
   }
   up_write(EXT4_I(inode)->i_data_sem)

Number of locked pages should not be too large
Or even more instead of massive page locking, we can lock page
one by one, and simulate fake writeback, so all new writers will
wait on that bit, but readers will see zeroes.
   down_write(EXT4_I(inode)->i_data_sem)
   while (index <= end && pagevec_lookup(&pvec, mapping, index,...) {
        lock_page(pvec[i]);
        zero_user_page(pvec[i], 0, PAGE_SIZE);
        ret = try_to_release_page(pvec[i]);
        set_page_writeback(pvec[i]);
        unlock_page(pvec[i])
   }
   
   ret = ext4_ext_remove_space(inode, from, to, NO_RELOCK)
   while (pvec_num) {
         end_page_writeback(pvec[i])
   }
   up_write(EXT4_I(inode)->i_data_sem)

								Honza

quoted

This patch performs following changes:

- Guard punch_hole with i_mutex
- fsync and punch_hole now wait for all writers in flight
  NOTE: XXX write-after-free race is still possible because
  truncate_pagecache_range() is not completely reliable and where
  is no easy way to stop writeback while punch_hole is in progress.

Signed-off-by: Dmitry Monakhov <redacted>
---
 fs/ext4/extents.c |   10 ++++++++--
 fs/ext4/fsync.c   |    1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e993879..8252651 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c

@@ -4845,6 +4845,7 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 			return err;
 	}
 
+	mutex_lock(&inode->i_mutex);
 	/* Now release the pages */
 	if (last_page_offset > first_page_offset) {
 		truncate_pagecache_range(inode, first_page_offset,

@@ -4852,12 +4853,15 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 	}
 
 	/* finish any pending end_io work */
+	inode_dio_wait(inode);
 	ext4_flush_completed_IO(inode);
 
 	credits = ext4_writepage_trans_blocks(inode);
 	handle = ext4_journal_start(inode, credits);
-	if (IS_ERR(handle))
-		return PTR_ERR(handle);
+	if (IS_ERR(handle)) {
+		err = PTR_ERR(handle);
+		goto out_mutex;
+	}
 
 	err = ext4_orphan_add(handle, inode);
 	if (err)

@@ -4951,6 +4955,8 @@ out:
 	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
 	ext4_mark_inode_dirty(handle, inode);
 	ext4_journal_stop(handle);
+out_mutex:
+	mutex_unlock(&inode->i_mutex);
 	return err;
 }
 int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 24f3719..290c5cf 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c

@@ -204,6 +204,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (inode->i_sb->s_flags & MS_RDONLY)
 		goto out;
 
+	inode_dio_wait(inode);
 	ret = ext4_flush_completed_IO(inode);
 	if (ret < 0)
 		goto out;

-- 
1.7.7.6

-- 
Jan Kara [off-list ref]
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html