ext4: Add multi-fsblock atomic write support using bigalloc

This is still an early preview (RFC v2) of multi-fsblock atomic write. Since the core design of the feature looks ready, wanted to post this for some early feedback. We will break this into more smaller and meaningful patches in later revision. However to simplify the review of the core design changes, this version is limited to just two patches. Individual patches might have more details in the commit msg.

Note: This overall needs more careful review (other than the core design) which I will be doing in parallel. However it would be helpful if one can provide any feedback on the core design changes. Specially around ext4_iomap_alloc() changes, ->end_io() changes and a new get block flag EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS.

v1 -> v2:
==========
1. Handled review comments from Ojaswin to optimize the ext4_map_block() calls in ext4_iomap_alloc().
2. Fixed the journal credits calculation for both:
- during block allocation in ext4_iomap_alloc()
- during dio completion in ->end_io callback.
Earlier we were starting multiple txns in ->end_io callback for unwritten to written conversion. But since in case of atomic writes, we want a single jbd2 txn, hence made the necessary changes there.

TODOs:
======
1. although this survived quick tests and some custom fio tests for atomic write, but I still think we need to add more tests for the corner cases. Will work on adding such corner test cases to xfstests.
2. Many fsx related tests in quick group were failing with bigalloc. But those were failing even without these patches. Need further investigation.
3. Finish more thorough testing of the series.
4. Refactoring + Cleanups and a more careful review of the overall patch series before sending v3.

Ritesh Harjani (IBM) (2):
ext4: Add multi-fsblock atomic write support with bigalloc
ext4: Add support for EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS

 fs/ext4/ext4.h    |   9 ++
 fs/ext4/extents.c |  69 ++++++++++++++
 fs/ext4/file.c    |   7 +-
 fs/ext4/inode.c   | 231 ++++++++++++++++++++++++++++++++++++++++++----
 fs/ext4/super.c   |   8 +-
 5 files changed, 302 insertions(+), 22 deletions(-)

--
2.49.0

EXT4 supports bigalloc feature which allows the FS to work in size of clusters (group of blocks) rather than individual blocks. This patch adds atomic write support for bigalloc so that systems with bs = ps can also create FS using -

    mkfs.ext4 -F -O bigalloc -b 4096 -C 16384 <dev>

With bigalloc ext4 can support multi-fsblock atomic writes. We will have to adjust ext4's atomic write unit max value to cluster size. This can then support atomic write of size anywhere between [blocksize, clustersize].

We first query the underlying region of the requested range by calling ext4_map_blocks() call. Here are the various cases which we then handle for block allocation depending upon the underlying mapping type:
1. If the underlying region for the entire requested range is a mapped extent, then we don't call ext4_map_blocks() to allocate anything. We don't need to even start the jbd2 txn in this case.
2. For an append write case, we create a mapped extent.
3. If the underlying region is entirely a hole, then we create an unwritten extent for the requested range.
4. If the underlying region is a large unwritten extent, then we split the extent into 2 unwritten extent of required size.
5. If the underlying region has any type of mixed mapping, then we call ext4_map_blocks() in a loop to zero out the unwritten and the hole regions within the requested range. This then provide a single mapped extent type mapping for the requested range.

Note: We invoke ext4_map_blocks() in a loop with the EXT4_GET_BLOCKS_ZERO flag only when the underlying extent mapping of the requested range is not entirely a hole, an unwritten extent, or a fully mapped extent. That is, if the underlying region contains a mix of hole(s), unwritten extent(s), and mapped extent(s), we use this loop to ensure that all the short mappings are zeroed out. This guarantees that the entire requested range becomes a single, uniformly mapped extent. It is ok to do so because we know this is being done on a bigalloc enabled filesystem where the block bitmap represents the entire cluster unit.

---
 fs/ext4/ext4.h    |   3 +
 fs/ext4/extents.c |  64 +++++++++++++++++++++
 fs/ext4/file.c    |   7 ++-
 fs/ext4/inode.c   | 142 +++++++++++++++++++++++++++++++++++++++++++---
 fs/ext4/super.c   |   8 ++-
 5 files changed, 213 insertions(+), 11 deletions(-)

