Re: [PATCH v2 2/2] ext4: protect ext4_release_dquot against freezing

[Jan Kara]

On Thu 28-11-24 10:28:58, Ritesh Harjani wrote:
> Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx> writes:
> 
> > Protect ext4_release_dquot against freezing so that we
> > don't try to start a transaction when FS is frozen, leading
> > to warnings.
> >
> > Further, avoid taking the freeze protection if a transaction
> > is already running so that we don't need end up in a deadlock
> > as described in
> >
> >   46e294efc355 ext4: fix deadlock with fs freezing and EA inodes
> >
> > Suggested-by: Jan Kara <jack@xxxxxxx>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> 
> Sorry for being late on this. Ideally, shouldn't it be the
> responsibility of higher level FS (ext4) to make sure that
> FS never freezes while there is pending work for releasing dquot
> structures and that it should also prevent any context where such dquot 
> structures gets added for release/delayed release. 
> 
> e.g. this is what FS takes care during freeze path i.e.
>   freeze_super() -> sync_fs -> ext4_sync_fs()-> dquot_writeback_dquots() -> flush_delayed_work() (now fixed)
> 
> Now coming to iput() case which Jan mentioned [1] which could still
> be called after FS have frozen. As I see we have a protection from FS
> freeze in the ext4_evict_path() right? So ideally we should never see

We don't if we go through:

ext4_evict_inode()
  if (inode->i_nlink) {
    truncate_inode_pages_final(&inode->i_data);
    goto no_delete;
  }
no_delete:
  ext4_clear_inode(inode)
    ...
    dquot_drop()

> dquot_drop() w/o fs freeze protection. And say, if the FS freezing immediately
> happened after we scheduled this delayed work (but before the work gets
> scheduled), then that will be taken care in the freeze_super() chain,
> where we will flush all the delayed work no? - which is what Patch-1 is
> fixing.
> 
> (There still might be an error handling path in ext4_evict_inode() ->
> ext4_clear_inode() which we don't freeze protect. I still need to take a
> closer look at that though).

It isn't error handling. It is a standard inode eviction path if the inode
isn't being deleted.

> So.. isn't this patch trying to hide the problem where FS failed to
> freeze protect some code path?

Well, it is kind of self-inflicted damage of ext4_dquot_release() because
it starts a transaction even if there will be nothing to do. We could add
checks to ext4_dquot_release() to start a transaction only if dquot
structure will need to be deleted but that's a layering violation because
it would have to make assumptions about how quota format code is going to
behave.

							Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR