Re: [PATCH -next] ext4: Fix symlink file size not match to file content

[Zhang Yi]

On 2022/3/21 23:11, Jan Kara wrote:
> Hello Yi!
> 
> On Mon 21-03-22 22:38:49, Zhang Yi wrote:
>> On 2022/3/21 19:37, Jan Kara wrote:
>>> On Mon 21-03-22 19:34:08, Ye Bin wrote:
>>>> We got issue as follows:
>>>> [home]# fsck.ext4  -fn  ram0yb
>>>> e2fsck 1.45.6 (20-Mar-2020)
>>>> Pass 1: Checking inodes, blocks, and sizes
>>>> Pass 2: Checking directory structure
>>>> Symlink /p3/d14/d1a/l3d (inode #3494) is invalid.
>>>> Clear? no
>>>> Entry 'l3d' in /p3/d14/d1a (3383) has an incorrect filetype (was 7, should be 0).
>>>> Fix? no
>>>>
>>>> As symlink file size not match to file content. If symlink data block
>>>> writback failed, will call ext4_finish_bio to end io. In this path don't
>>>> mark buffer error. When umount do checkpoint can't detect buffer error,
>>>> then will cleanup jounral. Actually, correct data maybe in journal area.
>>>> To solve this issue, mark buffer error when detect bio error in
>>>> ext4_finish_bio.
>>>
>>> Thanks for the patch! Let me rephrase the text a bit:
>>>
>>> As the symlink file size does not match the file content. If the writeback
>>> of the symlink data block failed, ext4_finish_bio() handles the end of IO.
>>> However this function fails to mark the buffer with BH_write_io_error and
>>> so when unmount does journal checkpoint it cannot detect the writeback
>>> error and will cleanup the journal. Thus we've lost the correct data in the
>>> journal area. To solve this issue, mark the buffer as BH_write_io_error in
>>> ext4_finish_bio().
>>>
>>
>> Thinking about this issue in depth, the symlink data block is one kind of
>> metadata, but the page mapping of such block is belongs to the ext4 inode,
>> it's not coordinate to other metadata blocks, e.g. directory block and extents
>> block. This is why we have already fix the same issue of other metadata blocks
>> in commit fcf37549ae19e9 "jbd2: ensure abort the journal if detect IO error
>> when writing original buffer back" but missing the case of symlink data block.
>> So, after Ye Bin's fix, I think it's worth to unify the symlink data block
>> mapping to bdev, any suggestions?
> 
> Well, symlink with external block is essentially a case of data=journal
> data block. So even if we would handle symlinks, we would still need to
> deal with other inodes with journalled data. Also we need to keep the> symlink contents in the page cache to make it simple for generic VFS code
> handling symlinks. So I don't see how we could substantially unify
> things...
> 

Yeah, this fix is still needed for other regular file's journalled data when we
mounted filesystem with data=jouranl mode. But if we just consider whether if we
could unify the journal mode of ext4's metadata blocks, it seems that using
data=journal mode for symlink's external data block is also complicated and
confused in the creating procedure. Instead, if we use ext4_bread(), it make
things clear, and it seems also has no side effect of reading symlinks. I write
a RFC patch to do this, please take a look at my latest mail "[RFC PATCH] ext4:
convert symlink external data block mapping to bdev".

Thanks,
Yi.