index 5a20e9cd7184..589d51389327 100644
@@ -3039,6 +3039,7 @@ extern void ext4_set_aops(struct inode *inode);
 extern int ext4_writepage_trans_blocks(struct inode *);
 extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
 extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
+extern int ext4_meta_trans_blocks(struct inode *, int nrblocks, int extents);
 extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
 			     loff_t lstart, loff_t lend);
 extern vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf);

@@ -3710,6 +3711,8 @@ extern long ext4_fallocate(struct file *file, int mode, loff_t offset,
 			  loff_t len);
 extern int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
 					  loff_t offset, ssize_t len);
+extern int ext4_convert_unwritten_extents_atomic(handle_t *handle,
+			struct inode *inode, loff_t offset, ssize_t len);
 extern int ext4_convert_unwritten_io_end_vec(handle_t *handle,
 					     ext4_io_end_t *io_end);
 extern int ext4_map_blocks(handle_t *handle, struct inode *inode,

index beb078ee4811..959328072c15 100644
@@ -377,7 +377,12 @@ static int ext4_dio_write_end_io(struct kiocb *iocb, ssize_t size,
 	loff_t pos = iocb->ki_pos;
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	if (!error && size && flags & IOMAP_DIO_UNWRITTEN)
+
+	if (!error && size && (flags & IOMAP_DIO_UNWRITTEN) &&
+			(iocb->ki_flags & IOCB_ATOMIC))
+		error = ext4_convert_unwritten_extents_atomic(NULL, inode, pos,
+							      size);
+	else if (!error && size && flags & IOMAP_DIO_UNWRITTEN)
 		error = ext4_convert_unwritten_extents(NULL, inode, pos, size);
 	if (error)
 		return error;

index 181934499624..5c319d08446f 100644
@@ -4442,12 +4442,13 @@ static int ext4_handle_clustersize(struct super_block *sb)
 /*
  * ext4_atomic_write_init: Initializes filesystem min & max atomic write units.
  * @sb: super block
- * TODO: Later add support for bigalloc
  */
 static void ext4_atomic_write_init(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct block_device *bdev = sb->s_bdev;
+	unsigned int blkbits = sb->s_blocksize_bits;
+	unsigned int clustersize = sb->s_blocksize;
 
 	if (!bdev_can_atomic_write(bdev))
 		return;

@@ -4455,9 +4456,12 @@ static void ext4_atomic_write_init(struct super_block *sb)
 	if (!ext4_has_feature_extents(sb))
 		return;
 
+	if (ext4_has_feature_bigalloc(sb))
+		clustersize = 1U << (sbi->s_cluster_bits + blkbits);
+
 	sbi->s_awu_min = max(sb->s_blocksize,
 			      bdev_atomic_write_unit_min_bytes(bdev));
-	sbi->s_awu_max = min(sb->s_blocksize,
+	sbi->s_awu_max = min(clustersize,
 			      bdev_atomic_write_unit_max_bytes(bdev));
 	if (sbi->s_awu_min && sbi->s_awu_max &&
 	    sbi->s_awu_min <= sbi->s_awu_max) {

--
2.49.0

There can be a case where there are contiguous extents on the adjacent leaf nodes of on-disk extent trees. So when someone tries to write to this contiguous range, ext4_map_blocks() call will split by returning 1 extent at a time if this is not already cached in extent_status tree cache (where if these extents when cached can get merged since they are contiguous).

This is fine for a normal write however in case of atomic writes, it can't afford to break the write into two. Now this is also something that will only happen in the slow write case where we call ext4_map_blocks() for each of these extents spread across different leaf nodes. However, there is no guarantee that these extent status cache cannot be reclaimed before the last call to ext4_map_blocks() in ext4_map_blocks_atomic_write_slow().

Hence this patch adds support of EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS. This flag checks if the requested range can be fully found in extent status cache and return. If not, it looks up in on-disk extent tree via ext4_map_query_blocks(). If the found extent is the last entry in the leaf node, then it goes and queries the next lblk to see if there is an adjacent contiguous extent in the adjacent leaf node of the on-disk extent tree.

Even though there can be a case where there are multiple adjacent extent entries spread across multiple leaf nodes. But we only read an adjacent leaf block i.e. in total of 2 extent entries spread across 2 leaf nodes. The reason for this is that we are mostly only going to support atomic writes with upto 64KB or maybe max upto 1MB of atomic write support.

---
 fs/ext4/ext4.h    |  6 ++++
 fs/ext4/extents.c |  9 +++--
 fs/ext4/inode.c   | 91 ++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 92 insertions(+), 14 deletions(-)

index 589d51389327..38f75d33d67f 100644
@@ -256,6 +256,7 @@ struct ext4_allocation_request {
 #define EXT4_MAP_UNWRITTEN	BIT(BH_Unwritten)
 #define EXT4_MAP_BOUNDARY	BIT(BH_Boundary)
 #define EXT4_MAP_DELAYED	BIT(BH_Delay)
+#define EXT4_MAP_LAST_IN_LEAF	BIT(BH_PrivateStart)
 #define EXT4_MAP_FLAGS		(EXT4_MAP_NEW | EXT4_MAP_MAPPED |\
 				 EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY |\
 				 EXT4_MAP_DELAYED)

@@ -725,6 +726,11 @@ enum {
 #define EXT4_GET_BLOCKS_IO_SUBMIT		0x0400
 	/* Caller is in the atomic contex, find extent if it has been cached */
 #define EXT4_GET_BLOCKS_CACHED_NOWAIT		0x0800
+/*
+ * Atomic write caller needs this to query in the slow path of mixed mapping
+ * case, when a contiguous extent can be split across two adjacent leaf nodes.
+ */
+#define EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS	0x1000
 
 /*
  * The bit position of these flags must not overlap with any of the

index 0e00b78b521c..12fae8d70f46 100644
@@ -4433,6 +4433,10 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	allocated = map->m_len;
 	ext4_ext_show_leaf(inode, path);
 out:
+	if (flags & EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS)
+		if (ex && (ex == EXT_LAST_EXTENT(path[depth].p_hdr)))
+			map->m_flags |= EXT4_MAP_LAST_IN_LEAF;
+
 	ext4_free_ext_path(path);
 
 	trace_ext4_ext_map_blocks_exit(inode, flags, map,

@@ -4788,7 +4792,8 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
 	struct ext4_map_blocks map;
 	unsigned int blkbits = inode->i_blkbits;
 	unsigned int credits = 0;
-	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT;
+	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT |
+			EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS;
 
 	map.m_lblk = offset >> blkbits;
 	max_blocks = EXT4_MAX_BLOCKS(len, offset, blkbits);

@@ -4815,7 +4820,7 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
 		map.m_lblk += ret;
 		map.m_len = (max_blocks -= ret);
 		ret = ext4_map_blocks(handle, inode, &map, flags);
-		if (ret != max_blocks)
+		if (!(map.m_flags & EXT4_MAP_LAST_IN_LEAF) && ret != max_blocks)
 			ext4_warning(inode->i_sb,
 				     "inode #%lu: block %u: len %u: "
 				     "split block mapping found for atomic write,"

2.49.0

I gave this a try and it looks ok, specifically atomic writing mixed mappings.

I'll try to look closer that the implementation details. But I do note that you use blkdev_issue_zeroout() to pre-zero any unwritten range which is being atomically written.

Thanks John for taking a look.

We are in the process of sending v3 (hopefully by tonight) which is an improved version w.r.t error handling, journal credits and few other changes. Although nothing has changed w.r.t the design aspect.

Yes, that is how internally ext4_map_blocks() with EXT4_GET_BLOCKS_CREATE_ZERO will return us the allocated blocks. During block allocation, on mixed mapping range, we ensure that the entire range becomes a contiguous mapped extent before starting any data writes. That means calling ext4_map_blocks() in a loop with EXT4_GET_BLOCKS_CREATE_ZERO, so that it can zero out any unwritten extents in the requested region.
I assume writing over a mixed mapping region is not a performance critical path.

Do you forsee any problems with the approach (since you said "But I do note...")?

-ritesh

It's a little dumb to write zeroes just so you can atomicwrite a block. However, ext4 lacks an out of place write handler, so I don't think there's much else that can be done easily.

--D