ext4: undo ext4_calc_metadata_amount if we fail to claim space

The function ext4_calc_metadata_amount() has side effects, although it's not obvious from its function name. So if we fail to claim space, regardless of whether we retry to claim the space again, or return an error, we need to undo these side effects. Otherwise we can end up incorrectly calculating the number of metadata blocks needed for the operation, which was responsible for an xfstests failure for test #271 when using an ext2 file system with delalloc enabled. Reported-by: Brian Foster <bfoster@redhat.com> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Cc: stable@vger.kernel.org

ext4: undo ext4_calc_metadata_amount if we fail to claim space
The function ext4_calc_metadata_amount() has side effects, although it's not obvious from its function name. So if we fail to claim space, regardless of whether we retry to claim the space again, or return an error, we need to undo these side effects. Otherwise we can end up incorrectly calculating the number of metadata blocks needed for the operation, which was responsible for an xfstests failure for test #271 when using an ext2 file system with delalloc enabled. Reported-by: Brian Foster <bfoster@redhat.com> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Cc: stable@vger.kernel.org
Theodore Ts'o
1 parent 97795d2a5b
Showing 1 changed file with 21 additions and 11 deletions Side-by-side Diff
fs/ext4/inode.c
@@ -1182,40 +1182,50 @@
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	unsigned int md_needed;
 	int ret;
+	ext4_lblk_t save_last_lblock;
+	int save_len;
  
 	/*
+	 * We will charge metadata quota at writeout time; this saves
+	 * us from metadata over-estimation, though we may go over by
+	 * a small amount in the end.  Here we just reserve for data.
+	 */
+	ret = dquot_reserve_block(inode, EXT4_C2B(sbi, 1));
+	if (ret)
+		return ret;
+
+	/*
 	 * recalculate the amount of metadata blocks to reserve
 	 * in order to allocate nrblocks
 	 * worse case is one extent per block
 	 */
 repeat:
 	spin_lock(&ei->i_block_reservation_lock);
+	/*
+	 * ext4_calc_metadata_amount() has side effects, which we have
+	 * to be prepared undo if we fail to claim space.
+	 */
+	save_len = ei->i_da_metadata_calc_len;
+	save_last_lblock = ei->i_da_metadata_calc_last_lblock;
 	md_needed = EXT4_NUM_B2C(sbi,
 				 ext4_calc_metadata_amount(inode, lblock));
 	trace_ext4_da_reserve_space(inode, md_needed);
-	spin_unlock(&ei->i_block_reservation_lock);
  
 	/*
-	 * We will charge metadata quota at writeout time; this saves
-	 * us from metadata over-estimation, though we may go over by
-	 * a small amount in the end.  Here we just reserve for data.
-	 */
-	ret = dquot_reserve_block(inode, EXT4_C2B(sbi, 1));
-	if (ret)
-		return ret;
-	/*
 	 * We do still charge estimated metadata to the sb though;
 	 * we cannot afford to run out of free blocks.
 	 */
 	if (ext4_claim_free_clusters(sbi, md_needed + 1, 0)) {
-		dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1));
+		ei->i_da_metadata_calc_len = save_len;
+		ei->i_da_metadata_calc_last_lblock = save_last_lblock;
+		spin_unlock(&ei->i_block_reservation_lock);
 		if (ext4_should_retry_alloc(inode->i_sb, &retries)) {
 			yield();
 			goto repeat;
 		}
+		dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1));
 		return -ENOSPC;
 	}
-	spin_lock(&ei->i_block_reservation_lock);
 	ei->i_reserved_data_blocks++;
 	ei->i_reserved_meta_blocks += md_needed;
 	spin_unlock(&ei->i_block_reservation_lock);
...	...	@@ -1182,40 +1182,50 @@
1182	1182	struct ext4_inode_info *ei = EXT4_I(inode);
1183	1183	unsigned int md_needed;
1184	1184	int ret;
	1185	+ ext4_lblk_t save_last_lblock;
	1186	+ int save_len;
1185	1187
1186	1188	/*
	1189	+ * We will charge metadata quota at writeout time; this saves
	1190	+ * us from metadata over-estimation, though we may go over by
	1191	+ * a small amount in the end. Here we just reserve for data.
	1192	+ */
	1193	+ ret = dquot_reserve_block(inode, EXT4_C2B(sbi, 1));
	1194	+ if (ret)
	1195	+ return ret;
	1196	+
	1197	+ /*
1187	1198	* recalculate the amount of metadata blocks to reserve
1188	1199	* in order to allocate nrblocks
1189	1200	* worse case is one extent per block
1190	1201	*/
1191	1202	repeat:
1192	1203	spin_lock(&ei->i_block_reservation_lock);
	1204	+ /*
	1205	+ * ext4_calc_metadata_amount() has side effects, which we have
	1206	+ * to be prepared undo if we fail to claim space.
	1207	+ */
	1208	+ save_len = ei->i_da_metadata_calc_len;
	1209	+ save_last_lblock = ei->i_da_metadata_calc_last_lblock;
1193	1210	md_needed = EXT4_NUM_B2C(sbi,
1194	1211	ext4_calc_metadata_amount(inode, lblock));
1195	1212	trace_ext4_da_reserve_space(inode, md_needed);
1196		- spin_unlock(&ei->i_block_reservation_lock);
1197	1213
1198	1214	/*
1199		- * We will charge metadata quota at writeout time; this saves
1200		- * us from metadata over-estimation, though we may go over by
1201		- * a small amount in the end. Here we just reserve for data.
1202		- */
1203		- ret = dquot_reserve_block(inode, EXT4_C2B(sbi, 1));
1204		- if (ret)
1205		- return ret;
1206		- /*
1207	1215	* We do still charge estimated metadata to the sb though;
1208	1216	* we cannot afford to run out of free blocks.
1209	1217	*/
1210	1218	if (ext4_claim_free_clusters(sbi, md_needed + 1, 0)) {
1211		- dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1));
	1219	+ ei->i_da_metadata_calc_len = save_len;
	1220	+ ei->i_da_metadata_calc_last_lblock = save_last_lblock;
	1221	+ spin_unlock(&ei->i_block_reservation_lock);
1212	1222	if (ext4_should_retry_alloc(inode->i_sb, &retries)) {
1213	1223	yield();
1214	1224	goto repeat;
1215	1225	}
	1226	+ dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1));
1216	1227	return -ENOSPC;
1217	1228	}
1218		- spin_lock(&ei->i_block_reservation_lock);
1219	1229	ei->i_reserved_data_blocks++;
1220	1230	ei->i_reserved_meta_blocks += md_needed;
1221	1231	spin_unlock(&ei->i_block_reservation_lock);