Re: [PATCH RFC 8/9] fat: sync the inode in fat_free_eofblocks()

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Jan Kara <jack@suse.cz> writes:

> On Thu 29-10-26 14:28:23, Christian Brauner wrote:
>> fat_evict_inode() calls fat_free_eofblocks() for a linked inode to give
>> back the clusters that were fallocated past the initialized size. For an
>> empty file that also resets the start cluster in the directory entry. It
>> truncates the clusters and writes the inode with:
>> 
>>   sync_inode_metadata(inode, inode_needs_sync(inode));
>> 
>> Unless the inode is IS_SYNC that is WB_SYNC_NONE writeback. It copies
>> the inode into the directory entry buffer, marks the buffer dirty and
>> sets I_METADATA_WRITEBACK. Then the inode goes away and with it the only
>> thing that would have told a later fsync() that the directory entry
>> still has to be written.
>> 
>> FAT doesn't have that problem. Its buffers are tracked on the FAT inode
>   ^^ exFAT??
>
>> and fat_file_fsync() writes them out on every fsync() of any file on the
>> filesystem.
>> 
>> So after the next fsync() the clusters are free on disk while the
>> directory entry of the empty file still points at them. If another file
>> reuses them and the machine crashes before the directory entry makes it
>> to disk, the two files share clusters. That's the corruption the comment
>> in fat_free_eofblocks() is about.
>
> Correct, but that does not violate anything AFAICT. fsync(2) promises data
> is reachable (which it is), not that the fs metadata is all
> consistent.
>
> For non-journaling filesystem you need fsck for such guarantee. And I know
> I'm a bit obtuse here but the change results in a synchronous buffer write
> on each eviction of inode that has preallocated blocks which will be a
> noticeable performance regression for some usecases. Probably Ogawa should
> decide whether this change is acceptable or whether we play fast and
> loose... Ogawa?

In my understand, the result of this change only affect to crash case
(FAT has no guarantee about it with non-sync mode). And this doesn't
lost data actually - lost some free blocks (maybe fsck would be
recoverable).

So my preference is no force sync.

Thanks.

>> So wait for the directory entry to reach the disk before the inode goes
>> away.
>> 
>> Reproducer. The image file shows what reached the disk. The flusher is
>> turned off so that it doesn't write the directory entry before fsck.fat
>> looks at the image:
>> 
>>   img=/tmp/fat.img
>>   truncate -s 64M $img
>>   mkfs.vfat $img > /dev/null
>>   dev=$(losetup -f --show $img)
>>   mount -t vfat $dev /mnt
>>   echo 0 > /proc/sys/vm/dirty_writeback_centisecs
>>   xfs_io -f -c 'falloc -k 0 1m' -c fsync /mnt/a
>>   echo 3 > /proc/sys/vm/drop_caches
>>   echo 3 > /proc/sys/vm/drop_caches
>>   xfs_io -c fsync /mnt/a
>>   fsck.fat -n $img | grep -A1 '^/a'
>>   umount /mnt
>>   losetup -d $dev
>> 
>> On v7.3-17318-g65ba8f34ec88 in a KASAN and lockdep vng guest this
>> prints:
>> 
>>   /a
>>     Contains a free cluster (3). Assuming EOF.
>> 
>> With this patch fsck.fat finds nothing wrong with /a.
>> 
>> The stable tag is narrower than the Fixes tag. Before v7.3
>> fat_free_eofblocks() called __fat_write_inode() directly and the fix
>> there is to pass 1 for wait.
>> 
>> Fixes: b13bb33eacb7 ("fat: add fat_fallocate operation")
>> Cc: stable@vger.kernel.org # v7.3
>> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
>> ---
>>  fs/fat/inode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
>> index d85c8fbdcca4..fc571ee2b174 100644
>> --- a/fs/fat/inode.c
>> +++ b/fs/fat/inode.c
>> @@ -675,7 +675,7 @@ static void fat_free_eofblocks(struct inode *inode)
>>  		 * any corruption on the next access to the cluster
>>  		 * chain for the file.
>>  		 */
>> -		err = sync_inode_metadata(inode, inode_needs_sync(inode));
>> +		err = sync_inode_metadata(inode, 1);
>>  		if (err) {
>>  			fat_msg(inode->i_sb, KERN_WARNING, "Failed to "
>>  					"update on disk inode for unused "
>> 
>> -- 
>> 2.53.0
>> 

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>