[v2] Fix generic/390 failure due to quota release after freeze

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

Posted by Ojaswin Mujoo 1 year, 2 months ago

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@suse.cz>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 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)
 	err = ext4_journal_stop(handle);
 	if (!ret)
 		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
 	return ret;
 }
 
-- 
2.43.5

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

Posted by Ojaswin Mujoo 11 months, 1 week ago

On Thu, Nov 21, 2024 at 06:08:55PM +0530, 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@suse.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Hey Ted,

Just a ping, I think you might have missed this patch. Let me know if
anything else is needed from my side.

Regards,
ojaswin

> ---
>  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)
>  	err = ext4_journal_stop(handle);
>  	if (!ret)
>  		ret = err;
> +
> +	if (freeze_protected)
> +		sb_end_intwrite(dquot->dq_sb);
> +
>  	return ret;
>  }
>  
> -- 
> 2.43.5
>

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

Posted by Theodore Ts'o 11 months, 1 week ago

On Thu, Mar 06, 2025 at 12:00:49PM +0530, Ojaswin Mujoo wrote:
> On Thu, Nov 21, 2024 at 06:08:55PM +0530, 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@suse.cz>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> Hey Ted,
> 
> Just a ping, I think you might have missed this patch. Let me know if
> anything else is needed from my side.

Yes, I did miss this patch; thanks for the reminder. It looks good
and I've added it to my tree.

				- Ted

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

Posted by Ritesh Harjani (IBM) 1 year, 2 months ago

Ojaswin Mujoo <ojaswin@linux.ibm.com> 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@suse.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

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

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

-ritesh

> ---
>  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)
>  	err = ext4_journal_stop(handle);
>  	if (!ret)
>  		ret = err;
> +
> +	if (freeze_protected)
> +		sb_end_intwrite(dquot->dq_sb);
> +
>  	return ret;
>  }
>  
> -- 
> 2.43.5

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

Posted by Jan Kara 1 year, 2 months ago

On Thu 28-11-24 10:28:58, Ritesh Harjani wrote:
> Ojaswin Mujoo <ojaswin@linux.ibm.com> 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@suse.cz>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> 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@suse.com>
SUSE Labs, CR

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

Posted by Baokun Li 1 year, 2 months ago

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@suse.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>   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().

So when the code before ext4_release_dquot() is added, the first matching
context found could be in ext4_write_dquot() or ext4_acquire_dquot().
>   	err = ext4_journal_stop(handle);
>   	if (!ret)
>   		ret = err;
> +
> +	if (freeze_protected)
> +		sb_end_intwrite(dquot->dq_sb);
> +
>   	return ret;
>   }
>   

Thus this is actually a bug in `git am`, which can be avoided by increasing
the number of context lines with `git format-patch -U8 -1`.

Otherwise it looks good. Feel free to add:

Reviewed-by: Baokun Li <libaokun1@huawei.com>

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

Posted by Ojaswin Mujoo 1 year, 2 months ago

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@suse.cz>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >   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:

> > @@ -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?

> 
> So when the code before ext4_release_dquot() is added, the first matching
> context found could be in ext4_write_dquot() or ext4_acquire_dquot().
> >   	err = ext4_journal_stop(handle);
> >   	if (!ret)
> >   		ret = err;
> > +
> > +	if (freeze_protected)
> > +		sb_end_intwrite(dquot->dq_sb);
> > +
> >   	return ret;
> >   }
> 
> Thus this is actually a bug in `git am`, which can be avoided by increasing
> the number of context lines with `git format-patch -U8 -1`.
> 
> Otherwise it looks good. Feel free to add:
> 
> Reviewed-by: Baokun Li <libaokun1@huawei.com>

Thanks Baokun!

Regards,
ojaswin
>

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

Posted by Baokun Li 1 year, 2 months ago

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@suse.cz>
>>> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
>>> ---
>>>    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

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

Posted by Ojaswin Mujoo 1 year, 1 month ago

On Tue, Dec 03, 2024 at 04:29:31PM +0800, Baokun Li wrote:
> 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@suse.cz>
> > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > ---
> > > >    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.

Ahh okay thanks for looking into this Baokun, pretty interesting since
I've never looked at git code :)

Regards,
ojaswin
> 
> 
> Regards,
> Baokun
>

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

Posted by Jan Kara 1 year, 2 months ago

On Thu 21-11-24 18:08:55, 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@suse.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Looks good to me (the 0-day reports seem to be due to wrong merge). Feel
free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  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)
>  	err = ext4_journal_stop(handle);
>  	if (!ret)
>  		ret = err;
> +
> +	if (freeze_protected)
> +		sb_end_intwrite(dquot->dq_sb);
> +
>  	return ret;
>  }
>  
> -- 
> 2.43.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

