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
