[PATCH] vfs: do not try to evict inode when super is frozen

[PATCH] vfs: do not try to evict inode when super is frozen

[Jaegeuk Kim]

Otherwise, we will get a deadlock.

[freeze test]                         shrinkder
freeze_super
 - pwercpu_down_write(SB_FREEZE_FS)
                                       - super_cache_scan
                                         - down_read(&sb->s_umount)
                                           - prune_icache_sb
                                            - dispose_list
                                             - evict
                                              - f2fs_evict_inode
thaw_super
 - down_write(&sb->s_umount);
                                              - __percpu_down_read(SB_FREEZE_FS)

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index 7af820ba5ad5..f7303d91f874 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -79,6 +79,12 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 	if (!trylock_super(sb))
 		return SHRINK_STOP;
 
+	/* This prevents inode eviction that requires SB_FREEZE_FS. */
+	if (sb->s_writers.frozen == SB_FREEZE_FS) {
+		up_read(&sb->s_umount);
+		return SHRINK_STOP;
+	}
+
 	if (sb->s_op->nr_cached_objects)
 		fs_objects = sb->s_op->nr_cached_objects(sb, sc);
 
-- 
2.35.1.616.g0bdcbb4464-goog

Re: [PATCH] vfs: do not try to evict inode when super is frozen

[Dave Chinner]

On Thu, Mar 03, 2022 at 06:21:04PM -0800, Jaegeuk Kim wrote:
> Otherwise, we will get a deadlock.

NACK.

We have to be able to evict clean inodes from memory on frozen
inodes because we can still instantiate inodes while the filesytem
is frozen. e.g. there's a find running when the filesystem is
frozen. What happens if we can't evict clean cached inodes from
memory when we run out of memory trying to instantiate new inodes?

> 
> [freeze test]                         shrinkder
> freeze_super
>  - pwercpu_down_write(SB_FREEZE_FS)
>                                        - super_cache_scan
>                                          - down_read(&sb->s_umount)
>                                            - prune_icache_sb
>                                             - dispose_list
>                                              - evict
>                                               - f2fs_evict_inode
> thaw_super
>  - down_write(&sb->s_umount);
>                                               - __percpu_down_read(SB_FREEZE_FS)

That seems like a f2fs bug, not a generic problem.

Filesystems already have to handle stuff like this if an unlinked
file is closed while the fs is frozen - we have to handle inode
eviction needing to modify the file, and different filesystems
handle this differently. Most filesystems simply block in
->evict_inode in this case, but this never occurs from the shrinker
context.

IOWs, the shrinker should never be evicting inodes that require the
filesystem to immediately block on frozen filesystems. If you have
such inodes in cache at the time the filesystem is frozen, then they
should be purged from the cache as part of the freeze process so the
shrinker won't ever find inodes that it could deadlock on.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] vfs: do not try to evict inode when super is frozen

[Jaegeuk Kim]

On 03/04, Dave Chinner wrote:
> On Thu, Mar 03, 2022 at 06:21:04PM -0800, Jaegeuk Kim wrote:
> > Otherwise, we will get a deadlock.
> 
> NACK.
> 
> We have to be able to evict clean inodes from memory on frozen
> inodes because we can still instantiate inodes while the filesytem
> is frozen. e.g. there's a find running when the filesystem is
> frozen. What happens if we can't evict clean cached inodes from
> memory when we run out of memory trying to instantiate new inodes?

Ok, that makes sense.

> 
> > 
> > [freeze test]                         shrinkder
> > freeze_super
> >  - pwercpu_down_write(SB_FREEZE_FS)
> >                                        - super_cache_scan
> >                                          - down_read(&sb->s_umount)
> >                                            - prune_icache_sb
> >                                             - dispose_list
> >                                              - evict
> >                                               - f2fs_evict_inode
> > thaw_super
> >  - down_write(&sb->s_umount);
> >                                               - __percpu_down_read(SB_FREEZE_FS)
> 
> That seems like a f2fs bug, not a generic problem.
> 
> Filesystems already have to handle stuff like this if an unlinked
> file is closed while the fs is frozen - we have to handle inode
> eviction needing to modify the file, and different filesystems
> handle this differently. Most filesystems simply block in
> ->evict_inode in this case, but this never occurs from the shrinker
> context.
> 
> IOWs, the shrinker should never be evicting inodes that require the
> filesystem to immediately block on frozen filesystems. If you have
> such inodes in cache at the time the filesystem is frozen, then they
> should be purged from the cache as part of the freeze process so the
> shrinker won't ever find inodes that it could deadlock on.

If so, is this a bug in drop_caches_sysctl_handler? Or, I shouldn't have
used "echo 3 > sysfs/drop_caches" with freezefs in xfstests?

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] vfs: do not try to evict inode when super is frozen

