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, why do we have to add this here? For FAT, if buffers are still
>> >> dirty, buffers will be flushed via bdev flush?
>> >
>> > The reason why I've put sync_mapping_buffers() here is the following
>> > sequence:
>> >   fd = open("file")
>> >   write(fd)
>> >   close(fd)
>> >     - now data gets written out, dentry & inode can get evicted from memory
>> >   fd = open("file")
>> >   fsync(fd)
>> >     - this should flush all dirty metadata associated with "file" but if we
>> >       didn't call sync_mapping_buffers() during inode eviction we wouldn't
>> >       have a way to do that.
>> >
>> > So in general I think sync_mapping_buffers() call is indeed needed.
>> 
>> Hm, it looks like not new issue, isn't it? Why we have changed now in
>> this series?
>
> It isn't a new issue. But so far inode_lru_isolate() was checking whether
> the metadata buffers list has any dirty buffers and if yes, it skipped the
> inode. So inodes with dirty buffers in this list could reach .evict method
> only for deleted inodes or during unmount and either case makes above
> problem impossible to happen. This is however a layering violation (generic
> inode handling code shouldn't care about details of buffer heads) and as a
> result it makes it difficult to abstract the metadata buffer handling this
> series is doing. And all this for a handful of filesystems which,
> honestly, aren't used in performace critical settings.

I see, I could understand why you wanted to change though. However, (in
my thought, this change has disadvantage by below reason) changing this
behavior because of layering violation is not good way, IMO.

>> It is including trade off write amplification vs reliability (i.e. may
>> not call fsync()), for example. So I think we should not add it easily.
>
> I expect in practice you'll hardly be able to observe the difference as
> inodes usually get quite a while to be reclaimed at which point the dirty
> buffers would be already flushed by background writeback. I don't see how
> this change would lead specifically to "write amplification" - that would
> mean frequent redirtying of the same metadata buffer of an inode
> interleaved with frequent reclaims of the inode and I don't see how that
> would happen in a realistic setting.
>
> If someone comes with a realistic workload which would suffer significant
> regression from this change, then of course we should address it. I have
> plans for adding an interface for filesystems to expose the information
> that inode has some pending dirty metadata and a way to flush them from
> flush worker because that is a common need a lot of filesystems has and
> doing the flushing from .evict isn't always doable due to locking
> constraints.

I think it would happen with normal operation, for example, copy many
files more than total memory. I think this would be much common than
write=>close=>open=>fsync in your example.  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.

> I'm still thinking about details but this has to be a properly
> abstracted interface all filesystems can use and not a special hack for a
> handful of old filesystems.

Sounds great. How about we delay this behavior change until this
interface?

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