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

On Mon 30-03-26 20:29:19, OGAWA Hirofumi wrote:
> Jan Kara <[email protected]> writes:
> > On Sun 29-03-26 22:55:09, OGAWA Hirofumi wrote:
> >> Jan Kara <[email protected]> writes:
> >> > There are only very few filesystems using generic metadata buffer head
> >> > tracking and everybody is paying the overhead. When we remove this
> >> > tracking for inode reclaim code .evict will start to see inodes with
> >> > metadata buffers attached so write them out and prune them.
> >> >
> >> > Signed-off-by: Jan Kara <[email protected]>
> >> > ---
> >> >  fs/fat/inode.c | 4 +++-
> >> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> >> > index 3cc5fb01afa1..ce88602b0d57 100644
> >> > --- a/fs/fat/inode.c
> >> > +++ b/fs/fat/inode.c
> >> > @@ -657,8 +657,10 @@ static void fat_evict_inode(struct inode *inode)
> >> >  	if (!inode->i_nlink) {
> >> >  		inode->i_size = 0;
> >> >  		fat_truncate_blocks(inode, 0);
> >> > -	} else
> >> > +	} else {
> >> > +		sync_mapping_buffers(inode->i_mapping);
> >> 
> >> 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.

> 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'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.

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>