Posted by Ojaswin Mujoo 1 year, 2 months ago

On Tue, Nov 26, 2024 at 10:04:52AM +0100, Jan Kara wrote:
> On Thu 21-11-24 18:08:55, 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@suse.cz>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> Looks good to me (the 0-day reports seem to be due to wrong merge). Feel
> free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

Thanks Jan, yes it does seem like an incorrect merge.

> 
> > ---
> >  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)
> >  	err = ext4_journal_stop(handle);
> >  	if (!ret)
> >  		ret = err;
> > +
> > +	if (freeze_protected)
> > +		sb_end_intwrite(dquot->dq_sb);
> > +
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.43.5
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

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

Posted by kernel test robot 1 year, 2 months ago

Hi Ojaswin,

kernel test robot noticed the following build errors:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on brauner-vfs/vfs.all jack-fs/for_next linus/master v6.12 next-20241121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ojaswin-Mujoo/quota-flush-quota_release_work-upon-quota-writeback/20241121-204331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20241121123855.645335-3-ojaswin%40linux.ibm.com
patch subject: [PATCH v2 2/2] ext4: protect ext4_release_dquot against freezing
config: arc-randconfig-001-20241122 (https://download.01.org/0day-ci/archive/20241122/202411221326.eoWoxSKf-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241122/202411221326.eoWoxSKf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411221326.eoWoxSKf-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/ext4/super.c: In function 'ext4_acquire_dquot':
>> fs/ext4/super.c:6912:13: error: 'freeze_protected' undeclared (first use in this function); did you mean 'freeze_processes'?
    6912 |         if (freeze_protected)
         |             ^~~~~~~~~~~~~~~~
         |             freeze_processes
   fs/ext4/super.c:6912:13: note: each undeclared identifier is reported only once for each function it appears in


vim +6912 fs/ext4/super.c

  6893	
  6894	static int ext4_acquire_dquot(struct dquot *dquot)
  6895	{
  6896		int ret, err;
  6897		handle_t *handle;
  6898	
  6899		handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
  6900					    EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb));
  6901		if (IS_ERR(handle))
  6902			return PTR_ERR(handle);
  6903		ret = dquot_acquire(dquot);
  6904		if (ret < 0)
  6905			ext4_error_err(dquot->dq_sb, -ret,
  6906				      "Failed to acquire dquot type %d",
  6907				      dquot->dq_id.type);
  6908		err = ext4_journal_stop(handle);
  6909		if (!ret)
  6910			ret = err;
  6911	
> 6912		if (freeze_protected)
  6913			sb_end_intwrite(dquot->dq_sb);
  6914	
  6915		return ret;
  6916	}
  6917	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

Posted by kernel test robot 1 year, 2 months ago

Hi Ojaswin,

kernel test robot noticed the following build errors:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on brauner-vfs/vfs.all jack-fs/for_next linus/master v6.12 next-20241121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ojaswin-Mujoo/quota-flush-quota_release_work-upon-quota-writeback/20241121-204331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20241121123855.645335-3-ojaswin%40linux.ibm.com
patch subject: [PATCH v2 2/2] ext4: protect ext4_release_dquot against freezing
config: i386-buildonly-randconfig-004-20241122 (https://download.01.org/0day-ci/archive/20241122/202411221352.NvzmPEy3-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241122/202411221352.NvzmPEy3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411221352.NvzmPEy3-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/ext4/super.c:27:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/x86/include/asm/cacheflush.h:5:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/ext4/super.c:6912:6: error: use of undeclared identifier 'freeze_protected'; did you mean 'freeze_processes'?
    6912 |         if (freeze_protected)
         |             ^~~~~~~~~~~~~~~~
         |             freeze_processes
   include/linux/freezer.h:46:12: note: 'freeze_processes' declared here
      46 | extern int freeze_processes(void);
         |            ^
   1 warning and 1 error generated.


vim +6912 fs/ext4/super.c

  6893	
  6894	static int ext4_acquire_dquot(struct dquot *dquot)
  6895	{
  6896		int ret, err;
  6897		handle_t *handle;
  6898	
  6899		handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
  6900					    EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb));
  6901		if (IS_ERR(handle))
  6902			return PTR_ERR(handle);
  6903		ret = dquot_acquire(dquot);
  6904		if (ret < 0)
  6905			ext4_error_err(dquot->dq_sb, -ret,
  6906				      "Failed to acquire dquot type %d",
  6907				      dquot->dq_id.type);
  6908		err = ext4_journal_stop(handle);
  6909		if (!ret)
  6910			ret = err;
  6911	
> 6912		if (freeze_protected)
  6913			sb_end_intwrite(dquot->dq_sb);
  6914	
  6915		return ret;
  6916	}
  6917	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki