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

[Baokun Li]

On 2024/11/27 14:01, Ojaswin Mujoo wrote:
> On Tue, Nov 26, 2024 at 10:49:14PM +0800, Baokun Li wrote:
> > On 2024/11/21 20:38, Ojaswin Mujoo wrote:
> > > 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>
> > > ---
> > >    fs/ext4/super.c | 17 +++++++++++++++++
> > >    1 file changed, 17 insertions(+)
> > >
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index 16a4ce704460..f7437a592359 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
> > >    {
> > >    	int ret, err;
> > >    	handle_t *handle;
> > > +	bool freeze_protected = false;
> > > +
> > > +	/*
> > > +	 * Trying to sb_start_intwrite() in a running transaction
> > > +	 * can result in a deadlock. Further, running transactions
> > > +	 * are already protected from freezing.
> > > +	 */
> > > +	if (!ext4_journal_current_handle()) {
> > > +		sb_start_intwrite(dquot->dq_sb);
> > > +		freeze_protected = true;
> > > +	}
> > >    	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
> > >    				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
> > >    	if (IS_ERR(handle)) {
> > >    		/* Release dquot anyway to avoid endless cycle in dqput() */
> > >    		dquot_release(dquot);
> > > +		if (freeze_protected)
> > > +			sb_end_intwrite(dquot->dq_sb);
> > >    		return PTR_ERR(handle);
> > >    	}
> > >    	ret = dquot_release(dquot);
> > > @@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
> > The `git am` command looks for the following context code from line 6903
> > to apply the changes. But there are many functions in fs/ext4/super.c that
> > have similar code, such as ext4_write_dquot() and ext4_acquire_dquot().
> Oh that's strange, shouldn't it match the complete line like:
A rough look at the `git am` source code looks like it only focuses on
line numbers between two ‘@@’.

am_run
 run_apply
  apply_all_patches
   apply_patch
    parse_chunk
     find_header
      parse_fragment_header
    check_patch_list
     check_patch
      apply_data
       load_preimage
       apply_fragments
        apply_one_fragment
         find_pos
          match_fragment
> > > @@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
> That should only have one occurence around line 6903? Or does it try to
> fuzzy match which ends up matching ext4_write_dquot etc?
In find_pos(), start from line 6903, compare the hash value of each line
of code line by line in forward direction, if it can't match, then match
the hash value of each line of code line by line in reverse direction from
line 6903. Fuzzy matching is used if some whitespace characters should be
ignored.


Regards,
Baokun