Re: [PATCH v3 0/7] ext4: Add multi-fsblock atomic write support with bigalloc

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

"Ritesh Harjani (IBM)" <ritesh.list@gmail.com> writes:

> This is v3 of multi-fsblock atomic write support using bigalloc. This has
> started looking into much better shape now. The major chunk of the design
> changes has been kept in Patch-4 & 5.
>
> This series can now be carefully reviewed, as all the error handling related
> code paths should be properly taken care of.
>

We spotted that multi-fsblock changes might need to force a journal
commit if there were mixed mappings in the underlying region e.g. say WUWUWUW...

The issue arises when, during block allocation, the unwritten ranges are
first zeroed out, followed by the unwritten-to-written extent
conversion. This conversion is part of a journaled metadata transaction
that has not yet been committed, as the transaction is still running.
If an iomap write then modifies the data on those multi-fsblocks and a
sudden power loss occurs before the transaction commits, the
unwritten-to-written conversion will not be replayed during journal
recovery. As a result, we end up with new data written over mapped
blocks, while the alternate unwritten blocks will read zeroes. This
could cause a torn write behavior for atomic writes.

So we were thinking we might need something like this. Hopefully this
should still be ok, as mixed mapping case mostly is a non-performance
critical path. Thoughts?


diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2642e1ef128f..59b59d609976 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3517,7 +3517,8 @@ static int ext4_map_blocks_atomic_write_slow(handle_t *handle,
  * underlying short holes/unwritten extents within the requested range.
  */
 static int ext4_map_blocks_atomic_write(handle_t *handle, struct inode *inode,
-                               struct ext4_map_blocks *map, int m_flags)
+                               struct ext4_map_blocks *map, int m_flags,
+                               bool *force_commit)
 {
        ext4_lblk_t m_lblk = map->m_lblk;
        unsigned int m_len = map->m_len;
@@ -3537,6 +3538,11 @@ static int ext4_map_blocks_atomic_write(handle_t *handle, struct inode *inode,
        map->m_len = m_len;
        map->m_flags = 0;

+       /*
+        * slow path means we have mixed mapping, that means we will need
+        * to force txn commit.
+        */
+       *force_commit = true;
        return ext4_map_blocks_atomic_write_slow(handle, inode, map);
 out:
        return ret;
@@ -3548,6 +3554,7 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
        handle_t *handle;
        u8 blkbits = inode->i_blkbits;
        int ret, dio_credits, m_flags = 0, retries = 0;
+       bool force_commit = false;

        /*
         * Trim the mapping request to the maximum value that we can map at
@@ -3610,7 +3617,8 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
                m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;

        if (flags & IOMAP_ATOMIC)
-               ret = ext4_map_blocks_atomic_write(handle, inode, map, m_flags);
+               ret = ext4_map_blocks_atomic_write(handle, inode, map, m_flags,
+                                                  &force_commit);
        else
                ret = ext4_map_blocks(handle, inode, map, m_flags);

@@ -3626,6 +3634,9 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
        if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
                goto retry;

+       if (ret > 0 && force_commit)
+               ext4_force_commit(inode->i_sb);
+
        return ret;
 }


-ritesh