Re: [PATCH] fs/ext4: get project quota from inode for mangling statfs results

From: Jan Kara
Date: Wed Oct 30 2019 - 08:57:08 EST

Next message: YueHaibing: "[PATCH -next] clk: tegra: Fix build error without CONFIG_PM_SLEEP"
Previous message: Amit Kucheria: "Re: [PATCH][next] drivers: thermal: tsens: fix potential integer overflow on multiply"
In reply to: Konstantin Khlebnikov: "Re: [PATCH] fs/ext4: get project quota from inode for mangling statfs results"
Next in thread: Dmitry Monakhov: "[PATCH] fs/ext4: get project quota from inode for mangling statfs results"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed 30-10-19 15:06:13, ÐÐÐÑÑÐÐ ÐÐÐÐÑÐÐ wrote:
> Â
> Â
> 30.10.2019, 13:59, "Jan Kara" <jack@xxxxxxx>:
>
>
> On Mon 28-10-19 13:38:43, Konstantin Khlebnikov wrote:
>
> ÂRight now ext4_statfs_project() does quota lookup by id every time.
> ÂThis is costly operation, especially if there is no inode who hold
> Âreference to this quota and dqget() reads it from disk each time.
>
> ÂFunction ext4_statfs_project() could be moved into generic quota code,
> Âit is required for every filesystem which uses generic project quota.
>
> ÂReported-by: Dmitry Monakhov <dmtrmonakhov@xxxxxxxxxxxxxx>
> ÂSigned-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
> Â---
> ÂÂfs/ext4/super.c | 25 ++++++++++++++++---------
> ÂÂ1 file changed, 16 insertions(+), 9 deletions(-)
>
> Âdiff --git a/fs/ext4/super.c b/fs/ext4/super.c
> Âindex dd654e53ba3d..f841c66aa499 100644
> Â--- a/fs/ext4/super.c
> Â+++ b/fs/ext4/super.c
> Â@@ -5532,18 +5532,23 @@ static int ext4_remount(struct super_block
> *sb, int *flags, char *data)
> ÂÂ}
>
> ÂÂ#ifdef CONFIG_QUOTA
> Â-static int ext4_statfs_project(struct super_block *sb,
> Â- kprojid_t projid, struct kstatfs *buf)
> Â+static int ext4_statfs_project(struct inode *inode, struct kstatfs
> *buf)
> ÂÂ{
> Â- struct kqid qid;
> Â+ struct super_block *sb = inode->i_sb;
> ÂÂÂÂÂÂÂÂÂÂstruct dquot *dquot;
> ÂÂÂÂÂÂÂÂÂÂu64 limit;
> ÂÂÂÂÂÂÂÂÂÂu64 curblock;
> Â+ int err;
> Â+
> Â+ err = dquot_initialize(inode);
>
>
> Hum, I'm kind of puzzled here: Your patch seems to be concerned with
> performance but how is this any faster than what we do now?
> dquot_initialize() will look up three dquots instead of one in the current
> code? Oh, I guess you are concerned about *repeated* calls to statfs() and
> thus repeated lookups of dquot structure? And this patch effectively caches
> looked up dquots in the inode?
>
> That starts to make some sense but still, even if dquot isn't cached in any
> inode, we still hold on to it (it's in the free_list) until shrinker evicts
> it. So lookup of such dquot should be just a hash table lookup which should
> be very fast. Then there's the cost of dquot_acquire() / dquot_release()
> that get always called on first / last get of a dquot. So are you concerned
> about that cost? Or do you really see IO happening to fetch quota structure
> on each statfs call again and again?
>
> Hi,
> No IO, only useless synchronization on journal
> Repeaded statfs result in dquot_acquire()/ dquot_release() which result in two
> ext4_journal_starts
> perf record -e 'ext4:*' -e 'jbd2:*' Âstat -f Âvolume
> perf script
> Â Â Â Â Â Âstat 520596 [002] 589927.123955: Â Â Â Â Â Â Â Â Â Â Â
> ext4:ext4_journal_start: dev 252,2 blocks, 73 rsv_blocks, 0 caller
> ext4_acquire_dquot
> Â Â Â Â Â Â stat 520596 [002] 589927.123958: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_start: dev 252,2 tid 187859 type 6 line_no 5550
> requested_blocks 73
> Â Â Â Â Â Â stat 520596 [002] 589927.123959: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_stats: dev 252,2 tid 187859 type 6 line_no 5550 interval 0
> sync 0 requested_blocks 73 dirtied_blocks 0
> Â Â Â Â Â Â stat 520596 [002] 589927.123960: Â Â Â Â Â Â Â Â Â Â Â
> ext4:ext4_journal_start: dev 252,2 blocks, 9 rsv_blocks, 0 caller
> ext4_release_dquot
> Â Â Â Â Â Â stat 520596 [002] 589927.123961: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_start: dev 252,2 tid 187859 type 6 line_no 5566
> requested_blocks 9
> Â Â Â Â Â Â stat 520596 [002] 589927.123962: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_stats: dev 252,2 tid 187859 type 6 line_no 5566 interval 0
> sync 0 requested_blocks 9 dirtied_blocks 0
> On host under io load this will be blocked on __jbd2_log_wait_for_space() which
> is no what people expects from statfs()

OK, makes sense.

> The only situation where I could seethat happening is when the quota
> structure would be actually completely
> empty (i.e., not originally present in the quota file). But then this
> cannot be a case when there's actually an inode belonging to this
> project...
>
> So I'm really curious about the details of what you are seeing as the
> changelog / patch doesn't quite make sense to me yet.
>
> Â
> This indeed happens if project quota goes out of sync, which is quite simple
> for non journaled Âquota case.
> And this provoke huge IO penalty on each statfs

Yes, but then I wonder how it can happen that project quota is out of sync
because ext4 does not support non-journalled project quotas (project quotas
must be stored in hidden system inodes). So it is a fs bug if project quota
goes out of sync.

Anyway, case 1 you mentioned above still makes sense so please just update
the changelog explaining more details about the problem and why your
patch helps that. Thanks!

Honza

> Â
> $perf record -e 'ext4:*' -e 'jbd2:*' Âstat -f Âvolume-with-staled-quota
> $perf script
> Â Â Â Â Â Â stat 528212 [002] 591269.007915: Â Â Â Â Â Â Â Â Â Â Â
> ext4:ext4_journal_start: dev 252,2 blocks, 73 rsv_blocks, 0 caller
> ext4_acquire_dquot
> Â Â Â Â Â Â stat 528212 [002] 591269.007919: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_start: dev 252,2 tid 188107 type 6 line_no 5550
> requested_blocks 73
> Â Â Â Â Â Â stat 528212 [002] 591269.007922: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 0
> Â Â Â Â Â Â stat 528212 [002] 591269.007923: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [0/1) 190361090 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007926: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007926: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007928: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007928: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007929: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007930: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007931: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 1
> Â Â Â Â Â Â stat 528212 [002] 591269.007931: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [1/1) 138484739 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007933: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 1
> Â Â Â Â Â Â stat 528212 [002] 591269.007933: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [1/1) 138484739 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007936: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007936: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007938: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 1
> Â Â Â Â Â Â stat 528212 [002] 591269.007938: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [1/1) 138484739 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007940: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_stats: dev 252,2 tid 188107 type 6 line_no 5550 interval 0
> sync 0 requested_blocks 73 dirtied_blocks 2
> Â Â Â Â Â Â stat 528212 [002] 591269.007941: Â Â Â Â Â Â Â Â Â Â Â
> ext4:ext4_journal_start: dev 252,2 blocks, 9 rsv_blocks, 0 caller
> ext4_release_dquot
> Â Â Â Â Â Â stat 528212 [002] 591269.007941: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_start: dev 252,2 tid 188107 type 6 line_no 5566
> requested_blocks 9
> Â Â Â Â Â Â stat 528212 [002] 591269.007942: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 0
> Â Â Â Â Â Â stat 528212 [002] 591269.007943: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [0/1) 190361090 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007944: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007944: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007945: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007954: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007954: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007955: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007956: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 1
> Â Â Â Â Â Â stat 528212 [002] 591269.007956: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [1/1) 138484739 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007957: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 1
> Â Â Â Â Â Â stat 528212 [002] 591269.007957: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [1/1) 138484739 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007958: Â Â Â Â Â Â
> Âext4:ext4_es_lookup_extent_enter: dev 252,2 ino 13 lblk 3
> Â Â Â Â Â Â stat 528212 [002] 591269.007958: Â Â Â Â Â Â Â
> ext4:ext4_es_lookup_extent_exit: dev 252,2 ino 13 found 1 [3/1) 188785674 W
> Â Â Â Â Â Â stat 528212 [002] 591269.007959: Â Â Â Â Â Â Â Â Â Â Â
> Âjbd2:jbd2_handle_stats: dev 252,2 tid 188107 type 6 line_no 5566 interval 0
> sync 0 requested_blocks 9 dirtied_blocks 0
>
>
>
> Â
>
> Â+ if (err)
> Â+ return err;
> Â+
> Â+ spin_lock(&inode->i_lock);
> Â+ dquot = ext4_get_dquots(inode)[PRJQUOTA];
> Â+ if (!dquot)
> Â+ goto out_unlock;
>
> Â- qid = make_kqid_projid(projid);
> Â- dquot = dqget(sb, qid);
> Â- if (IS_ERR(dquot))
> Â- return PTR_ERR(dquot);
> ÂÂÂÂÂÂÂÂÂÂspin_lock(&dquot->dq_dqb_lock);
>
> ÂÂÂÂÂÂÂÂÂÂlimit = (dquot->dq_dqb.dqb_bsoftlimit ?
> Â@@ -5569,7 +5574,9 @@ static int ext4_statfs_project(struct
> super_block *sb,
> ÂÂÂÂÂÂÂÂÂÂ}
>
> ÂÂÂÂÂÂÂÂÂÂspin_unlock(&dquot->dq_dqb_lock);
> Â- dqput(dquot);
> Â+out_unlock:
> Â+ spin_unlock(&inode->i_lock);
> Â+
> ÂÂÂÂÂÂÂÂÂÂreturn 0;
> ÂÂ}
> ÂÂ#endif
> Â@@ -5609,7 +5616,7 @@ static int ext4_statfs(struct dentry *dentry,
> struct kstatfs *buf)
> ÂÂ#ifdef CONFIG_QUOTA
> ÂÂÂÂÂÂÂÂÂÂif (ext4_test_inode_flag(dentry->d_inode,
> EXT4_INODE_PROJINHERIT) &&
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂsb_has_quota_limits_enabled(sb, PRJQUOTA))
> Â- ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
> Â+ ext4_statfs_project(dentry->d_inode, buf);
> ÂÂ#endif
> ÂÂÂÂÂÂÂÂÂÂreturn 0;
> ÂÂ}
> Â
>
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR

Next message: YueHaibing: "[PATCH -next] clk: tegra: Fix build error without CONFIG_PM_SLEEP"
Previous message: Amit Kucheria: "Re: [PATCH][next] drivers: thermal: tsens: fix potential integer overflow on multiply"
In reply to: Konstantin Khlebnikov: "Re: [PATCH] fs/ext4: get project quota from inode for mangling statfs results"
Next in thread: Dmitry Monakhov: "[PATCH] fs/ext4: get project quota from inode for mangling statfs results"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]