| Message ID | 20220605143815.2330891-2-willy@infradead.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Cache quota files in the page cache | expand |
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) <willy@infradead.org> 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 >
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) <willy@infradead.org> > > 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) <willy@infradead.org> > > > > 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
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; }
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) <willy@infradead.org> --- fs/quota/dquot.c | 9 +++++++++ 1 file changed, 9 insertions(+)