| Message ID | 20240412094942.2131243-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | quota: fix to propagate error of mark_dquot_dirty() to caller | expand |
On Fri 12-04-24 17:49:42, Chao Yu wrote: > in order to let caller be aware of failure of mark_dquot_dirty(). > > Signed-off-by: Chao Yu <chao@kernel.org> Thanks. I've added the patch to my tree. Honza > --- > fs/quota/dquot.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index dacbee455c03..b2a109d8b198 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1737,7 +1737,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags) > > if (reserve) > goto out_flush_warn; > - mark_all_dquot_dirty(dquots); > + ret = mark_all_dquot_dirty(dquots); > out_flush_warn: > srcu_read_unlock(&dquot_srcu, index); > flush_warnings(warn); > @@ -1786,7 +1786,7 @@ int dquot_alloc_inode(struct inode *inode) > warn_put_all: > spin_unlock(&inode->i_lock); > if (ret == 0) > - mark_all_dquot_dirty(dquots); > + ret = mark_all_dquot_dirty(dquots); > srcu_read_unlock(&dquot_srcu, index); > flush_warnings(warn); > return ret; > @@ -1990,7 +1990,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > qsize_t inode_usage = 1; > struct dquot __rcu **dquots; > struct dquot *transfer_from[MAXQUOTAS] = {}; > - int cnt, index, ret = 0; > + int cnt, index, ret = 0, err; > char is_valid[MAXQUOTAS] = {}; > struct dquot_warn warn_to[MAXQUOTAS]; > struct dquot_warn warn_from_inodes[MAXQUOTAS]; > @@ -2087,8 +2087,12 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > * mark_all_dquot_dirty(). > */ > index = srcu_read_lock(&dquot_srcu); > - mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); > - mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); > + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); > + if (err < 0) > + ret = err; > + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); > + if (err < 0) > + ret = err; > srcu_read_unlock(&dquot_srcu, index); > > flush_warnings(warn_to); > @@ -2098,7 +2102,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > for (cnt = 0; cnt < MAXQUOTAS; cnt++) > if (is_valid[cnt]) > transfer_to[cnt] = transfer_from[cnt]; > - return 0; > + return ret; > over_quota: > /* Back out changes we already did */ > for (cnt--; cnt >= 0; cnt--) { > @@ -2726,6 +2730,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) > struct mem_dqblk *dm = &dquot->dq_dqb; > int check_blim = 0, check_ilim = 0; > struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type]; > + int ret; > > if (di->d_fieldmask & ~VFS_QC_MASK) > return -EINVAL; > @@ -2807,7 +2812,9 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) > else > set_bit(DQ_FAKE_B, &dquot->dq_flags); > spin_unlock(&dquot->dq_dqb_lock); > - mark_dquot_dirty(dquot); > + ret = mark_dquot_dirty(dquot); > + if (ret < 0) > + return ret; > > return 0; > } > -- > 2.40.1 >
On Fri 12-04-24 14:15:17, Jan Kara wrote: > On Fri 12-04-24 17:49:42, Chao Yu wrote: > > in order to let caller be aware of failure of mark_dquot_dirty(). > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > Thanks. I've added the patch to my tree. So this patch was buggy because mark_all_dquots() dirty was returning 1 in case some dquot was indeed dirtied which resulted in e.g. dquot_alloc_inode() to return 1 and consequently __ext4_new_inode() to fail and eventually we've crashed in ext4_create(). I've fixed up the patch to make mark_all_dquots() return 0 or error. Honza > > --- > > fs/quota/dquot.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > > index dacbee455c03..b2a109d8b198 100644 > > --- a/fs/quota/dquot.c > > +++ b/fs/quota/dquot.c > > @@ -1737,7 +1737,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags) > > > > if (reserve) > > goto out_flush_warn; > > - mark_all_dquot_dirty(dquots); > > + ret = mark_all_dquot_dirty(dquots); > > out_flush_warn: > > srcu_read_unlock(&dquot_srcu, index); > > flush_warnings(warn); > > @@ -1786,7 +1786,7 @@ int dquot_alloc_inode(struct inode *inode) > > warn_put_all: > > spin_unlock(&inode->i_lock); > > if (ret == 0) > > - mark_all_dquot_dirty(dquots); > > + ret = mark_all_dquot_dirty(dquots); > > srcu_read_unlock(&dquot_srcu, index); > > flush_warnings(warn); > > return ret; > > @@ -1990,7 +1990,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > > qsize_t inode_usage = 1; > > struct dquot __rcu **dquots; > > struct dquot *transfer_from[MAXQUOTAS] = {}; > > - int cnt, index, ret = 0; > > + int cnt, index, ret = 0, err; > > char is_valid[MAXQUOTAS] = {}; > > struct dquot_warn warn_to[MAXQUOTAS]; > > struct dquot_warn warn_from_inodes[MAXQUOTAS]; > > @@ -2087,8 +2087,12 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > > * mark_all_dquot_dirty(). > > */ > > index = srcu_read_lock(&dquot_srcu); > > - mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); > > - mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); > > + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); > > + if (err < 0) > > + ret = err; > > + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); > > + if (err < 0) > > + ret = err; > > srcu_read_unlock(&dquot_srcu, index); > > > > flush_warnings(warn_to); > > @@ -2098,7 +2102,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) > > for (cnt = 0; cnt < MAXQUOTAS; cnt++) > > if (is_valid[cnt]) > > transfer_to[cnt] = transfer_from[cnt]; > > - return 0; > > + return ret; > > over_quota: > > /* Back out changes we already did */ > > for (cnt--; cnt >= 0; cnt--) { > > @@ -2726,6 +2730,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) > > struct mem_dqblk *dm = &dquot->dq_dqb; > > int check_blim = 0, check_ilim = 0; > > struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type]; > > + int ret; > > > > if (di->d_fieldmask & ~VFS_QC_MASK) > > return -EINVAL; > > @@ -2807,7 +2812,9 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) > > else > > set_bit(DQ_FAKE_B, &dquot->dq_flags); > > spin_unlock(&dquot->dq_dqb_lock); > > - mark_dquot_dirty(dquot); > > + ret = mark_dquot_dirty(dquot); > > + if (ret < 0) > > + return ret; > > > > return 0; > > } > > -- > > 2.40.1 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR >
On 2024/4/12 21:01, Jan Kara wrote: > On Fri 12-04-24 14:15:17, Jan Kara wrote: >> On Fri 12-04-24 17:49:42, Chao Yu wrote: >>> in order to let caller be aware of failure of mark_dquot_dirty(). >>> >>> Signed-off-by: Chao Yu <chao@kernel.org> >> >> Thanks. I've added the patch to my tree. > > So this patch was buggy because mark_all_dquots() dirty was returning 1 in > case some dquot was indeed dirtied which resulted in e.g. > dquot_alloc_inode() to return 1 and consequently __ext4_new_inode() to fail Correct, I missed that case. > and eventually we've crashed in ext4_create(). I've fixed up the patch to > make mark_all_dquots() return 0 or error. Thank you for catching and fixing it. Thanks, > > Honza > >>> --- >>> fs/quota/dquot.c | 21 ++++++++++++++------- >>> 1 file changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c >>> index dacbee455c03..b2a109d8b198 100644 >>> --- a/fs/quota/dquot.c >>> +++ b/fs/quota/dquot.c >>> @@ -1737,7 +1737,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags) >>> >>> if (reserve) >>> goto out_flush_warn; >>> - mark_all_dquot_dirty(dquots); >>> + ret = mark_all_dquot_dirty(dquots); >>> out_flush_warn: >>> srcu_read_unlock(&dquot_srcu, index); >>> flush_warnings(warn); >>> @@ -1786,7 +1786,7 @@ int dquot_alloc_inode(struct inode *inode) >>> warn_put_all: >>> spin_unlock(&inode->i_lock); >>> if (ret == 0) >>> - mark_all_dquot_dirty(dquots); >>> + ret = mark_all_dquot_dirty(dquots); >>> srcu_read_unlock(&dquot_srcu, index); >>> flush_warnings(warn); >>> return ret; >>> @@ -1990,7 +1990,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> qsize_t inode_usage = 1; >>> struct dquot __rcu **dquots; >>> struct dquot *transfer_from[MAXQUOTAS] = {}; >>> - int cnt, index, ret = 0; >>> + int cnt, index, ret = 0, err; >>> char is_valid[MAXQUOTAS] = {}; >>> struct dquot_warn warn_to[MAXQUOTAS]; >>> struct dquot_warn warn_from_inodes[MAXQUOTAS]; >>> @@ -2087,8 +2087,12 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> * mark_all_dquot_dirty(). >>> */ >>> index = srcu_read_lock(&dquot_srcu); >>> - mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); >>> - mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); >>> + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); >>> + if (err < 0) >>> + ret = err; >>> + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); >>> + if (err < 0) >>> + ret = err; >>> srcu_read_unlock(&dquot_srcu, index); >>> >>> flush_warnings(warn_to); >>> @@ -2098,7 +2102,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> for (cnt = 0; cnt < MAXQUOTAS; cnt++) >>> if (is_valid[cnt]) >>> transfer_to[cnt] = transfer_from[cnt]; >>> - return 0; >>> + return ret; >>> over_quota: >>> /* Back out changes we already did */ >>> for (cnt--; cnt >= 0; cnt--) { >>> @@ -2726,6 +2730,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) >>> struct mem_dqblk *dm = &dquot->dq_dqb; >>> int check_blim = 0, check_ilim = 0; >>> struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type]; >>> + int ret; >>> >>> if (di->d_fieldmask & ~VFS_QC_MASK) >>> return -EINVAL; >>> @@ -2807,7 +2812,9 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) >>> else >>> set_bit(DQ_FAKE_B, &dquot->dq_flags); >>> spin_unlock(&dquot->dq_dqb_lock); >>> - mark_dquot_dirty(dquot); >>> + ret = mark_dquot_dirty(dquot); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> -- >>> 2.40.1 >>> >> -- >> Jan Kara <jack@suse.com> >> SUSE Labs, CR >>
On 2024/4/12 21:01, Jan Kara wrote: > On Fri 12-04-24 14:15:17, Jan Kara wrote: >> On Fri 12-04-24 17:49:42, Chao Yu wrote: >>> in order to let caller be aware of failure of mark_dquot_dirty(). >>> >>> Signed-off-by: Chao Yu <chao@kernel.org> >> >> Thanks. I've added the patch to my tree. > > So this patch was buggy because mark_all_dquots() dirty was returning 1 in > case some dquot was indeed dirtied which resulted in e.g. > dquot_alloc_inode() to return 1 and consequently __ext4_new_inode() to fail This is what I try to say in my another f2fs patch, some callers use if return value is zero not *less than zero* to check success or not. Luckily, maybe f2fs doesn't use it this way. > and eventually we've crashed in ext4_create(). I've fixed up the patch to > make mark_all_dquots() return 0 or error. > > Honza > >>> --- >>> fs/quota/dquot.c | 21 ++++++++++++++------- >>> 1 file changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c >>> index dacbee455c03..b2a109d8b198 100644 >>> --- a/fs/quota/dquot.c >>> +++ b/fs/quota/dquot.c >>> @@ -1737,7 +1737,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags) >>> >>> if (reserve) >>> goto out_flush_warn; >>> - mark_all_dquot_dirty(dquots); >>> + ret = mark_all_dquot_dirty(dquots); >>> out_flush_warn: >>> srcu_read_unlock(&dquot_srcu, index); >>> flush_warnings(warn); >>> @@ -1786,7 +1786,7 @@ int dquot_alloc_inode(struct inode *inode) >>> warn_put_all: >>> spin_unlock(&inode->i_lock); >>> if (ret == 0) >>> - mark_all_dquot_dirty(dquots); >>> + ret = mark_all_dquot_dirty(dquots); >>> srcu_read_unlock(&dquot_srcu, index); >>> flush_warnings(warn); >>> return ret; >>> @@ -1990,7 +1990,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> qsize_t inode_usage = 1; >>> struct dquot __rcu **dquots; >>> struct dquot *transfer_from[MAXQUOTAS] = {}; >>> - int cnt, index, ret = 0; >>> + int cnt, index, ret = 0, err; >>> char is_valid[MAXQUOTAS] = {}; >>> struct dquot_warn warn_to[MAXQUOTAS]; >>> struct dquot_warn warn_from_inodes[MAXQUOTAS]; >>> @@ -2087,8 +2087,12 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> * mark_all_dquot_dirty(). >>> */ >>> index = srcu_read_lock(&dquot_srcu); >>> - mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); >>> - mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); >>> + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); >>> + if (err < 0) >>> + ret = err; >>> + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); >>> + if (err < 0) >>> + ret = err; >>> srcu_read_unlock(&dquot_srcu, index); >>> >>> flush_warnings(warn_to); >>> @@ -2098,7 +2102,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) >>> for (cnt = 0; cnt < MAXQUOTAS; cnt++) >>> if (is_valid[cnt]) >>> transfer_to[cnt] = transfer_from[cnt]; >>> - return 0; >>> + return ret; >>> over_quota: >>> /* Back out changes we already did */ >>> for (cnt--; cnt >= 0; cnt--) { >>> @@ -2726,6 +2730,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) >>> struct mem_dqblk *dm = &dquot->dq_dqb; >>> int check_blim = 0, check_ilim = 0; >>> struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type]; >>> + int ret; >>> >>> if (di->d_fieldmask & ~VFS_QC_MASK) >>> return -EINVAL; >>> @@ -2807,7 +2812,9 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) >>> else >>> set_bit(DQ_FAKE_B, &dquot->dq_flags); >>> spin_unlock(&dquot->dq_dqb_lock); >>> - mark_dquot_dirty(dquot); >>> + ret = mark_dquot_dirty(dquot); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> -- >>> 2.40.1 >>> >> -- >> Jan Kara <jack@suse.com> >> SUSE Labs, CR >>
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index dacbee455c03..b2a109d8b198 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1737,7 +1737,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags) if (reserve) goto out_flush_warn; - mark_all_dquot_dirty(dquots); + ret = mark_all_dquot_dirty(dquots); out_flush_warn: srcu_read_unlock(&dquot_srcu, index); flush_warnings(warn); @@ -1786,7 +1786,7 @@ int dquot_alloc_inode(struct inode *inode) warn_put_all: spin_unlock(&inode->i_lock); if (ret == 0) - mark_all_dquot_dirty(dquots); + ret = mark_all_dquot_dirty(dquots); srcu_read_unlock(&dquot_srcu, index); flush_warnings(warn); return ret; @@ -1990,7 +1990,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) qsize_t inode_usage = 1; struct dquot __rcu **dquots; struct dquot *transfer_from[MAXQUOTAS] = {}; - int cnt, index, ret = 0; + int cnt, index, ret = 0, err; char is_valid[MAXQUOTAS] = {}; struct dquot_warn warn_to[MAXQUOTAS]; struct dquot_warn warn_from_inodes[MAXQUOTAS]; @@ -2087,8 +2087,12 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) * mark_all_dquot_dirty(). */ index = srcu_read_lock(&dquot_srcu); - mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); - mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_from); + if (err < 0) + ret = err; + err = mark_all_dquot_dirty((struct dquot __rcu **)transfer_to); + if (err < 0) + ret = err; srcu_read_unlock(&dquot_srcu, index); flush_warnings(warn_to); @@ -2098,7 +2102,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to) for (cnt = 0; cnt < MAXQUOTAS; cnt++) if (is_valid[cnt]) transfer_to[cnt] = transfer_from[cnt]; - return 0; + return ret; over_quota: /* Back out changes we already did */ for (cnt--; cnt >= 0; cnt--) { @@ -2726,6 +2730,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) struct mem_dqblk *dm = &dquot->dq_dqb; int check_blim = 0, check_ilim = 0; struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type]; + int ret; if (di->d_fieldmask & ~VFS_QC_MASK) return -EINVAL; @@ -2807,7 +2812,9 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di) else set_bit(DQ_FAKE_B, &dquot->dq_flags); spin_unlock(&dquot->dq_dqb_lock); - mark_dquot_dirty(dquot); + ret = mark_dquot_dirty(dquot); + if (ret < 0) + return ret; return 0; }
in order to let caller be aware of failure of mark_dquot_dirty(). Signed-off-by: Chao Yu <chao@kernel.org> --- fs/quota/dquot.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)