[Jaegeuk Kim]

On 03/03, Jaegeuk Kim wrote:
> On 03/04, Dave Chinner wrote:
> > On Thu, Mar 03, 2022 at 06:21:04PM -0800, Jaegeuk Kim wrote:
> > > Otherwise, we will get a deadlock.
> > 
> > NACK.
> > 
> > We have to be able to evict clean inodes from memory on frozen
> > inodes because we can still instantiate inodes while the filesytem
> > is frozen. e.g. there's a find running when the filesystem is
> > frozen. What happens if we can't evict clean cached inodes from
> > memory when we run out of memory trying to instantiate new inodes?
> 
> Ok, that makes sense.
> 
> > 
> > > 
> > > [freeze test]                         shrinkder
> > > freeze_super
> > >  - pwercpu_down_write(SB_FREEZE_FS)
> > >                                        - super_cache_scan
> > >                                          - down_read(&sb->s_umount)
> > >                                            - prune_icache_sb
> > >                                             - dispose_list
> > >                                              - evict
> > >                                               - f2fs_evict_inode
> > > thaw_super
> > >  - down_write(&sb->s_umount);
> > >                                               - __percpu_down_read(SB_FREEZE_FS)
> > 
> > That seems like a f2fs bug, not a generic problem.
> > 
> > Filesystems already have to handle stuff like this if an unlinked
> > file is closed while the fs is frozen - we have to handle inode
> > eviction needing to modify the file, and different filesystems
> > handle this differently. Most filesystems simply block in
> > ->evict_inode in this case, but this never occurs from the shrinker
> > context.
> > 
> > IOWs, the shrinker should never be evicting inodes that require the
> > filesystem to immediately block on frozen filesystems. If you have
> > such inodes in cache at the time the filesystem is frozen, then they
> > should be purged from the cache as part of the freeze process so the
> > shrinker won't ever find inodes that it could deadlock on.
> 
> If so, is this a bug in drop_caches_sysctl_handler? Or, I shouldn't have
> used "echo 3 > sysfs/drop_caches" with freezefs in xfstests?

My bad. I totally misunderstood. I'm testing a patch to call evict_inodes()
in f2fs_freeze(). Thank you for the comment. :)

> 
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com

Re: [PATCH] vfs: do not try to evict inode when super is frozen

[Dave Chinner]

On Thu, Mar 03, 2022 at 09:14:50PM -0800, Jaegeuk Kim wrote:
> On 03/04, Dave Chinner wrote:
> > On Thu, Mar 03, 2022 at 06:21:04PM -0800, Jaegeuk Kim wrote:
> > > Otherwise, we will get a deadlock.
> > 
> > NACK.
> > 
> > We have to be able to evict clean inodes from memory on frozen
> > inodes because we can still instantiate inodes while the filesytem
> > is frozen. e.g. there's a find running when the filesystem is
> > frozen. What happens if we can't evict clean cached inodes from
> > memory when we run out of memory trying to instantiate new inodes?
> 
> Ok, that makes sense.
> 
> > 
> > > 
> > > [freeze test]                         shrinkder
> > > freeze_super
> > >  - pwercpu_down_write(SB_FREEZE_FS)
> > >                                        - super_cache_scan
> > >                                          - down_read(&sb->s_umount)
> > >                                            - prune_icache_sb
> > >                                             - dispose_list
> > >                                              - evict
> > >                                               - f2fs_evict_inode
> > > thaw_super
> > >  - down_write(&sb->s_umount);
> > >                                               - __percpu_down_read(SB_FREEZE_FS)
> > 
> > That seems like a f2fs bug, not a generic problem.
> > 
> > Filesystems already have to handle stuff like this if an unlinked
> > file is closed while the fs is frozen - we have to handle inode
> > eviction needing to modify the file, and different filesystems
> > handle this differently. Most filesystems simply block in
> > ->evict_inode in this case, but this never occurs from the shrinker
> > context.
> > 
> > IOWs, the shrinker should never be evicting inodes that require the
> > filesystem to immediately block on frozen filesystems. If you have
> > such inodes in cache at the time the filesystem is frozen, then they
> > should be purged from the cache as part of the freeze process so the
> > shrinker won't ever find inodes that it could deadlock on.
> 
> If so, is this a bug in drop_caches_sysctl_handler?

IMO, no.

> Or, I shouldn't have
> used "echo 3 > sysfs/drop_caches" with freezefs in xfstests?

That should just work.

As I said above - if the filesystem cannot process eviction of
certain types of inodes when the filesystem is frozen, it needs to
take steps during the freeze process to ensure they can't be evicted
when the fs is frozen....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com