On 2021/7/16 18:08, Jan Kara wrote:

On Fri 16-07-21 11:56:06, Zhang Yi wrote:

quoted

On 2021/7/15 20:08, Jan Kara wrote:

quoted

On Thu 15-07-21 09:54:51, Zhang Yi wrote:

quoted

Now that the inline_data file write end procedure are falled into the
common write end functions, it is not clear. Factor them out and do
some cleanup. This patch also drop ext4_da_write_inline_data_end()
and switch to use ext4_write_inline_data_end() instead because we also
need to do the same error processing if we failed to write data into
inline entry.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Just two small comments below.

quoted

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 28b666f25ac2..3d227b32b21c 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c

...

quoted

+out:
+	/*
+	 * If we have allocated more blocks and copied less. We will have
+	 * blocks allocated outside inode->i_size, so truncate them.
+	 */
+	if (pos + len > inode->i_size && ext4_can_truncate(inode))
+		ext4_orphan_add(handle, inode);

I don't think we need this error handling here. For inline data we never
allocate any blocks so shorter writes don't need any cleanup.

quoted

-	return copied;
+	ret2 = ext4_journal_stop(handle);
+	if (!ret)
+		ret = ret2;
+	if (pos + len > inode->i_size) {
+		ext4_truncate_failed_write(inode);
+		/*
+		 * If truncate failed early the inode might still be
+		 * on the orphan list; we need to make sure the inode
+		 * is removed from the orphan list in that case.
+		 */
+		if (inode->i_nlink)
+			ext4_orphan_del(NULL, inode);
+	}

And this can go away as well...

Yeah, but if we don't call ext4_truncate_failed_write()->..->
ext4_inline_data_truncate(), it will lead to incorrect larger i_inline_size
and data entry. Although it seems harmless (i_size can prevent read zero
data), I think it's better to restore the data entry(the comments need
change later), or else it will occupy more xattr space. What do you think ?

Good point. I've found this out last time when I was reviewing your patches
and then forgot again. So please leave the code there but fix this
misleading comment:

/*
 * If we have allocated more blocks and copied less. We will have
 * blocks allocated outside inode->i_size, so truncate them.
 */

Something like:

/*
 * If we didn't copy as much data as expected, we need to trim back size of
 * xattr containing inline data.
 */

OK.

Thanks,
Yi.