As described in commit 02117b8ae9c0 ("f2fs: Set GF_NOFS in
read_cache_page_gfp while doing f2fs_quota_read"), we must not enter
filesystem reclaim while holding the dq_lock. Prevent this more generally
by using memalloc_nofs_save() while holding the lock.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/quota/dquot.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index a74aef99bd3d..cdb22d6d7488 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -425,9 +425,11 @@ EXPORT_SYMBOL(mark_info_dirty);
int dquot_acquire(struct dquot *dquot)
{
int ret = 0, ret2 = 0;
+ unsigned int memalloc;
struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
mutex_lock(&dquot->dq_lock);
+ memalloc = memalloc_nofs_save();
if (!test_bit(DQ_READ_B, &dquot->dq_flags)) {
ret = dqopt->ops[dquot->dq_id.type]->read_dqblk(dquot);
if (ret < 0)
@@ -458,6 +460,7 @@ int dquot_acquire(struct dquot *dquot)
smp_mb__before_atomic();
set_bit(DQ_ACTIVE_B, &dquot->dq_flags);
out_iolock:
+ memalloc_nofs_restore(memalloc);
mutex_unlock(&dquot->dq_lock);
return ret;
}
@@ -469,9 +472,11 @@ EXPORT_SYMBOL(dquot_acquire);
int dquot_commit(struct dquot *dquot)
{
int ret = 0;
+ unsigned int memalloc;
struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
mutex_lock(&dquot->dq_lock);
+ memalloc = memalloc_nofs_save();
if (!clear_dquot_dirty(dquot))
goto out_lock;
/* Inactive dquot can be only if there was error during read/init
@@ -481,6 +486,7 @@ int dquot_commit(struct dquot *dquot)
else
ret = -EIO;
out_lock:
+ memalloc_nofs_restore(memalloc);
mutex_unlock(&dquot->dq_lock);
return ret;
}
@@ -492,9 +498,11 @@ EXPORT_SYMBOL(dquot_commit);
int dquot_release(struct dquot *dquot)
{
int ret = 0, ret2 = 0;
+ unsigned int memalloc;
struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
mutex_lock(&dquot->dq_lock);
+ memalloc = memalloc_nofs_save();
/* Check whether we are not racing with some other dqget() */
if (dquot_is_busy(dquot))
goto out_dqlock;
@@ -510,6 +518,7 @@ int dquot_release(struct dquot *dquot)
}
clear_bit(DQ_ACTIVE_B, &dquot->dq_flags);
out_dqlock:
+ memalloc_nofs_restore(memalloc);
mutex_unlock(&dquot->dq_lock);
return ret;
}
--
2.35.1
On Sun 05-06-22 15:38:13, Matthew Wilcox (Oracle) wrote:
> As described in commit 02117b8ae9c0 ("f2fs: Set GF_NOFS in
> read_cache_page_gfp while doing f2fs_quota_read"), we must not enter
> filesystem reclaim while holding the dq_lock. Prevent this more generally
> by using memalloc_nofs_save() while holding the lock.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
This is definitely a good cleanup to have and seems mostly unrelated to the
rest. I'll take it rightaway into my tree. Thanks for the patch!
Honza
> ---
> fs/quota/dquot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index a74aef99bd3d..cdb22d6d7488 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -425,9 +425,11 @@ EXPORT_SYMBOL(mark_info_dirty);
> int dquot_acquire(struct dquot *dquot)
> {
> int ret = 0, ret2 = 0;
> + unsigned int memalloc;
> struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
>
> mutex_lock(&dquot->dq_lock);
> + memalloc = memalloc_nofs_save();
> if (!test_bit(DQ_READ_B, &dquot->dq_flags)) {
> ret = dqopt->ops[dquot->dq_id.type]->read_dqblk(dquot);
> if (ret < 0)
> @@ -458,6 +460,7 @@ int dquot_acquire(struct dquot *dquot)
> smp_mb__before_atomic();
> set_bit(DQ_ACTIVE_B, &dquot->dq_flags);
> out_iolock:
> + memalloc_nofs_restore(memalloc);
> mutex_unlock(&dquot->dq_lock);
> return ret;
> }
> @@ -469,9 +472,11 @@ EXPORT_SYMBOL(dquot_acquire);
> int dquot_commit(struct dquot *dquot)
> {
> int ret = 0;
> + unsigned int memalloc;
> struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
>
> mutex_lock(&dquot->dq_lock);
> + memalloc = memalloc_nofs_save();
> if (!clear_dquot_dirty(dquot))
> goto out_lock;
> /* Inactive dquot can be only if there was error during read/init
> @@ -481,6 +486,7 @@ int dquot_commit(struct dquot *dquot)
> else
> ret = -EIO;
> out_lock:
> + memalloc_nofs_restore(memalloc);
> mutex_unlock(&dquot->dq_lock);
> return ret;
> }
> @@ -492,9 +498,11 @@ EXPORT_SYMBOL(dquot_commit);
> int dquot_release(struct dquot *dquot)
> {
> int ret = 0, ret2 = 0;
> + unsigned int memalloc;
> struct quota_info *dqopt = sb_dqopt(dquot->dq_sb);
>
> mutex_lock(&dquot->dq_lock);
> + memalloc = memalloc_nofs_save();
> /* Check whether we are not racing with some other dqget() */
> if (dquot_is_busy(dquot))
> goto out_dqlock;
> @@ -510,6 +518,7 @@ int dquot_release(struct dquot *dquot)
> }
> clear_bit(DQ_ACTIVE_B, &dquot->dq_flags);
> out_dqlock:
> + memalloc_nofs_restore(memalloc);
> mutex_unlock(&dquot->dq_lock);
> return ret;
> }
> --
> 2.35.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Jun 06, 2022 at 10:03:34AM +0200, Jan Kara wrote:
> On Sun 05-06-22 15:38:13, Matthew Wilcox (Oracle) wrote:
> > As described in commit 02117b8ae9c0 ("f2fs: Set GF_NOFS in
> > read_cache_page_gfp while doing f2fs_quota_read"), we must not enter
> > filesystem reclaim while holding the dq_lock. Prevent this more generally
> > by using memalloc_nofs_save() while holding the lock.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
>
> This is definitely a good cleanup to have and seems mostly unrelated to the
> rest. I'll take it rightaway into my tree. Thanks for the patch!
Thanks! It's really a pre-requisite for the second patch; I haven't
seen anywhere in the current codebase that will have a problem. All
the buffer_heads are allocated with GFP_NOFS | __GFP_NOFAIL (in
grow_dev_page()).
On Mon 06-06-22 13:42:10, Matthew Wilcox wrote:
> On Mon, Jun 06, 2022 at 10:03:34AM +0200, Jan Kara wrote:
> > On Sun 05-06-22 15:38:13, Matthew Wilcox (Oracle) wrote:
> > > As described in commit 02117b8ae9c0 ("f2fs: Set GF_NOFS in
> > > read_cache_page_gfp while doing f2fs_quota_read"), we must not enter
> > > filesystem reclaim while holding the dq_lock. Prevent this more generally
> > > by using memalloc_nofs_save() while holding the lock.
> > >
> > > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> >
> > This is definitely a good cleanup to have and seems mostly unrelated to the
> > rest. I'll take it rightaway into my tree. Thanks for the patch!
>
> Thanks! It's really a pre-requisite for the second patch; I haven't
> seen anywhere in the current codebase that will have a problem. All
> the buffer_heads are allocated with GFP_NOFS | __GFP_NOFAIL (in
> grow_dev_page()).
Yes, I understand. But as f2fs case shows, there can be fs-local
allocations that may be impacted. And it is good to have it documented in
the code that dq_lock is not reclaim safe to avoid bugs like f2fs had in
the future.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR