* [PATCH] quota: don't let mark_dquot_dirty() fail silently
@ 2024-04-07 7:31 Chao Yu
2024-04-08 14:30 ` Jan Kara
2024-04-09 6:31 ` wangjianjian (C)
0 siblings, 2 replies; 4+ messages in thread
From: Chao Yu @ 2024-04-07 7:31 UTC (permalink / raw)
To: jack; +Cc: linux-fsdevel, linux-kernel, Chao Yu
mark_dquot_dirty() will callback to specified filesystem function,
it may fail due to any reasons, however, no caller will check return
value of mark_dquot_dirty(), so, it may fail silently, let's print
one line message for such case.
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/quota/dquot.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index dacbee455c03..c5df7863942a 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -399,21 +399,20 @@ int dquot_mark_dquot_dirty(struct dquot *dquot)
EXPORT_SYMBOL(dquot_mark_dquot_dirty);
/* Dirtify all the dquots - this can block when journalling */
-static inline int mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
+static inline void mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
{
- int ret, err, cnt;
+ int ret, cnt;
struct dquot *dquot;
- ret = err = 0;
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
dquot = srcu_dereference(dquots[cnt], &dquot_srcu);
- if (dquot)
- /* Even in case of error we have to continue */
- ret = mark_dquot_dirty(dquot);
- if (!err)
- err = ret;
+ if (!dquot)
+ continue;
+ ret = mark_dquot_dirty(dquot);
+ if (ret < 0)
+ quota_error(dquot->dq_sb,
+ "mark_all_dquot_dirty fails, ret: %d", ret);
}
- return err;
}
static inline void dqput_all(struct dquot **dquot)
@@ -2725,6 +2724,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;
+ int ret;
struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
if (di->d_fieldmask & ~VFS_QC_MASK)
@@ -2807,7 +2807,10 @@ 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)
+ quota_error(dquot->dq_sb,
+ "mark_dquot_dirty fails, ret: %d", ret);
return 0;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] quota: don't let mark_dquot_dirty() fail silently
2024-04-07 7:31 [PATCH] quota: don't let mark_dquot_dirty() fail silently Chao Yu
@ 2024-04-08 14:30 ` Jan Kara
2024-04-11 10:27 ` Chao Yu
2024-04-09 6:31 ` wangjianjian (C)
1 sibling, 1 reply; 4+ messages in thread
From: Jan Kara @ 2024-04-08 14:30 UTC (permalink / raw)
To: Chao Yu; +Cc: jack, linux-fsdevel, linux-kernel
On Sun 07-04-24 15:31:28, Chao Yu wrote:
> mark_dquot_dirty() will callback to specified filesystem function,
> it may fail due to any reasons, however, no caller will check return
> value of mark_dquot_dirty(), so, it may fail silently, let's print
> one line message for such case.
>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/quota/dquot.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index dacbee455c03..c5df7863942a 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -399,21 +399,20 @@ int dquot_mark_dquot_dirty(struct dquot *dquot)
> EXPORT_SYMBOL(dquot_mark_dquot_dirty);
>
> /* Dirtify all the dquots - this can block when journalling */
> -static inline int mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
> +static inline void mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
> {
> - int ret, err, cnt;
> + int ret, cnt;
> struct dquot *dquot;
>
> - ret = err = 0;
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> dquot = srcu_dereference(dquots[cnt], &dquot_srcu);
> - if (dquot)
> - /* Even in case of error we have to continue */
> - ret = mark_dquot_dirty(dquot);
> - if (!err)
> - err = ret;
> + if (!dquot)
> + continue;
> + ret = mark_dquot_dirty(dquot);
> + if (ret < 0)
> + quota_error(dquot->dq_sb,
> + "mark_all_dquot_dirty fails, ret: %d", ret);
Do you have any practical case you care about? Because in practice the
filesystem will usually report if there's some catastrophic error (and the
errors from ->mark_dirty() all mean the filesystem is in unhealthy state).
So this message just adds to the noise in the error log - and e.g. if the
disk goes bad so we cannot write, we could spew a lot of messages like
this.
> }
> - return err;
> }
>
> static inline void dqput_all(struct dquot **dquot)
> @@ -2725,6 +2724,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;
> + int ret;
> struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
>
> if (di->d_fieldmask & ~VFS_QC_MASK)
> @@ -2807,7 +2807,10 @@ 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)
> + quota_error(dquot->dq_sb,
> + "mark_dquot_dirty fails, ret: %d", ret);
Here, we can propagate the error back to userspace, which is probably
better than spamming the logs.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] quota: don't let mark_dquot_dirty() fail silently
2024-04-07 7:31 [PATCH] quota: don't let mark_dquot_dirty() fail silently Chao Yu
2024-04-08 14:30 ` Jan Kara
@ 2024-04-09 6:31 ` wangjianjian (C)
1 sibling, 0 replies; 4+ messages in thread
From: wangjianjian (C) @ 2024-04-09 6:31 UTC (permalink / raw)
To: Chao Yu, jack; +Cc: linux-fsdevel, linux-kernel
On 2024/4/7 15:31, Chao Yu wrote:
> mark_dquot_dirty() will callback to specified filesystem function,
> it may fail due to any reasons, however, no caller will check return
> value of mark_dquot_dirty(), so, it may fail silently, let's print
> one line message for such case.
>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/quota/dquot.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index dacbee455c03..c5df7863942a 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -399,21 +399,20 @@ int dquot_mark_dquot_dirty(struct dquot *dquot)
> EXPORT_SYMBOL(dquot_mark_dquot_dirty);
>
> /* Dirtify all the dquots - this can block when journalling */
> -static inline int mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
> +static inline void mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
> {
> - int ret, err, cnt;
> + int ret, cnt;
> struct dquot *dquot;
>
> - ret = err = 0;
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> dquot = srcu_dereference(dquots[cnt], &dquot_srcu);
> - if (dquot)
> - /* Even in case of error we have to continue */
> - ret = mark_dquot_dirty(dquot);
> - if (!err)
> - err = ret;
> + if (!dquot)
> + continue;
> + ret = mark_dquot_dirty(dquot);
> + if (ret < 0)
> + quota_error(dquot->dq_sb,
> + "mark_all_dquot_dirty fails, ret: %d", ret);
> }
> - return err;
> }
>
> static inline void dqput_all(struct dquot **dquot)
> @@ -2725,6 +2724,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;
> + int ret;
> struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
>
> if (di->d_fieldmask & ~VFS_QC_MASK)
> @@ -2807,7 +2807,10 @@ 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);
Here it overwrite previous error.
> + if (ret < 0)
> + quota_error(dquot->dq_sb,
> + "mark_dquot_dirty fails, ret: %d", ret);
>
> return 0;
> }
--
Regards
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] quota: don't let mark_dquot_dirty() fail silently
2024-04-08 14:30 ` Jan Kara
@ 2024-04-11 10:27 ` Chao Yu
0 siblings, 0 replies; 4+ messages in thread
From: Chao Yu @ 2024-04-11 10:27 UTC (permalink / raw)
To: Jan Kara; +Cc: jack, linux-fsdevel, linux-kernel
On 2024/4/8 22:30, Jan Kara wrote:
> On Sun 07-04-24 15:31:28, Chao Yu wrote:
>> mark_dquot_dirty() will callback to specified filesystem function,
>> it may fail due to any reasons, however, no caller will check return
>> value of mark_dquot_dirty(), so, it may fail silently, let's print
>> one line message for such case.
>>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>> fs/quota/dquot.c | 23 +++++++++++++----------
>> 1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
>> index dacbee455c03..c5df7863942a 100644
>> --- a/fs/quota/dquot.c
>> +++ b/fs/quota/dquot.c
>> @@ -399,21 +399,20 @@ int dquot_mark_dquot_dirty(struct dquot *dquot)
>> EXPORT_SYMBOL(dquot_mark_dquot_dirty);
>>
>> /* Dirtify all the dquots - this can block when journalling */
>> -static inline int mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
>> +static inline void mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
>> {
>> - int ret, err, cnt;
>> + int ret, cnt;
>> struct dquot *dquot;
>>
>> - ret = err = 0;
>> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>> dquot = srcu_dereference(dquots[cnt], &dquot_srcu);
>> - if (dquot)
>> - /* Even in case of error we have to continue */
>> - ret = mark_dquot_dirty(dquot);
>> - if (!err)
>> - err = ret;
>> + if (!dquot)
>> + continue;
>> + ret = mark_dquot_dirty(dquot);
>> + if (ret < 0)
>> + quota_error(dquot->dq_sb,
>> + "mark_all_dquot_dirty fails, ret: %d", ret);
>
> Do you have any practical case you care about? Because in practice the
Actually, no.
> filesystem will usually report if there's some catastrophic error (and the
> errors from ->mark_dirty() all mean the filesystem is in unhealthy state).
> So this message just adds to the noise in the error log - and e.g. if the
> disk goes bad so we cannot write, we could spew a lot of messages like
> this.
Agreed,
I guess we can propagate the error to caller rather than printing redundant
message in log.
>
>> }
>> - return err;
>> }
>>
>> static inline void dqput_all(struct dquot **dquot)
>> @@ -2725,6 +2724,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;
>> + int ret;
>> struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
>>
>> if (di->d_fieldmask & ~VFS_QC_MASK)
>> @@ -2807,7 +2807,10 @@ 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)
>> + quota_error(dquot->dq_sb,
>> + "mark_dquot_dirty fails, ret: %d", ret);
>
> Here, we can propagate the error back to userspace, which is probably
> better than spamming the logs.
Yes, let me submit a new patch for this.
Thanks,
>
> Honza
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-11 10:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-07 7:31 [PATCH] quota: don't let mark_dquot_dirty() fail silently Chao Yu
2024-04-08 14:30 ` Jan Kara
2024-04-11 10:27 ` Chao Yu
2024-04-09 6:31 ` wangjianjian (C)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).