Re: [PATCH 15/42] fat: Sync and invalidate metadata buffers from fat_evict_inode()

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

Jan Kara <jack@suse.cz> writes:

>> Hm, metadata block is shared by several inodes. So earlier flush
>> makes fewer chance to combining multiple dirties.
>> 
>> For example,
>>   create dir-A
>>   reclaimed and flushed dir-A
>>   add new entries to dir-A
>>   lost chance to combining re-dirty of dir-A
>
> Yes, but for this to be possible you would have to:
> 1) stop using dir-A between dir create & file creates
> 2) create enough memory pressure to cycle the dentry of dir-A through the
> LRU and reclaim it
> 3) continue memory pressure to cycle the inode for dir-A through the LRU
> and reclaim it
>
> So the amount of work that already has to happen to trigger flushing of
> a single block is so large that IMHO that flush will be lost in the noise.
>  
>> >> Anyway, with it, reclaimed
>> >> inode metadata will be flushed forcibly and frequently (yeah, may not be
>> >> significant though. but I can't see the benefit for users from this
>> >> change.), and lost to chance combining multiple time of dirty while copy
>> >> many files.
>> >
>> > The benefit for users is 24 bytes saved for the majority of inodes that are
>> > there in the system - all the virtual inodes on sysfs / proc filesystem,
>> > all tmpfs inodes, all XFS inodes, all ext4 inodes when using journal (once I
>> > optimize ext4 code a bit), etc. So actually quite a bit of kernel memory
>> > saved in common configurations.
>> >
>> > Another win is that with metadata buffer head tracking now separated, I can
>> > modify that code (which will require growing the tracking structure) to
>> > properly track buffer head containing the inode and flush it on fsync(2).
>> > Currently there's a race that if flush worker writes out inode before
>> > fsync(2), then fsync(2) does not writeout the buffer containing the inode
>> > at all and thus data is not really persistent. This is actually my initial
>> > motivation for this refactoring since growing inode for everybody to fix
>> > data consistency issues of FAT/ext2/udf isn't popular these days...
>> 
>> Agree, it is good.  I'm only saying about the flushing earlier. To
>> implement it, is the flush earlier really necessary?
>
> Yes, to separate metadata buffer head tracking into a separate structure we
> must remove the handling of buffer head list from generic inode reclaim (as
> the filesystem has no way to provide the separate tracking structure there).
> Of course we could add a filesystem hook to inode reclaim to allow for
> handling of metadata bhs but:
>
> a) I'd rather do that in a way that is usable also for other issues
> filesystems have with inode reclaim as I mentioned in this thread before
>
> b) I don't think it's warranted for FAT etc. at this point as I don't think
> the possible overhead of metadata bh flushing on inode reclaim will be a
> problem in practice.
>
> But of course we can reevaluate if my gut feeling is wrong and someone
> comes with a workload which significantly regresses due to these changes.

OK.  I'm still thinking we should go the way to reduce the amplification
for performance and storage lifetime if possible, not increasing.

However discussion looks like enough for us, and looks like we just
voted to different priority.

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