On 2021/8/17 1:14, Jan Kara wrote: > On Mon 16-08-21 22:29:01, Zhang Yi wrote: >> On 2021/8/13 21:44, Jan Kara wrote: >>> On Tue 10-08-21 22:27:22, Zhang Yi wrote: >>>> In ext4_get_inode_loc(), we may skip IO and get an zero && uptodate >>>> inode buffer when the inode monopolize an inode block for performance >>>> reason. For most cases, ext4_mark_iloc_dirty() will fill the inode >>>> buffer to make it fine, but we could miss this call if something bad >>>> happened. Finally, __ext4_get_inode_loc_noinmem() may probably get an >>>> empty inode buffer and trigger ext4 error. >>>> >>>> For example, if we remove a nonexistent xattr on inode A, >>>> ext4_xattr_set_handle() will return ENODATA before invoking >>>> ext4_mark_iloc_dirty(), it will left an uptodate but zero buffer. We >>>> will get checksum error message in ext4_iget() when getting inode again. >>>> >>>> EXT4-fs error (device sda): ext4_lookup:1784: inode #131074: comm cat: iget: checksum invalid >>>> >>>> Even worse, if we allocate another inode B at the same inode block, it >>>> will corrupt the inode A on disk when write back inode B. >>>> >>>> So this patch clear uptodate flag and mark buffer new if we get an empty >>>> buffer, clear it after we fill inode data or making read IO. >>>> >>>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx> >>> >>> Thanks for the fix! Really good catch! The patch looks correct but >>> honestly, I'm not very happy about the special buffer_new handling. It >>> looks correct but I'm a bit uneasy that e.g. the block device code can >>> access this buffer and manipulate its state. Cannot we instead e.g. check >>> whether the buffer is uptodate in ext4_mark_iloc_dirty(), if not, lock it, >>> if still not uptodate, zero it, mark as uptodate, unlock it and then call >>> ext4_do_update_inode()? That would seem like a bit more foolproof solution >>> to me. Basically the fact that the buffer is not uptodate in >>> ext4_mark_iloc_dirty() would mean that nobody else is past >>> __ext4_get_inode_loc() for another inode in that buffer and so zeroing is >>> safe. >>> >> >> Thanks for your suggestion! I understand what you're concerned and your >> approach looks fine except mark buffer uptodate just behind zero buffer >> in ext4_mark_iloc_dirty(). Because I think (1) if ext4_do_update_inode() >> return error before filling the inode, it will still left an uptodate >> but zero buffer, and it's not easy to handle the error path. (2) it is >> still not conform the semantic of buffer uptodate because it it not >> contain an uptodate inode information. How about move mark as uptodate >> into ext4_do_update_inode(), something like that（not tested）？ > > OK, but this way could loading of buffer from the disk race with > ext4_do_update_inode() and overwrite its updates? You have to have buffer > uptodate before you start modifying it or you have to keep the buffer > locked all the time while you are updating it to avoid such races. Indeed. > > Luckily the only place where ext4_do_update_inode() can fail before copying > data to the buffer is due to ext4_inode_blocks_set() which should never > happen because we set s_maxsize so that i_blocks cannot overflow. So maybe > we can just get rid of that case and keep the uptodate setting with the > zeroing? > It's fine, Let's fix it this way now.(But I guess it's fragile because we have to prevent modify ext4_do_update_inode() return before filling data into inode buffer cautiously in the future.) BTW, could we also add a patch to just remove the ext4_has_feature_huge_file() check in ext4_inode_blocks_set() or move it to ext4_mark_iloc_dirty() before ext4_mark_iloc_dirty()? Or else we may get confused and have to add comments to explain it. Thanks, Yi.
